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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] crypto: zeroization bypass and DMA vulnerability if ZAPQ returns BUSY
- [Critical] crypto: Use-After-Free of vdev pointer after reset timeout
- [High] crypto: resource leak and erroneous WARN when ZAPQ returns BUSY
- [High] crypto: zeroization bypass if non-reset state change completes at timeout
- [Medium] crypto: spurious ZAPQs from conflating TAPQ and ZAPQ BUSY responses
- [Low] crypto: dead code setting fallback reset status on timeout
--

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

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

This commit fixes an unbounded wait loop in the apq_reset_check() worker by
adding a 2000ms timeout. It also updates TAPQ status checks to accurately
verify asynchronous queue zeroization completion before freeing AQIC
resources.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 087e8474a34ab..07fbfa6f10152 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2164,6 +2164,12 @@ static int apq_status_check(int apqn, struct ap_queue_status *status)
>  		return -EBUSY;
>  
>  	case AP_RESPONSE_BUSY:
> +		/*
> +		 * The queue is busy with something unrelated to a reset and our
> +		 * ZAPQ was rejected outright. Re-issue the ZAPQ.
> +		 */
> +		return -EAGAIN;
> +

[Severity: Medium]
Does returning `-EAGAIN` here trigger spurious ZAPQs by conflating TAPQ and
ZAPQ BUSY responses?

`apq_status_check()` evaluates the output of `ap_tapq()`. If `ap_tapq()`
returns `AP_RESPONSE_BUSY`, it means the read instruction itself was
transiently rejected. However, this code appears to incorrectly assume it
means the previous ZAPQ was rejected outright, returning `-EAGAIN`. 

This forces the worker loop to immediately issue a new `ap_zapq()`, which
overwrites `q->reset_status` and destroys the correct queue state tracking,
likely leading to further failures.

[ ... ]

> @@ -2223,30 +2280,73 @@ static void apq_reset_check(struct work_struct *reset_work)
>  			 */
>  			memcpy(&q->reset_status, &status, sizeof(status));
>  			return;
> -		}
> -		if (ret == -EBUSY) {
> +		} else if (elapsed >= AP_RESET_MAX_WAIT) {
> +			/*Timed out without being able to verify zapq completed */
> +			if (!ret || ret == -ENODEV) {

[Severity: High]
Does evaluating the timeout condition before the retry logic allow an
unrelated state change completion to bypass the required zeroization step?

If the initial ZAPQ returns `AP_RESPONSE_STATE_CHANGE_IN_PROGRESS`, the worker
polls until it completes before retrying ZAPQ. If the state change completes
exactly at `elapsed >= AP_RESET_MAX_WAIT`, `ap_tapq()` indicates the queue is
idle, and `ret = 0`.

Because the loop evaluates the timeout condition `else if (elapsed >=
AP_RESET_MAX_WAIT)` before the ZAPQ retry logic, the timeout block executes
`apq_reset_finalize()`, forces the status to `AP_RESPONSE_NORMAL`, and returns.

The required `ap_zapq()` is entirely bypassed, leaving the queue unzeroized
while freeing AQIC resources, resulting in a DMA vulnerability.

> +				/*
> +				 * Zeroization confirmed (ret == 0): the TAPQ status bits
> +				 * indicate the async portion of the ZAPQ completed
> +				 * successfully. Free AQIC resources and return.
> +				 *
> +				 * Queue non-operational (ret == -ENODEV): the queue is
> +				 * deconfigured or checkstopped; interrupts are not
> +				 * possible so AQIC resources can be safely freed.
> +				 * Zeroization cannot be confirmed in this state, but the
> +				 * queue cannot generate interrupts, so the NIB page is
> +				 * no longer a DMA target and it is safe to free it.
> +				 */
> +				apq_reset_finalize(q, &status, ret);
> +				return;
> +			}
> +
> +			report_apq_reset_check_timeout(q);
> +
> +			/*
> +			 * Zeroization could not be confirmed; set
> +			 * reset_status to AP_RESPONSE_RESET_IN_PROGRESS.
> +			 * This is used internally to signal that the reset
> +			 * did not complete, and ensures that if the queue
> +			 * is reset again, the re-issue logic in
> +			 * apq_reset_check() will re-issue the ZAPQ.
> +			 */
> +			q->reset_status.response_code = AP_RESPONSE_RESET_IN_PROGRESS;

[Severity: Low]
Is this assignment to `q->reset_status.response_code` dead code?

The timeout logic assigns `AP_RESPONSE_RESET_IN_PROGRESS` to theoretically
trigger a re-issue during the next reset. However, the next reset requires
calling `vfio_ap_mdev_reset_queue()`, which immediately executes
`status = ap_zapq(q->apqn, 0)` and unconditionally overwrites
`q->reset_status` via `memcpy`.

> +
> +			return;

[Severity: Critical]
Does returning early here skip cleanup and leave `q->aqic_resources.vdev`
as a dangling pointer?

`q->aqic_resources.vdev` points to the `vdev` embedded in `ap_matrix_mdev`
without holding a reference count. When the new timeout mechanism returns
early without calling `apq_reset_finalize()`, it leaves
`q->aqic_resources.vdev` populated.

When the mediated device is removed, `vfio_ap_mdev_remove()` flushes the
worker and calls `vfio_put_device()`, which frees the `ap_matrix_mdev`.
This turns `q->aqic_resources.vdev` into a dangling pointer on a surviving
`vfio_ap_queue`.

Later, if the AP queue is unbound or reused, `vfio_ap_free_aqic_resources()`
unconditionally dereferences this pointer via
`vfio_unpin_pages(q->aqic_resources.vdev, ...)`, which could trigger a
use-after-free on the host.

> +		} else if (ret == -EBUSY) {
>  			pr_notice_ratelimited(WAIT_MSG, elapsed,
>  					      AP_QID_CARD(q->apqn),
>  					      AP_QID_QUEUE(q->apqn),
>  					      status.response_code,
>  					      status.queue_empty,
>  					      status.irq_enabled);
> +			continue;
>  		} 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) {
> +			if (ret == -EAGAIN ||
> +			    q->reset_status.response_code == AP_RESPONSE_RESET_IN_PROGRESS ||
> +			    q->reset_status.response_code == AP_RESPONSE_STATE_CHANGE_IN_PROGRESS) {

[Severity: Critical]
Is it possible for `AP_RESPONSE_BUSY` to be missed in this retry check?

If `ap_zapq()` inside the worker loop is rejected with `AP_RESPONSE_BUSY`,
`q->reset_status.response_code` is set to `AP_RESPONSE_BUSY`.
On the next iteration, if the queue becomes idle, `ap_tapq()` returns
`AP_RESPONSE_NORMAL` (ret = 0).

Since `AP_RESPONSE_BUSY` is missing from the retry condition, the worker seems
to skip re-issuing the ZAPQ. It would fall through to `apq_reset_finalize()`,
falsely overriding the status to `AP_RESPONSE_NORMAL` and freeing the AQIC
resources. Can this leave secret keys intact and expose the host to wild
DMA writes?

> +				status = ap_zapq(q->apqn, 0);
> +				memcpy(&q->reset_status, &status, sizeof(status));
>  				continue;
>  			}


I also have a question about `vfio_ap_mdev_reset_queue()`:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_reset_queue() {
    ...
	switch (status.response_code) {
	case AP_RESPONSE_NORMAL:
	case AP_RESPONSE_RESET_IN_PROGRESS:
	case AP_RESPONSE_STATE_CHANGE_IN_PROGRESS:
    ...
}

[Severity: High]
Does lacking `case AP_RESPONSE_BUSY:` here cause legitimately busy queues
to fail in the default case?

If the queue is transiently busy, ZAPQ returns `AP_RESPONSE_BUSY`. Execution
falls to the `default:` branch, triggering a host WARN (falsely claiming a
hardware bug) and intentionally leaking AQIC resources without queueing the
worker to retry. Could this allow a guest that keeps the queue busy to cause
resource leaks and kernel warnings on the host when the device is removed or
reset?

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