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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The loop in apq_reset_check() may exit successfully on timeout without issuing a ZAPQ reset if the queue becomes idle on the final iteration, leaving cryptographic keys intact.
- [High] The mitigation to prevent a DMA Use-After-Free by intentionally leaking the pinned NIB page fails when the VFIO container is closed, leading to host memory corruption.
- [High] When queue reset times out, the `saved_iova` and `saved_isc` fields in `vfio_ap_queue` are not cleared, leading to incorrect unpinning of a future guest's memory when the queue is reassigned.
--

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

    s390/vfio-ap: Fix unbounded loop in apq_reset_check()
    
    This patch introduces a 2000ms timeout to apq_reset_check() to prevent an
    unbounded loop when hardware continuously returns busy responses. It
    intentionally leaks AQIC resources on timeout to avoid a use-after-free,
    and correctly checks status bits for completion of queue zeroization.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index a4980d993b681..6c315d7a0a088 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
[ ... ]
> @@ -2067,6 +2115,47 @@ static void apq_reset_check(struct work_struct *reset_work)
>  		ret = apq_status_check(q->apqn, &status);
>  		if (ret == -EIO)
>  			return;
> +		if (elapsed >= AP_RESET_MAX_WAIT) {
> +			/*
> +			 * If the status check determined that the reset completed
> +			 * successfully or the queue is not operational, clean up
> +			 * the AQIC resources because queue reset disables
> +			 * interrupts and interrupts are not possible on a
> +			 * non-operational queue.
> +			 */
> +			if (!ret)
> +				goto done;

[Severity: High]
Does this early exit skip issuing a required ZAPQ retry if the queue happens
to become idle on the final iteration?

If the initial ap_zapq() fails with AP_RESPONSE_BUSY and the queue becomes
idle exactly on the 100th iteration (2000ms), apq_status_check() will return 0.
The timeout check intercepts this with goto done, which successfully exits
without ever executing the else block that re-issues the ap_zapq() command.

Can this leave cryptographic keys intact on a supposedly reset queue, leaking
them to the next assigned guest?

> +			/*
> +			 * Timed out without being able to verify reset 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 use-after-free
> +			 * kernel crash.
> +			 *
> +			 * 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 use-after-free or waiting indefinitely:
> +			 * the caller of apq_reset_check() holds mdevs_lock while flush_work()
> +			 * blocks holds the matrix_dev->mdevs_lock mutex, which
> +			 * serializes access to all mdev objects system-wide, so blocking
> +			 * here would stall all other guests using AP queues.
> +			 */
> +			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]
Will intentionally leaking the pinned NIB page actually prevent a
use-after-free on host KVM shutdown?

The timeout logic skips vfio_unpin_pages() and returns here to intentionally
leak the page. However, when the KVM guest is shut down and the VFIO container
is closed, the VFIO IOMMU Type1 driver automatically unpins and frees all
remaining pinned pages.

Since the mdev driver does not take an independent reference (e.g., using
get_page()), the physical page is returned to the host buddy allocator while
the AP hardware may still be performing DMA writes to it. Could this still
lead to host memory corruption?

[Severity: High]
Could returning here without clearing saved_iova and saved_isc corrupt a
future guest's memory when the queue is reassigned?

Because the vfio_ap_queue struct outlives the guest, the saved_iova and
saved_isc fields remain populated with the old guest's state. When this queue
is assigned to a new mdev and the new guest calls vfio_ap_irq_enable(),
vfio_ap_free_aqic_resources() will be called against the new q->matrix_mdev.

Will this erroneously unpin the old guest's IOVA from the new guest's VFIO
container, potentially freeing active memory belonging to the new guest and
unregistering the old ISC from the new KVM instance?

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