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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] crypto: vfio_ap: stale reset_status allows non-operational queues to bypass zeroization validation
- [High] crypto: vfio_ap: unassigned queues bypass hardware zeroization due to 0-initialized reset_status
--

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

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

This commit limits the wait timeout for queue resets and modifies the
passability checks to ensure strict queue zeroization before guest
assignment. It also introduces a state machine callback to handle
hardware queue state transitions.

Cc: stable@vger.kernel.org

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index ad19d44a67bc4..89efb73d70355 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -881,14 +881,14 @@ static bool _queue_passable(struct vfio_ap_queue *q)
>  	if (!q)
>  		return false;
>  
> -	switch (q->reset_status.response_code) {
> -	case AP_RESPONSE_NORMAL:
> -	case AP_RESPONSE_DECONFIGURED:
> -	case AP_RESPONSE_CHECKSTOPPED:
> -		return true;
> -	default:
> -		return false;
> -	}
> +	/*
> +	 * A queue is only passable if zeroization was confirmed by
> +	 * apq_reset_check() via TAPQ status bit verification. This is
> +	 * indicated by reset_status.response_code == AP_RESPONSE_NORMAL (0).
> +	 * This is to protect against leaking the internal state of the queue
> +	 * to the guest.
> +	 */
> +	return q->reset_status.response_code == AP_RESPONSE_NORMAL;
>  }

[ ... ]

> @@ -2855,6 +2855,8 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
>  
>  	if (matrix_mdev) {
>  		vfio_ap_mdev_link_queue(matrix_mdev, q);
> +		vfio_ap_mdev_reset_queue(q);
> +		flush_work(&q->reset_work);
>  
>  		/*

[Severity: High]
Does the 0-initialization of reset_status in vfio_ap_mdev_probe_queue() cause
unassigned queues to bypass the zeroization verification in _queue_passable()?

If a queue is probed without an assigned matrix_mdev:

	memset(&q->reset_status, 0, sizeof(q->reset_status));
	INIT_WORK(&q->reset_work, apq_reset_check);

	if (matrix_mdev) {
		vfio_ap_mdev_link_queue(matrix_mdev, q);
		vfio_ap_mdev_reset_queue(q);
		...

The hardware reset is skipped, but the memset sets the response code to 0
(AP_RESPONSE_NORMAL). If this adapter is later linked to an mdev via
assign_adapter_store(), will _queue_passable() incorrectly evaluate to true and
grant the guest access to unzeroized hardware state?

[ ... ]

> +	case AP_QUEUE_CONFIG_OFF:
> +	case AP_QUEUE_CHKSTOP_ON:
> +		/*
> +		 * The queue has become non-operational. Hot-unplug its adapter
> +		 * from the guest's shadow APCB so the guest stops issuing
> +		 * operations to a queue it can no longer reach. Since queues are
> +		 * addressed via a card/domain matrix it is not possible to remove
> +		 * a single queue; the whole adapter must be unplugged.
> +		 *
> +		 * The queue remains bound to the driver and stays in
> +		 * matrix_dev->info; vfio_ap_mdev_filter_matrix() will
> +		 * re-admit it automatically when the queue returns to an
> +		 * operational state (CONFIG_ON or CHKSTOP_OFF).
> +		 */
> +		if (test_bit_inv(AP_QID_CARD(q->apqn),
> +				 matrix_mdev->shadow_apcb.apm) &&
> +		    test_bit_inv(AP_QID_QUEUE(q->apqn),
> +				 matrix_mdev->shadow_apcb.aqm)) {
> +			clear_bit_inv(AP_QID_CARD(q->apqn),
> +				      matrix_mdev->shadow_apcb.apm);
> +			vfio_ap_mdev_update_guest_apcb(matrix_mdev);
> +			reset_queues_for_apid(matrix_mdev,
> +					      AP_QID_CARD(q->apqn));
> +		}
> +		break;

[Severity: High]
The commit message states that for non-operational queues, q->reset_status
is zeroed and its response code is set to AP_RESPONSE_DECONFIGURED or
AP_RESPONSE_CHECKSTOPPED.

However, does this block miss updating the reset status?

If the queue retains an old AP_RESPONSE_NORMAL (0) status from when it was
operational, can _queue_passable() incorrectly evaluate to true and allow the
queue to bypass the required hardware zeroization when later evaluated by the
filtering logic?

> +	default:
> +		break;
> +	}
> +
> +out_unlock:
> +	release_update_locks_for_mdev(matrix_mdev);
> +}

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