Re: [PATCH v7 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 1 potential issue(s) to consider:

Pre-existing issues:
- [High] A hardware error during queue reset circumvents the newly introduced security check, incorrectly marking a potentially un-zeroized queue as 'passable'.
--

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

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

This commit introduces an upper bound on the wait time for a queue reset
to complete and prevents the leakage of queue state to guests. It ensures
zeroization is strictly confirmed by returning true in _queue_passable()
only when the response code is AP_RESPONSE_NORMAL.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -785,14 +785,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;
>  }

[ ... ]

> @@ -2129,7 +2153,7 @@ static void apq_reset_check(struct work_struct *reset_work)
>  			 */
>  			vfio_ap_free_aqic_resources(q);
>  			return;

[Severity: High]
This is a pre-existing issue, but does this error path leave
q->reset_status.response_code set to AP_RESPONSE_NORMAL if a hardware or
firmware fault occurs?

When vfio_ap_mdev_reset_queue() successfully initiates a reset, ap_zapq()
returns AP_RESPONSE_NORMAL (0), which is initially copied into
q->reset_status.

If ap_tapq() subsequently returns an invalid response code causing
apq_status_check() to return -EIO, this workqueue function aborts and frees
the AQIC resources. However, it exits without updating q->reset_status to a
failure state.

Because _queue_passable() now strictly checks for AP_RESPONSE_NORMAL, could
this fail-open state result in an un-zeroized queue being incorrectly passed
to an untrusted guest, leaking cryptographic key material?

> -
> +		}
>  		if (elapsed >= AP_RESET_MAX_WAIT) {
>  			/*
>  			 * Zeroization confirmed (ret == 0): the TAPQ status bits
> @@ -2143,8 +2167,10 @@ static void apq_reset_check(struct work_struct *reset_work)
>  			 * queue cannot generate interrupts, so the NIB page is
>  			 * no longer a DMA target and it is safe to free it.
>  			 */

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