Re: [PATCH v3 3/4] s390/vfio-ap: Fix unbounded loop in apq_reset_check()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The timeout condition is evaluated before checking for a successful reset, causing the driver to falsely report success while skipping required resource cleanup if success occurs exactly at the timeout boundary.
- [High] The timeout path fails to clear `q->saved_isc`, leading to Cross-VM KVM interrupt routing corruption when the queue is reassigned to a new guest.
--

commit bdd9b4236348c839f4b1d25d5fe39d6e047b2068
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390/vfio-ap: Fix unbounded loop in apq_reset_check()

This commit introduces a 2000ms maximum wait time for AP queue resets in
apq_reset_check() to prevent indefinite spinning. If the reset times out,
the function breaks out of the loop and leaves the AQIC resources pinned to
avoid use-after-free issues.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 4c32fd6eaaa63..a9a33f4949a0e 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
[ ... ]
> @@ -2016,8 +2017,32 @@ static void apq_reset_check(struct work_struct *reset_work)
>  		elapsed += AP_RESET_INTERVAL;
>  		status = ap_tapq(q->apqn, NULL);
>  		ret = apq_status_check(q->apqn, &status);
> -		if (ret == -EIO)
> +		if (ret == -EIO) {
> +			memcpy(&q->reset_status, &status, sizeof(status));
>  			return;
> +		}
> +		if (elapsed >= AP_RESET_MAX_WAIT) {
> +		/*
> +		 * Timed out waiting for reset to complete.
> +		 *
> +		 * The AQIC resources associated with this queue - the pinned page
> +		 * containing the NIB and the registered guest ISC - cannot be freed
> +		 * here. The NIB is the active DMA target for AP interrupt delivery
> +		 * until the reset completes; freeing the pinned page while the
> +		 * hardware may still write to it would result in a use-after-free
> +		 * kernel crash.
> +		 *
> +		 * If the reset eventually completes, interrupts will be terminated
> +		 * and the pinned NIB page and ISC registration will be leaked. This
> +		 * is preferable to either a use-after-free or waiting indefinitely:
> +		 * the caller of apq_reset_check() holds mdevs_lock while flush_work()
> +		 * blocks holds the matrix_dev->mdevs_lock mutex, which
> +		 * serializes access to all mdev objects system-wide, so blocking
> +		 * here would stall all other guests using AP queues.
> +		 */
> +			memcpy(&q->reset_status, &status, sizeof(status));
> +			return;

[Severity: High]
Does this code falsely report success while skipping required resource cleanup
if the reset succeeds exactly at the timeout boundary?

Because this timeout condition is evaluated before checking if the response is
successful, if ap_tapq() returns AP_RESPONSE_NORMAL on the exact iteration
where elapsed reaches AP_RESET_MAX_WAIT (2000ms), it is treated as a timeout.

The loop exits without calling vfio_ap_free_aqic_resources(), but copies the
success status into q->reset_status, meaning the caller thinks the reset
fully succeeded.

[Severity: High]
Does this timeout path fail to clear q->saved_isc, leading to cross-vm kvm
interrupt routing corruption when the queue is reassigned to a new guest?

When apq_reset_check() times out, it skips vfio_ap_free_aqic_resources() to
leak the memory, but leaves q->saved_isc populated since the vfio_ap_queue
struct persists across mdev assignments.

When a new guest later enables AP interrupts, vfio_ap_free_aqic_resources()
uses the stale q->saved_isc to call:

    kvm_s390_gisc_unregister(new_kvm, q->saved_isc);

This unregisters the stale ISC against the new guest's KVM object,
decrementing the new guest's ISC reference count and wrongly disabling alerts
for that ISC.

> +		}
>  		if (ret == -EBUSY) {

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