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

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

On 9/25/26 9:00 AM, sashiko-bot@kernel.org wrote:
> 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.

Asked and answered in response to same comment in patch 1/6.

>
>> +			}
>> +		}
>>   	}
>>   }
> [ ... ]
>> @@ -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.

This is a legitimate concern and will be fixed in v9 of the series 
forthcoming.

>
>> +		}
>> +
>> +		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.

This is a legitimate concern; The guest ISC needs to be unregistered 
using the
kvm pointer used to register it if possible. The pinned NIB page needs to be
freed using the vfio_device whose IOMMU container owns the pinned NIB
page. This will be fixed in v9 of the series forthcoming.

>
>> +		}
>> +
>>   		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.

The premature early exit for !ret needs to be removed from the top of 
the loop.
When the worker starts after an initial ap_zapq() returns
AP_RESPONSE_STATE_CHANGE_IN_PROGRESS or AP_RESPONSE_RESET_IN_PROGRESS,
TAPQ might initially report AP_RESPONSE_NORMAL (returning ret = 0); in 
this case,
the queue will not have been reset/zeroized, so we need to let it fall 
through to
the block above. This will be fixed in the forthcoming v9 of the series.


>
>> +			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().

The the 'elapsed' timeout counter should not be reset to 0.
If the hardware continuously returns busy / EAGAIN responses, the
loop will be guaranteed to exit once elapsed >= AP_RESET_MAX_WAIT (2000ms),
preventing any unbounded spinning or host deadlock.

>
>>   		}
>>   	}
>>   }
> [ ... ]
>> @@ -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.

The AP_RESPONSE_BUSY (05) response code is not a valid response for
ZAPQ according to the architecture; so, it makes sense to let it fall 
through
to the default case since it is architecturally incorrect.  On the other 
hand,
AP_RESPONSE_BUSY is valid for TAPQ which is used by apq_status_check()
to verify whether a reset has successfully completed. If TAPQ returns
AP_RESPONSE_BUSY, the status check returns -EAGAIN so the ZAPQ
is re-issued.

>