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

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

On 9/4/26 5:53 AM, sashiko-bot@kernel.org wrote:
> 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?

The AQIC resources need to be freed here.

>
> 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?

These are valid concerns.

The cases that result in verifying completion of the ZAPQ on a work 
queue are:
AP_RESPONSE_NORMAL
AP_RESPONSE_RESET_IN_PROGRESS
AP_RESPONSE_STATE_CHANGE_IN_PROGRESS

That is what will be in the q->reset_status.response_code. The NORMAL is 
fine,
but the others are not. For ret == 0, the response code needs to be set to
AP_RESPONSE_NORMAL; for ret = -ENODEV, it should be set to whatever
response code was returned from TAPQ in apq_status_check().

>
>>   		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?

The queue is probed by the vfio_ap driver precisely because the AP bus
scan detected it in the host's AP configuration, so ap_zapq() returning
AP_RESPONSE_Q_NOT_AVAIL at probe time is not possible

>
> 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?

Yes. This will be removed. There is also concern with hanging the function
indefinitely.

>
>> +		/*
>> +		 * 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?

That will go away too.

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