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

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

On 8/28/26 6:02 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Critical] The timeout path incorrectly treats an idle queue state as a completed reset, bypassing the necessary zeroize command and causing Use-After-Free.
> - [High] The reset operation silently reports success to callers when the reset times out, masking the failure and misleading userspace.
> - [Medium] The `elapsed` timer is not reset when a new reset command is issued, causing subsequent resets to prematurely time out.
> --
>
> commit 3ef072206a03ac06e2b0604371727e31f5cba1a8
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
>
> s390/vfio-ap: Fix unbounded loop in apq_reset_check()
>
> This commit introduces a 2000ms timeout for queue reset operations in
> apq_reset_check() to prevent indefinite blocking in the worker thread.
> Upon timeout, it skips freeing AQIC resources to avoid a use-after-free,
> leaking them instead.
>
> Fixes: dd174833e44e ("s390/vfio-ap: remove upper limit on wait for queue reset to complete")
> Cc: stable@vger.kernel.org
>
> The commit message states:
>
>>     Note that on timeout, q->reset_status will hold the status from the most
>>     recent reset operation so that callers inspecting
>>     q->reset_status.response_code after flush_work() will see the value
>>     and can return an appropriate return code.
> Is this description accurate?

Yes

>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 363d9e53e2490..3f5b012be450c 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -2067,6 +2068,37 @@ static void apq_reset_check(struct work_struct *reset_work)
>>   		ret = apq_status_check(q->apqn, &status);
>>   		if (ret == -EIO)
>>   			return;
>> +		if (elapsed >= AP_RESET_MAX_WAIT) {
>> +			/*
>> +			 * If the status check determined that the reset completed
>> +			 * successfully or the queue is not operational, clean up
>> +			 * the AQIC resources because queue reset disables
>> +			 * interrupts, or because interrupts are not possible on a
>> +			 * non-operational queue.
>> +			 */
>> +			if (!ret)
>> +				goto done;
> [Severity: Critical]
> Does this incorrectly treat an idle queue as a successfully reset queue?
>
> If the queue is continuously busy for 2000ms, the initial ap_zapq() command
> is rejected (returning AP_RESPONSE_BUSY). On the exact iteration where
> elapsed reaches AP_RESET_MAX_WAIT (2000ms), if the queue finally stops being
> busy, ap_tapq() returns 0.
>
> Could this misinterpret the 0 as a completed reset, jump to done, and free
> the AQIC resources? Since the queue was never actually zeroized, interrupts
> remain enabled. Can this lead to a host kernel use-after-free if the hardware
> writes to the freed NIB page?

After further review of the architecture, it can't be assumed that 
AP_RESPONSE_VALID
indicates the zeroize is complete because the initial response code only 
indicates
that the ZAPQ was successfully initiated, but the zeroizing of the queue 
is being
done asynchronously. To verify it has completed, the following bits in 
the status
returned from TAPQ must be checked:

queue_empty == 1
replies_waiting == 0
irq_enabled == 0
async == 0

This will be fixed in v5

>
>> +			/*
>> +			 * Timed out without being able to verify reset completed.
>> +			 *
>> +			 * 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.
>> +			 */
>> +
>> +			return;
> [Severity: High]
> Does this return early on timeout without updating q->reset_status to indicate
> a failure?
>
> If the queue reset times out and q->reset_status.response_code was 0 from the
> initial accepted command, it remains unmodified.
>
> Would callers like vfio_ap_mdev_reset_queues() check this unmodified response
> code, incorrectly assume the reset succeeded, and mislead userspace into
> believing the VFIO device was successfully reset while the hardware is still
> operational?

See previous response

>
> [ ... ]
>
>> @@ -2083,11 +2115,13 @@ static void apq_reset_check(struct work_struct *reset_work)
>>   				memcpy(&q->reset_status, &status, sizeof(status));
>>   				continue;
> [Severity: Medium]
> Is the elapsed timer supposed to be reset when a new reset command is issued
> here?
>
> If the queue is initially busy, takes time (e.g., 1980ms) to become idle, and
> requires a retry of the ap_zapq() command, the next loop iteration will
> instantly hit the 2000ms limit because elapsed was not cleared.
>
> Could this prematurely time out subsequent legitimate reset operations and
> result in resource leaks?

This makes sense and will be changed for v5.

>
>>   			}
>> -			if (q->saved_isc != VFIO_AP_ISC_INVALID)
>> -				vfio_ap_free_aqic_resources(q);
>> -			break;
>> +			goto done;
>>   		}
>>   	}
>> +
>> +done:
>> +	if (q->saved_isc != VFIO_AP_ISC_INVALID)
>> +		vfio_ap_free_aqic_resources(q);
>>   }