Re: [PATCH v6 5/5] s390/vfio-ap: fix queue state leakage to guest and host

[sashiko-bot@kernel.org]

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

New issues:
- [High] `apq_reset_check()` fails to update `q->reset_status` on successful completion, breaking queue assignment and state tracking.
- [High] `matrix_mdev` is accessed after its protecting locks are released in `vfio_ap_mdev_remove_queue()`, leading to a UAF.
- [Medium] Unconditional queue resets during probe and removal bypass the host configuration check, triggering unhandled kernel WARNINGs.

Pre-existing issues:
- [High] Early return on `-EIO` in `apq_reset_check()` bypasses freeing AQIC resources, leading to a resource leak and Use-After-Free.
--

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

s390/vfio-ap: fix queue state leakage to guest and host

This patch modifies the queue reset logic to prevent state leakage to guests
and hosts. It changes the passable state for queues to strictly require
AP_RESPONSE_NORMAL, introduces a variable maximum wait time for resets, and
ensures queues are properly reset and verified during probe and removal.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 6a964f82c8e89..e054fd4a94970 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2113,7 +2113,7 @@ static void apq_reset_check(struct work_struct *reset_work)
>  		ret = apq_status_check(q->apqn, &status);
>  		if (ret == -EIO)
>  			return;

[Severity: High]
This isn't a bug introduced by this patch, but does returning early on -EIO
here bypass resource cleanup at the done: label?

If a hardware failure causes an unrecognized response code,
vfio_ap_free_aqic_resources() is skipped here and in the default switch case of
vfio_ap_mdev_reset_queue(), but q is eventually freed.

Can the guest's gmap notifier remain registered, holding a pointer to q,
resulting in a use-after-free when the notifier dereferences the freed q
pointer?

> -		if (elapsed >= AP_RESET_MAX_WAIT) {
> +		if (q->reset_max_wait && elapsed >= q->reset_max_wait) {
>  			/*
>  			 * Zeroization confirmed (ret == 0): the TAPQ status bits
>  			 * indicate the async portion of the ZAPQ completed

[ ... ]

> @@ -2161,6 +2161,8 @@ static void apq_reset_check(struct work_struct *reset_work)
>  
>  			return;
>  		}
> +		if (!ret || ret == -ENODEV)
> +			goto done;

[Severity: High]
Does this early exit skip updating the queue's reset_status with the actual
final state?

If apq_reset_check() loops until ap_tapq() indicates successful zeroization
(ret == 0), but jumps to done without updating q->reset_status to
AP_RESPONSE_NORMAL, does this mean _queue_passable() will incorrectly reject
successfully zeroized queues?

Furthermore, could this cause the new logic in vfio_ap_mdev_remove_queue() to
evaluate q->reset_status.response_code != AP_RESPONSE_NORMAL to true,
triggering an incorrect redundant reset that blocks indefinitely?

>  		if (ret == -EBUSY) {
>  			pr_notice_ratelimited(WAIT_MSG, elapsed,
>  					      AP_QID_CARD(q->apqn),

[ ... ]

> @@ -2675,8 +2675,10 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
>  
>  	q->apqn = apqn;
>  	q->saved_isc = VFIO_AP_ISC_INVALID;
> -	memset(&q->reset_status, 0, sizeof(q->reset_status));
> +	q->reset_max_wait = AP_RESET_MAX_WAIT;
>  	INIT_WORK(&q->reset_work, apq_reset_check);
> +	vfio_ap_mdev_reset_queue(q);
> +	flush_work(&q->reset_work);

[Severity: Medium]
Will this unconditional reset during probe trigger a WARN if the probed device
is not in the host's AP configuration?

When an AP device is bound to the vfio_ap driver but is not present in the
host's AP configuration, ap_zapq() fails with response code 0x01.

Does this feed the 0x01 response code into vfio_ap_mdev_reset_queue(), which
falls through to the default switch case and triggers a WARN?

>  
>  	if (matrix_mdev) {
>  		vfio_ap_mdev_link_queue(matrix_mdev, q);

[ ... ]

> @@ -2759,8 +2761,19 @@ void vfio_ap_mdev_remove_queue(struct ap_device *apdev)
>  		vfio_ap_unlink_queue_fr_mdev(q);
>  
>  	dev_set_drvdata(&apdev->device, NULL);
> -	kfree(q);
>  	release_update_locks_for_mdev(matrix_mdev);
> +
> +	if (q->reset_status.response_code != AP_RESPONSE_NORMAL) {

[Severity: High]
Are the locks protecting matrix_mdev dropped before the blocking reset
operations, exposing it to concurrent freeing?

vfio_ap_unlink_queue_fr_mdev() unlinks the queue but leaves the dangling
pointer q->matrix_mdev intact.

If flush_work() blocks indefinitely and a concurrent unbind tears down the
mdev and frees matrix_mdev, will vfio_ap_free_aqic_resources() dereference
q->matrix_mdev->kvm, resulting in a use-after-free?

> +		/*
> +		 * Loop until zeroization of queue is verified so we don't leak
> +		 * the internal state of the queue to the caller.
> +		 */
> +		q->reset_max_wait = 0;
> +		vfio_ap_mdev_reset_queue(q);
> +		flush_work(&q->reset_work);

[Severity: Medium]
Does this fallback reset completely ignore the test_bit_inv() host
configuration check present earlier in this function, hitting the same WARN on
unbind if the device is not in the host's AP configuration?

> +	}
> +
> +	kfree(q);
>  }
>  
>  /**

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