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

[sashiko-bot@kernel.org]

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.

> +	} 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);
> +	}
> +}
> +
> +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.

> +		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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925124551.665448-1-akrowiak@linux.ibm.com?part=6