Re: [PATCH v8 6/6] s390/vfio-ap: replace guest-reachable WARNs with ratelimited warnings

[Anthony Krowiak <akrowiak@linux.ibm.com>]

On 9/25/26 8:59 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] s390/vfio-ap: NULL pointer deref in vfio_ap_free_aqic_resources()
> - [Low] s390/vfio-ap: format specifiers truncate DMA addr and corrupt log
> --
>
> commit bb0d1a9a828269662641d7a1b6f445a55a9d0e1d
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
>
> s390/vfio-ap: replace guest-reachable WARNs with ratelimited warnings
>
> WARN and WARN_ONCE macros in code paths reachable by a guest
> can be triggered repeatedly by a malicious or misbehaving guest,
> flooding the kernel log and potentially impacting system
> stability. Replace all WARN and WARN_ONCE calls reachable from
> the guest AP interrupt enable/disable and queue reset paths with
> ratelimited warning functions.
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index cd4a436c43194..4b6e64daed253 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> [ ... ]
>> +static void report_gisc_unregister_failure(struct vfio_ap_queue *q)
>> +{
>> +	if (q->matrix_mdev) {
>> +		dev_warn_ratelimited(mdev_dev(q->matrix_mdev->mdev),
>> +				     "APQN %02x.%04x: Failed to unregister guest ISC %c\n",
>> +				     AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
>> +				     q->saved_isc);
> [Severity: Low]
> Could this result in missing or mangled information in the kernel log?
> The guest ISC is printed using %c, which will output a non-printable ASCII
> character for the numeric value, rather than the integer itself.

Will change to %d

>
>> +	} else {
>> +		pr_warn_ratelimited("APQN %02x.%04x: Failed to unregister guest ISC %c\n",
>> +				    AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
>> +				    q->saved_isc);
>> +	}
>> +}

Same here

>> +
>> +static bool verify_free_aqic_resourcers(struct vfio_ap_queue *q)
>> +{
>> +	bool verified = true;
>> +
>> +	if (q->saved_isc != VFIO_AP_ISC_INVALID &&
>> +	    !(q->matrix_mdev && q->matrix_mdev->kvm)) {
>> +		report_gisc_unregister_failure(q);
>> +		verified = false;
>> +	}
>> +
>> +	if (q->saved_iova && !q->matrix_mdev) {
>> +		pr_warn_ratelimited("APQN %02x.%04x: Failed to unpin NIB page %08x\n",
>> +				    AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
>> +				    q->saved_iova);
> [Severity: Low]
> Does this truncate the logged DMA address? q->saved_iova is a 64-bit
> dma_addr_t, but it's being printed with a 32-bit format specifier %08x.

Will change to %pad

>
>> +		verified = false;
>> +	}
>> +
>> +	return verified;
>> +}
>> +
>>   /**
>>    * vfio_ap_free_aqic_resources - free vfio_ap_queue resources
>>    * @q: The vfio_ap_queue
>> @@ -316,17 +398,13 @@ static int vfio_ap_wait_for_irqclear(int apqn, struct ap_queue_status *tapq_stat
>>    */
>>   static void vfio_ap_free_aqic_resources(struct vfio_ap_queue *q)
>>   {
>> -	if (!q)
>> +	if (!q || !verify_free_aqic_resourcers(q))
>>   		return;
>> -	if (q->saved_isc != VFIO_AP_ISC_INVALID &&
>> -	    !WARN_ON(!(q->matrix_mdev && q->matrix_mdev->kvm))) {
>> -		kvm_s390_gisc_unregister(q->matrix_mdev->kvm, q->saved_isc);
>> -		q->saved_isc = VFIO_AP_ISC_INVALID;
>> -	}
>> -	if (q->saved_iova && !WARN_ON(!q->matrix_mdev)) {
>> -		vfio_unpin_pages(&q->matrix_mdev->vdev, q->saved_iova, 1);
>> -		q->saved_iova = 0;
>> -	}
>> +
>> +	kvm_s390_gisc_unregister(q->matrix_mdev->kvm, q->saved_isc);
>> +	q->saved_isc = VFIO_AP_ISC_INVALID;
>> +	vfio_unpin_pages(&q->matrix_mdev->vdev, q->saved_iova, 1);
>> +	q->saved_iova = 0;
>>   }
> [Severity: High]
> Can this lead to a NULL pointer dereference or a memory leak?
>
> In verify_free_aqic_resourcers(), the logic couples the validation of the two
> independent resources. If both resources are empty (q->saved_isc ==
> VFIO_AP_ISC_INVALID and q->saved_iova == 0), the function returns true. If
> q->matrix_mdev is NULL, vfio_ap_free_aqic_resources() will then unconditionally
> dereference q->matrix_mdev->kvm, resulting in a kernel panic. This path can be
> reached when an unbound queue is removed from the host AP configuration.
>
> Additionally, if only one resource fails validation (e.g., kvm is NULL),
> verify_free_aqic_resourcers() returns false, which skips the cleanup entirely.
> This prevents the NIB page from being unpinned, leaking the memory.

Getting rid of verify_free_aqic_resourcers() and doing the checking in 
vfio_ap_free_aqic_resources() with a fix that eliminates the NULL 
dereference.
>