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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Critical] Unpinning the guest NIB page via `vfio_ap_free_aqic_resources()` when `apq_reset_check()` times out causes a critical DMA use-after-free vulnerability.

Pre-existing issues:
- [Critical] Unpinning the guest NIB page after `ap_aqic` timeout in `vfio_ap_irq_disable()` causes a DMA use-after-free.
- [High] The introduced timeout mechanism fails to break out of the infinite loop when the hardware returns an asynchronous error (yielding `-EAGAIN`).
--

--- Patch [3]: [PATCH 3/4] s390/vfio-ap: Fix unbounded loop in apq_reset_check() ---
commit afa7b19f4ae6cc32897da341d31f38a06cb912ec
Author: Anthony Krowiak <[email protected]>

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

This commit introduces a timeout mechanism (AP_RESET_MAX_WAIT) in
apq_reset_check() to prevent the worker from spinning indefinitely when
waiting for an AP queue reset. If the reset takes longer than 2000ms, the
worker writes back the status, frees AQIC resources, and exits.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 6e4569d6b9753..c7eebbd0ed40e 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
[ ... ]
> @@ -1973,6 +1974,12 @@ static void apq_reset_check(struct work_struct *reset_work)
>  					      status.response_code,
>  					      status.queue_empty,
>  					      status.irq_enabled);
> +			if (elapsed >= AP_RESET_MAX_WAIT) {
> +				/* Timed out waiting for reset to complete */
> +				memcpy(&q->reset_status, &status, sizeof(status));
> +				vfio_ap_free_aqic_resources(q);

[Severity: Critical]
If the hardware gets stuck and we time out here, does unpinning the NIB page
cause a DMA use-after-free regression?

If the hardware is stuck in a busy or reset-in-progress state, it might still
hold the physical address of the NIB.

vfio_ap_free_aqic_resources() unpins the active DMA page:

vfio_ap_free_aqic_resources() {
    ...
    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 the hardware later completes an operation or sends an interrupt, could it
execute a DMA write to the now-freed physical page, potentially corrupting
arbitrary memory on the host?

To safely handle permanently busy hardware, does the NIB page need to be
intentionally leaked or quarantined rather than unpinned?

> +				return;
> +			}
>  		} 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]
This isn't a bug introduced by this patch, but does this logic still allow an
unbounded loop regression?

Because the elapsed timeout check is entirely inside the if (ret == -EBUSY)
block, if apq_status_check() repeatedly returns -EAGAIN, we take this else
branch and continue without ever checking the elapsed time.

Could this still result in the worker thread hanging indefinitely and
permanently blocking global locks?

>  			}

[Severity: Critical]
This is a pre-existing issue, but a similar DMA use-after-free regression
seems to exist in vfio_ap_irq_disable():

vfio_ap_irq_disable() {
    ...
end_free:
    vfio_ap_free_aqic_resources(q);
    return status;
}

If the hardware fails to process the ap_aqic disable command after its retries,
this code unconditionally calls vfio_ap_free_aqic_resources(), unpinning the
NIB page.

If the hardware is still actively using it and an interrupt fires later,
couldn't this also cause a DMA write to a freed page?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3