Re: [PATCH v9 1/6] s390/vfio-ap: Fix leaks of pinned NIB and registered GISC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] vfio_ap: Use-After-Free of vfio_device on AQIC resource release
- [High] vfio_ap: ABA pointer aliasing causes unintended KVM GISC unregistration
- [High] vfio_ap: Host panic due to VFIO BUG_ON when queue reset fails during unmap
- [Medium] vfio_ap: KVM GISC alert reference leak on dynamic queue unplug
--

commit 83f84bb0e3c067b95ee373106f589b768094e294
Author: Anthony Krowiak <akrowiak@linux.ibm.com>
Subject: s390/vfio-ap: Fix leaks of pinned NIB and registered GISC

Several code paths in the vfio_ap driver failed to free the AQIC resources,
specifically the pinned guest NIB page and the registered guest ISC used to
enable interrupts for a queue. This commit adds fixes to handle response
codes and prevent memory exhaustion and depletion of KVM interrupt subclass
registrations during dynamic AP reconfiguration.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 4db878c18f41c..e178b657faa8f 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
[ ... ]
> @@ -254,90 +276,231 @@ static void vfio_ap_wait_for_irqclear(int apqn)
[ ... ]
>  static void vfio_ap_free_aqic_resources(struct vfio_ap_queue *q)
>  {
>  	if (!q)
>  		return;
> -	if (q->saved_isc != VFIO_AP_ISC_INVALID) {
> -		if (!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->aqic_resources.isc != VFIO_AP_ISC_INVALID) {
> +		if (q->aqic_resources.kvm &&
> +		    q->matrix_mdev && q->aqic_resources.kvm == q->matrix_mdev->kvm) {
> +			kvm_s390_gisc_unregister(q->aqic_resources.kvm, q->aqic_resources.isc);

[Severity: High]
Can this equality check fall victim to an ABA problem? If the original guest's
struct kvm is freed and a new guest's struct kvm is allocated at the exact
same memory address, could this evaluate to true and mistakenly unregister
the new guest's GISC, silently stopping its AP interrupts?

[Severity: Medium]
If a queue is hot-unplugged from a running guest via sysfs and the reset
fails, the AQIC resources are intentionally leaked and q->matrix_mdev is set
to NULL. If the queue recovers later, will bypassing
kvm_s390_gisc_unregister() permanently leak the KVM GISA alert reference for
the still-running guest?

> +		} else {
> +			report_gisc_unregister_failure(q);
> +		}
> +		q->aqic_resources.isc = VFIO_AP_ISC_INVALID;
> +		q->aqic_resources.kvm = NULL;
>  	}
> -	if (q->saved_iova && !WARN_ON(!q->matrix_mdev)) {
> -		vfio_unpin_pages(&q->matrix_mdev->vdev, q->saved_iova, 1);
> -		q->saved_iova = 0;
> +
> +	if (q->aqic_resources.iova) {
> +		if (q->aqic_resources.vdev)
> +			vfio_unpin_pages(q->aqic_resources.vdev, q->aqic_resources.iova, 1);

[Severity: Critical]
Is it possible for the q->aqic_resources.vdev pointer to become dangling here
if the original matrix_mdev was destroyed? If a queue reset failed previously
and the resources were kept leaked, the queue would retain the unreferenced
vdev pointer while the matrix_mdev is freed. Could a subsequent reset or
reassignment trigger a use-after-free when dereferencing it for
vfio_unpin_pages()?

> +		else
> +			pr_warn_ratelimited("APQN %02x.%04x: Failed to unpin NIB page at %pad\n",
> +					    AP_QID_CARD(q->apqn),
> +					    AP_QID_QUEUE(q->apqn),
> +					    &q->aqic_resources.iova);
> +		q->aqic_resources.iova = 0;
> +		q->aqic_resources.vdev = NULL;
>  	}
>  }
[ ... ]
> @@ -489,27 +658,48 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q,
>  	status = ap_aqic(q->apqn, aqic_gisa, h_nib);
>  	switch (status.response_code) {
>  	case AP_RESPONSE_NORMAL:
[ ... ]
>  		vfio_ap_free_aqic_resources(q);
> -		q->saved_iova = nib;
> -		q->saved_isc = isc;
> +		q->aqic_resources.iova = nib;
> +		q->aqic_resources.isc  = isc;
> +		q->aqic_resources.vdev = &q->matrix_mdev->vdev;

[Severity: Critical]
Does this code need to acquire a lifecycle reference to the vdev pointer
before storing it here? Snapshooting the embedded pointer from matrix_mdev
without taking a reference appears to expose the use-after-free discussed
above if the mediated device goes away.

> +		q->aqic_resources.kvm  = q->matrix_mdev->kvm;
>  		break;
[ ... ]
> @@ -1857,8 +2046,29 @@ static void unmap_iova(struct ap_matrix_mdev *matrix_mdev, u64 iova, u64 length)
>  	int loop_cursor;
>  
>  	hash_for_each(qtable->queues, loop_cursor, q, mdev_qnode) {
> -		if (q->saved_iova >= iova && q->saved_iova < iova + length)
> +		if (q->aqic_resources.iova >= iova && q->aqic_resources.iova < iova + length) {
>  			vfio_ap_irq_disable(q);
> +			/*
> +			 * If AQIC disable was unable to confirm that interrupts were
> +			 * disabled (IR=0), q->aqic_res.iova remains non-zero. VFIO
> +			 * core requires the mapped pages to be unpinned during DMA
> +			 * unmap notifications (or type1 IOMMU will BUG_ON after 10
> +			 * retries).
> +			 *
> +			 * To satisfy VFIO core safely, fall back to a queue reset via
> +			 * ZAPQ. ZAPQ wipes the queue state and clears the hardware's
> +			 * internal NIB address register, neutralizing pending DMA.
> +			 *
> +			 * Flush the reset worker. If the reset completes, the worker
> +			 * frees the AQIC resources (unpinning the NIB). If the reset
> +			 * times out or fails with an error, the resources are kept
> +			 * leaked to guard against wild DMA writes.
> +			 */
> +			if (q->aqic_resources.iova) {
> +				vfio_ap_mdev_reset_queue(q);
> +				flush_work(&q->reset_work);
> +			}

[Severity: High]
If the queue reset times out or fails with an error, the NIB page is kept
leaked and remains pinned. Since the VFIO core requires all mapped pages to
be unpinned during an unmap notifier, could returning to the VFIO core from
unmap_iova() without unpinning the page trigger a BUG_ON or WARN_ON and panic
the host kernel?

> +		}
>  	}
>  }

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