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

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

On 8/24/26 6:11 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
>
> New issues:
> - [Critical] The timeout mechanism intentionally skips unpinning the NIB page to avoid a driver-level UAF, but fails to realize the VFIO core will automatically unpin and free the page when the device is closed, resulting in a critical hardware DMA Use-After-Free.
> - [Critical] The timeout handler leaves `q->saved_iova` and `q->saved_isc` populated with the old guest's state, leading to cross-guest resource corruption when the queue is assigned to a new mdev.
> - [High] The timeout error path deliberately leaks the registered guest ISC and pinned NIB page, which permanently exhausts ISCs and directly causes a host kernel WARN when the KVM guest terminates.
> - [High] The worker can still loop indefinitely because the new timeout check is bypassed in the 'else' branch when retrying the reset via ap_zapq().
> - [Medium] The commit message explicitly claims that AQIC resources are freed on the early exit path, but the code does the exact opposite.
>
> Pre-existing issues:
> - [High] The worker returns early on `-EIO` without writing the final hardware status back to `q->reset_status`.
> --
>
> --- Patch [3]: [PATCH v2 3/4] s390/vfio-ap: Fix unbounded loop in apq_reset_check() ---
> commit 84304c1f304fd7bd597c0aa370c7ee8ddc904d98
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
>
> s390/vfio-ap: Fix unbounded loop in apq_reset_check()
>
> This commit introduces a maximum wait timeout in the apq_reset_check worker
> to prevent indefinite loops when the hardware continuously returns a busy
> response. On timeout, the function writes the busy status back and breaks out
> of the loop.
>
>> Fix this by introducing AP_RESET_MAX_WAIT (2000ms) and breaking out
>> of the poll loop when elapsed time reaches that threshold. On
>> timeout the final busy status is written back to q->reset_status
>> so that callers inspecting reset_status.response_code after
>> flush_work() see a non-zero value and can return an appropriate
>> error. vfio_ap_free_aqic_resources() is called before returning
>> to release any KVM ISC registration and pinned NIB page,
>> consistent with all other early-exit paths in the function.
> [Severity: Medium]
> Is this description accurate? The code in the patch explicitly omits calling
> vfio_ap_free_aqic_resources() on the timeout path and includes a large
> comment explaining why it must leak these resources.

The description needs to change to match the code change.

>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 3f99b239fa959..7a9b5448e90a0 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> [ ... ]
>> @@ -1971,6 +1972,27 @@ static void apq_reset_check(struct work_struct *reset_work)
>>   					      status.response_code,
>>   					      status.queue_empty,
>>   					      status.irq_enabled);
> [Severity: High]
> This is a pre-existing issue, but does the early return on -EIO just above
> this block need to update q->reset_status before returning?

The response status returned from the tapq needs to be co

>
> If ap_zapq() initially succeeded, q->reset_status.response_code would be 0.
> If a subsequent ap_tapq() results in an -EIO from apq_status_check(), the
> function returns immediately. Will the caller vfio_ap_mdev_reset_queues()
> incorrectly assume the reset was successful because it sees a 0 response code?
>
>> +			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.
> [Severity: Critical]
> The comment mentions leaking the NIB page to avoid a use-after-free, but does
> the driver hold an independent reference to the page?
>
> When the user process terminates and the VFIO container is closed, the VFIO
> core automatically unpins tracked pages. Will the physical page be returned to
> the buddy allocator while the active hardware can still perform a DMA write to
> it, potentially corrupting memory reallocated to other processes?

Pages pinned via vfio_pin_pages() are reference-counted by the VFIO layer
and are not automatically unpinned unless the driver calls
vfio_unpin_pages(). The page is genuinely leaked (intentionally), not
silently freed.

The driver does hold an independent reference to the page, so it should
freed when the kvm is torn down, the mdev is removed, a queue is
removed from the device driver or a queue is unassigned from the mdev.
This will be fixed in a forthcoming patch.

>
>> +				 *
>> +				 * If the reset eventually completes, interrupts will be terminated
>> +				 * and the pinned NIB page and ISC registration will be leaked. This
> [Severity: High]
> By intentionally skipping vfio_ap_free_aqic_resources(),
> kvm_s390_gisc_unregister() is never called. Does this mean KVM ISCs are
> permanently depleted for the guest?
>
> Also, when the KVM guest is torn down, will kvm_s390_gisa_destroy() trigger a
> host kernel WARN due to the non-zero alert mask?

This is a legitimate concern. Similar to my response above, this will be
fixed in a forthcoming patch.

>
>> +				 * is preferable to either a use-after-free or waiting indefinitely:
>> +				 * apq_reset_check() 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: Critical]
> If the timeout handler leaves q->saved_iova and q->saved_isc populated, what
> happens when this queue is assigned to a new mdev?
>
> When the new guest enables interrupts, vfio_ap_free_aqic_resources() will
> process the old guest's saved IOVA and ISC using the new guest's KVM and
> VFIO container. Could this unpin an active page in the new guest or corrupt
> its GISA state?

This is a legitimate concern that will be fixed by clearing these resources
before any queue can get assigned to another mdev (see above responses).


>
>>   		} else {
>>   			if (q->reset_status.response_code == AP_RESPONSE_RESET_IN_PROGRESS ||
>>   			    q->reset_status.response_code == AP_RESPONSE_BUSY ||
>>   			    q->reset_status.response_code == AP_RESPONSE_STATE_CHANGE_IN_PROGRESS ||
>>   			    ret == -EAGAIN) {
>>   				status = ap_zapq(q->apqn, 0);
>>   				memcpy(&q->reset_status, &status, sizeof(status));
>>   				continue;
>>   			}
> [Severity: High]
> Does this bypass the timeout check entirely if apq_status_check() returns
> -EAGAIN?
>
> The loop will drop into this else block, re-issue ap_zapq(), update the
> status, and continue without evaluating the elapsed time. Will the worker
> spin indefinitely if the hardware continuously returns a state that triggers
> -EAGAIN?

Yes. This will be fixed in v3.

>
>>   			if (q->saved_isc != VFIO_AP_ISC_INVALID)
>>   				vfio_ap_free_aqic_resources(q);
>>   			break;
>>   		}