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

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

On 9/29/26 8:33 AM, sashiko-bot@kernel.org wrote:
> 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?

Not valid. `vfio_ap_mdev_set_kvm()` takes a `kvm_get_kvm()`
reference before storing the pointer, so the struct cannot be
freed while the mdev holds it. `vfio_ap_mdev_unset_kvm()` sets
`matrix_mdev->kvm = NULL` before dropping the ref, so by the
time the struct could be freed the comparison cannot be true.

>
> [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?

Not valid. `kvm_s390_gisa_destroy()` zeroes `gi->alert.mask`
and sets `gi->origin = NULL` on guest teardown. After that,
`kvm_s390_gisc_unregister()` returns -ENODEV harmlessly. The
KVM teardown path cleans up the GISC registration regardless
of whether vfio-ap explicitly calls unregister.

>
>> +		} 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()?

Not valid in practice. `vfio_ap_mdev_remove()` flushes all
reset work before calling `vfio_put_device()`, so
`vfio_ap_free_aqic_resources()` — the only site that
dereferences `vdev` — always completes before `matrix_mdev`
is freed. No reachable path calls it on a detached queue.

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

Not valid in practice, same reason as Comment 3. Taking a
vfio ref would also be wrong: `vdev` is embedded in
`matrix_mdev`, so holding a ref while resources are
intentionally leaked would prevent the mdev from ever being
freed — introducing a new leak.

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

Valid. If both `vfio_ap_irq_disable()` and the ZAPQ fallback
fail, `unmap_iova()` returns with `aqic_resources.iova`
non-zero. `vfio_iommu_type1.c:1522` hits
`BUG_ON(++retries > 10)` — a real host panic. Fix: add a
last-resort forced unpin with a `pr_crit` after `flush_work()`
when the ZAPQ also fails to release the NIB.

>
>> +		}
>>   	}
>>   }