Re: [PATCH v8 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] s390/vfio-ap: unmap_iova() defeats timeout leak, causing wild DMA write
- [Critical] s390/vfio-ap: vfio_ap_mdev_reset_queue() abandons reset on AP_RESPONSE_BUSY
- [Critical] s390/vfio-ap: apq_reset_check() falsely reports zeroization complete
- [High] s390/vfio-ap: apq_reset_check() infinite loop on AP_RESPONSE_BUSY
- [High] s390/vfio-ap: cross-guest resource corruption due to dangling IOVA/ISC
- [Low] s390/vfio-ap: dead code and ineffective state handling on reset timeout
--

commit 3205ec637b4cfa8d70d28a8f5abc909aba0bd19c
Author: Anthony Krowiak <akrowiak@linux.ibm.com>
Subject: s390/vfio-ap: Fix unbounded loop in apq_reset_check()

The commit introduces a 2000ms timeout to apq_reset_check() to prevent
indefinite spinning when the hardware continuously returns a busy response.
If the timeout is reached, the NIB page and ISC registration are intentionally
leaked to avoid memory corruption from potential wild DMA writes. It also
corrects the confirmation logic by checking multiple status bits returned
from TAPQ.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 940c0ff668bee..47d4936fb9d7c 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
[ ... ]
> @@ -1857,8 +2006,28 @@ static void unmap_iova(struct ap_matrix_mdev *matrix_mdev, u64 iova, u64 length)
>  	int loop_cursor;
>  
>  	hash_for_each(qtable->queues, loop_cursor, q, mdev_qnode) {
> -		if (q->saved_iova >= iova && q->saved_iova < iova + length)
> +		if (q->saved_iova >= iova && q->saved_iova < iova + length) {
>  			vfio_ap_irq_disable(q);
> +			/*
> +			 * If IRQ disable failed or IR=0 could not be confirmed,
> +			 * vfio_ap_irq_disable() intentionally leaks the NIB to
> +			 * prevent a wild DMA write. But vfio core requires the
> +			 * page to be unpinned before dma_unmap returns, or it
> +			 * will BUG_ON after 10 re-notification rounds.
> +			 *
> +			 * Fall back to a bounded queue reset. The ZAPQ zeroizes
> +			 * the NIB pointer in hardware, eliminating the DMA risk
> +			 * that justified the leak. Once the worker finishes (or
> +			 * times out with a reset confirmed in-progress), the
> +			 * hardware no longer holds a reference to saved_iova and
> +			 * it is safe to unpin unconditionally.
> +			 */
> +			if (q->saved_iova) {
> +				vfio_ap_mdev_reset_queue(q);
> +				flush_work(&q->reset_work);
> +				vfio_ap_free_aqic_resources(q);

[Severity: Critical]
Could this sequence unconditionally free the AQIC resources immediately after
the timeout completes, defeating the deliberate leak?

apq_reset_check() intentionally leaks the NIB page on timeout because hardware
might still be actively writing to it. However, unmap_iova() executes
flush_work(&q->reset_work) and then unconditionally checks q->saved_iova.

Because the timeout path didn't clear saved_iova, unmap_iova() immediately
unpins the page via vfio_ap_free_aqic_resources(). Returning an active DMA
target to the page allocator could cause host memory corruption via wild DMA
writes.

> +			}
> +		}
>  	}
>  }
[ ... ]
> @@ -1959,8 +2220,77 @@ 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 || ret == -ENODEV) {
> +			/*
> +			 * 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;

[Severity: Critical]
Does this immediate return prevent the rejected ZAPQ instruction from being
reissued, bypassing queue zeroization entirely while falsely reporting success?

If the initial ap_zapq() is rejected (e.g., returns
AP_RESPONSE_STATE_CHANGE_IN_PROGRESS), apq_reset_check() is queued without the
queue being zeroized. apq_reset_check() polls with ap_tapq().

Once the hardware state change finishes, ap_tapq() returns AP_RESPONSE_NORMAL
(queue empty). apq_reset_check() sees !ret, executes apq_reset_finalize(),
and returns success.

The ZAPQ command is never reissued. The queue is falsely marked as reset and
can be assigned to a new guest with the previous guest's cryptographic secrets
intact.

> +		}
> +
> +		if (elapsed >= AP_RESET_MAX_WAIT) {
> +			/*
> +			 * Timed out without being able to verify zapq completed.
[ ... ]
> +			 */
> +			report_aqic_resource_leak(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;
> +
> +			return;

[Severity: High]
Can leaving q->saved_iova and q->saved_isc populated after the timeout corrupt
cross-guest resources?

If the queue is assigned to a new guest, q->matrix_mdev points to the new
guest's matrix_mdev. When the new guest enables interrupts,
vfio_ap_free_aqic_resources() is called.

It uses the new q->matrix_mdev to unpin the old q->saved_iova and unregister
the old q->saved_isc, corrupting the new guest's IOMMU pinning counts
and KVM ISC refcounts.

> +		}
> +
>  		if (ret == -EBUSY) {
>  			pr_notice_ratelimited(WAIT_MSG, elapsed,
>  					      AP_QID_CARD(q->apqn),
> @@ -1968,18 +2298,15 @@ static void apq_reset_check(struct work_struct *reset_work)
>  					      status.response_code,
>  					      status.queue_empty,
>  					      status.irq_enabled);
> -		} 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;
> -			}
> -			if (q->saved_isc != VFIO_AP_ISC_INVALID)
> -				vfio_ap_free_aqic_resources(q);
> -			break;
> +			continue;
> +		}
> +
> +		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: Low]
Is this condition checking dead code because earlier flow control guarantees
ret == -EAGAIN is always true here?

The code attempts to signal a failed reset by setting
reset_status.response_code = AP_RESPONSE_RESET_IN_PROGRESS on timeout.

However, vfio_ap_mdev_reset_queue() overwrites reset_status unconditionally
with the result of a new ap_zapq() call on the next reset attempt, rendering
the signal useless. Furthermore, the conditional check for this status inside
apq_reset_check() is completely unreachable because execution only reaches that
block if ret == -EAGAIN, which short-circuits the OR condition.

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

[Severity: High]
Will this repeatedly reset the timeout counter when dealing with busy
responses, bypassing the timeout logic?

If the hardware continuously returns AP_RESPONSE_BUSY, apq_status_check()
maps this to -EAGAIN. In apq_reset_check(), if ret == -EAGAIN, the elapsed
timeout counter is reset to 0, completely bypassing the new 2000ms timeout
logic.

This causes the worker to spin indefinitely, deadlocking the host system as
callers hold global mutexes like mdevs_lock while waiting on flush_work().

>  		}
>  	}
>  }
[ ... ]
> @@ -1995,18 +2322,27 @@ static void vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q)
>  	switch (status.response_code) {
>  	case AP_RESPONSE_NORMAL:
>  	case AP_RESPONSE_RESET_IN_PROGRESS:
> -	case AP_RESPONSE_BUSY:
>  	case AP_RESPONSE_STATE_CHANGE_IN_PROGRESS:
>  		/*
>  		 * Let's verify whether the ZAPQ completed successfully on a work queue.
>  		 */
>  		queue_work(system_long_wq, &q->reset_work);
>  		break;

[Severity: Critical]
Would removing AP_RESPONSE_BUSY here cause the flow to fall through to the
default error case?

If the initial ap_zapq() returns AP_RESPONSE_BUSY, it falls to the default
case, aborting the reset and not queueing the reset_work.

This leaves the cryptographic queue unzeroized, leading to an information
leak when the queue is reassigned.

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