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

[Anthony Krowiak <akrowiak@linux.ibm.com>]

On 9/4/26 6:46 PM, sashiko-bot@kernel.org wrote:
> 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?

This will be fixed in the next version.

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