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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Dangling AQIC resources after a reset timeout cause arbitrary IOVA unpinning in a subsequent guest's VFIO container.

Pre-existing issues:
- [High] Early return on an unknown hardware response code (-EIO) silently leaks AQIC resources and fails to update the queue's reset status.
--

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

This commit fixes an unbounded loop in the queue reset worker by introducing a
maximum wait time of 2000ms. It also adds logic to verify the asynchronous
completion of queue zeroization and intentionally leaks AQIC resources on
timeout to avoid a potential host crash from wild DMA writes.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index ea0625f10c7e8..32b80d91a6431 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c

[ ... ]

> @@ -2050,6 +2113,54 @@ static void apq_reset_check(struct work_struct *reset_work)
>  		ret = apq_status_check(q->apqn, &status);
>  		if (ret == -EIO)
>  			return;

[Severity: High]
This is a pre-existing issue, but does this early return on an unknown
hardware response code (-EIO) silently leak AQIC resources?

By returning immediately, this path bypasses the resource cleanup in
vfio_ap_free_aqic_resources(). This leaves q->saved_iova dangling and fails
to update q->reset_status.response_code. 

Since the status remains 0 (AP_RESPONSE_NORMAL), callers might incorrectly 
believe the queue is securely zeroed. If the queue is then reassigned to a 
new guest, could the next successful reset unpin the old IOVA from the new 
guest's IOMMU domain and corrupt memory?

> +		if (elapsed >= AP_RESET_MAX_WAIT) {
> +			/*
> +			 * 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.
> +			 */
> +			if (!ret || ret == -ENODEV)
> +				goto done;
> +			/*
> +			 * Timed out without being able to verify zapq completed.
> +			 *
> +			 * 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 wild DMA write that could
> +			 * corrupt host memory.
> +			 *
> +			 * If the reset eventually completes, interrupts will be
> +			 * terminated and the pinned NIB page and ISC registration
> +			 * will be leaked. This is preferable to either a wild DMA
> +			 * write or waiting indefinitely: flush_work() callers hold
> +			 * the matrix_dev->mdevs_lock mutex which serializes access
> +			 * to all mdev objects system-wide, so blocking here would
> +			 * hang all guests to which those mdevs are attached.
> +			 */
> +			report_aqic_resource_leak(q);
> +			/*
> +			 * Report the actual non-zero hardware response code, or
> +			 * synthesize AP_RESPONSE_RESET_IN_PROGRESS if TAPQ
> +			 * completed normally but the status bits failed to
> +			 * transition to their post-reset states.
> +			 */
> +			if (status.response_code == AP_RESPONSE_NORMAL)
> +				q->reset_status.response_code = AP_RESPONSE_RESET_IN_PROGRESS;
> +			else
> +				q->reset_status.response_code = status.response_code;
> +
> +			return;
> +		}

[Severity: High]
Does this early return on timeout leave dangling AQIC resources that could
cause arbitrary IOVA unpinning in a subsequent guest's VFIO container?

When the worker exits on timeout without clearing the dangling q->saved_iova
field, and the adapter is unassigned from the current guest and hot-plugged
into a new guest, q->matrix_mdev is updated to the new guest.

When the new guest successfully resets the queue, vfio_ap_free_aqic_resources()
will be called. Because q->matrix_mdev is now the new guest's mdev and
q->saved_iova is still the old guest's IOVA, it seems this would erroneously 
unpin an arbitrary IOVA from the new guest's VFIO container:

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;
    }
}

[ ... ]

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