Re: [PATCH v4 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:
- [Critical] The timeout path incorrectly treats an idle queue state as a completed reset, bypassing the necessary zeroize command and causing Use-After-Free.
- [High] The reset operation silently reports success to callers when the reset times out, masking the failure and misleading userspace.
- [Medium] The `elapsed` timer is not reset when a new reset command is issued, causing subsequent resets to prematurely time out.
--

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

s390/vfio-ap: Fix unbounded loop in apq_reset_check()

This commit introduces a 2000ms timeout for queue reset operations in
apq_reset_check() to prevent indefinite blocking in the worker thread.
Upon timeout, it skips freeing AQIC resources to avoid a use-after-free,
leaking them instead.

Fixes: dd174833e44e ("s390/vfio-ap: remove upper limit on wait for queue reset to complete")
Cc: stable@vger.kernel.org

The commit message states:

>    Note that on timeout, q->reset_status will hold the status from the most
>    recent reset operation so that callers inspecting
>    q->reset_status.response_code after flush_work() will see the value
>    and can return an appropriate return code.

Is this description accurate?

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 363d9e53e2490..3f5b012be450c 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2067,6 +2068,37 @@ 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, or because interrupts are not possible on a
> +			 * non-operational queue.
> +			 */
> +			if (!ret)
> +				goto done;

[Severity: Critical]
Does this incorrectly treat an idle queue as a successfully reset queue?

If the queue is continuously busy for 2000ms, the initial ap_zapq() command
is rejected (returning AP_RESPONSE_BUSY). On the exact iteration where
elapsed reaches AP_RESET_MAX_WAIT (2000ms), if the queue finally stops being
busy, ap_tapq() returns 0.

Could this misinterpret the 0 as a completed reset, jump to done, and free
the AQIC resources? Since the queue was never actually zeroized, interrupts
remain enabled. Can this lead to a host kernel use-after-free if the hardware
writes to the freed NIB page?

> +			/*
> +			 * 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.
> +			 */
> +
> +			return;

[Severity: High]
Does this return early on timeout without updating q->reset_status to indicate
a failure?

If the queue reset times out and q->reset_status.response_code was 0 from the
initial accepted command, it remains unmodified.

Would callers like vfio_ap_mdev_reset_queues() check this unmodified response
code, incorrectly assume the reset succeeded, and mislead userspace into
believing the VFIO device was successfully reset while the hardware is still
operational?

[ ... ]

> @@ -2083,11 +2115,13 @@ static void apq_reset_check(struct work_struct *reset_work)
>  				memcpy(&q->reset_status, &status, sizeof(status));
>  				continue;

[Severity: Medium]
Is the elapsed timer supposed to be reset when a new reset command is issued
here?

If the queue is initially busy, takes time (e.g., 1980ms) to become idle, and
requires a retry of the ap_zapq() command, the next loop iteration will
instantly hit the 2000ms limit because elapsed was not cleared.

Could this prematurely time out subsequent legitimate reset operations and
result in resource leaks?

>  			}
> -			if (q->saved_isc != VFIO_AP_ISC_INVALID)
> -				vfio_ap_free_aqic_resources(q);
> -			break;
> +			goto done;
>  		}
>  	}
> +
> +done:
> +	if (q->saved_isc != VFIO_AP_ISC_INVALID)
> +		vfio_ap_free_aqic_resources(q);
>  }

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