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

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

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

Ironically I was just reviewing this code and drew that same conclusion.
We need to check the return code from the call to apq_reset_check()
and if it 0, bail out of the wait loop.

Regarding copying the status into q->reset_status, that should only be
done prior to entering the loop and on ret == -EAGAIN after the
AQIC instruction is executed.

>
> [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 the return code from apq_status_check() returns 0, the
AQIC resources need to be freed.

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