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

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

On 9/4/26 5:48 AM, sashiko-bot@kernel.org wrote:
> 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?

This will be fixed in patch 1 of the next (v6) of the patch series.

>
> 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:

The scenario described cannot occur. When the adapter is
unassigned from the mdev, unassign_adapter_store() calls
vfio_ap_mdev_hot_unplug_adapter() which calls
vfio_ap_mdev_hot_unplug_adapters(). That function calls
vfio_ap_mdev_unlink_adapter(), which removes the queue from
the mdev's hash table, but q->matrix_mdev is not set to NULL
until vfio_ap_unlink_mdev_fr_queue() is called later. Before
that, vfio_ap_mdev_reset_qlist() is called, which issues a
fresh ZAPQ for the queue while q->matrix_mdev is still valid.
If that ZAPQ completes successfully, vfio_ap_free_aqic_resources()
will correctly unpin the old NIB page and unregister the ISC
using the still-valid q->matrix_mdev. So while the reviewer
is correct that the AQIC resources are leaked by the timeout
path, they will be cleaned up by the ZAPQ issued during the
unassign, provided the reset succeeds at that point.

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