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

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

On 9/25/26 9:03 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] crypto: vfio: Use-After-Free of vfio_ap_queue in vfio_ap_on_qstate_transition
> - [High] crypto: vfio: TOCTOU on q->matrix_mdev leads to unprotected KVM state updates and deadlocks
> - [Low] crypto: vfio: Incorrect comment for __drv_notify_qstate_transitioned
> --
>
> commit 6964322bafaab43ec603a04a262b5d1370b6779d
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
>
> s390/vfio-ap: fix queue state leakage to guest and host
>
> This patch limits the wait for a queue reset to complete and prevents the
> leakage of queue state to a guest. It introduces an on_qstate_transition
> callback to track queue state changes and restricts queue passability to
> fully zeroized states.
>
> Cc: stable@vger.kernel.org
>
>> diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
>> index d82df5b4e2db3..a53cfad3543eb 100644
>> --- a/drivers/s390/crypto/ap_bus.c
>> +++ b/drivers/s390/crypto/ap_bus.c
> [ ... ]
>> @@ -1979,6 +1979,28 @@ static inline void notify_scan_complete(void)
>>   			 __drv_notify_scan_complete);
>>   }
>>   
>> +/* Helper function for notify_config_changed */
>> +static int __drv_notify_qstate_transitioned(struct device_driver *drv, void *data)
> [Severity: Low]
> Is this comment accurate? It states this is a helper function for
> notify_config_changed, but the function name and usage indicate it is a
> helper for notify_qstate_transitioned.

This will be fixed ion the forthcoming v9 patch series

>
>> +{
> [ ... ]
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index ffc2d8715bd96..cd4a436c43194 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> [ ... ]
>> @@ -3309,3 +3310,46 @@ void vfio_ap_on_scan_complete(struct ap_config_info *new_config_info,
>>   
>>   	mutex_unlock(&matrix_dev->guests_lock);
>>   }
>> +
>> +/**
>> + * vfio_ap_on_qstate_transition:
>> + *Instead of calling vfio_ap_find_queue() without locks and reading q->matrix_mdev, vfio_ap_on_qstate_transition() now calls get_update_locks_by_apqn(apqn)
>> + * AP bus callback notifying the vfio_ap device driver that the state of a
>> + * queue has transitioned.
>> + *
>> + * @qstate_trans: the object containing a reference to the queue device and the
>> + *		  state to which it transitioned.
>> + */
>> +void vfio_ap_on_qstate_transition(struct ap_qstate_transition *qstate_trans)
>> +{
>> +	struct vfio_ap_queue *q = vfio_ap_find_queue(qstate_trans->queue->qid);
>> +	DECLARE_BITMAP(apm_filtered, AP_DEVICES);
>> +
>> +	/*
>> +	 * If the queue is not bound to the vfio_ap device driver, then it won't
>> +	 * be passed through to a guest; so, no need to continue.
>> +	 */
>> +	if (!q)
>> +		return;
>> +
>> +	get_update_locks_for_mdev(q->matrix_mdev);
> [Severity: High]
> Can this dereference a freed pointer?
>
> If vfio_ap_find_queue() reads the device's drvdata and drops the device
> reference before returning, q has no reference count. Because
> vfio_ap_on_qstate_transition() holds no locks, a concurrent sysfs unbind
> can invoke vfio_ap_mdev_remove_queue(), which clears drvdata and calls
> kfree(q).
>
> This would cause a use-after-free when reading q->matrix_mdev.
>
> Additionally, is there a TOCTOU race when reading q->matrix_mdev without
> locks?
>
> If q->matrix_mdev is NULL here, get_update_locks_for_mdev() will skip
> acquiring kvm->lock. If another thread concurrently updates q->matrix_mdev
> to a non-NULL value while waiting on guests_lock, this could lead to issues
> later in the function.

This is a legitimate concern.
Instead of calling vfio_ap_find_queue() without locks and reading 
q->matrix_mdev,
matrix_mdev = get_update_locks_by_apqn(apqn) can be used since the apqn 
can be
obtained from qstate_trans->queue->qid. get_update_locks_by_apqn() 
acquires matrix_dev->guests_lock, searches matrix_dev->mdev_list for the 
mdev that owns the apqn, acquires matrix_mdev->kvm->lock (if attached to 
a guest), and finally acquires matrix_dev->mdevs_lock. Lookup of q via 
vfio_ap_mdev_get_queue() is done entirely under the protection of all 
three locks, completely eliminating the UAF and TOCTOU races.
>
>> +
>> +	switch (qstate_trans->new_state) {
>> +	case AP_QUEUE_CONFIG_ON:
>> +	case AP_QUEUE_CHKSTOP_OFF:
>> +		vfio_ap_mdev_reset_queue(q);
>> +		flush_work(&q->reset_work);
> [Severity: High]
> Does this also cause a use-after-free for the same reason? Passing the
> potentially freed queue object to vfio_ap_mdev_reset_queue(q) and flushing
> its work struct could trigger further memory corruption.

As stated above, using matrix_mdev = get_update_locks_by_apqn(apqn) should
also resolve this issue. If q is retrieved from matrix_mdev under
mdevs_lock, and vfio_ap_mdev_remove_queue() requires mdevs_lock to unlink
and free q, q is guaranteed valid throughout the reset and
flush_work(&q->reset_work) sequence.

>
>> +
>> +		if (q->matrix_mdev) {
>> +			if (vfio_ap_mdev_filter_matrix(q->matrix_mdev, apm_filtered)) {
>> +				vfio_ap_mdev_update_guest_apcb(q->matrix_mdev);
> [Severity: High]
> If q->matrix_mdev changed concurrently after get_update_locks_for_mdev()
> was called, could this update the KVM guest state without having acquired
> the proper KVM locks?

As stated above, using matrix_mdev = get_update_locks_by_apqn(apqn) should
also resolve this issue. If an mdev is attached to a guest
(matrix_mdev->kvm != NULL), kvm->lock is held. All matrix filtering,
APCB updates (vfio_ap_mdev_update_guest_apcb()), and queue resets operate
strictly on the locked matrix_mdev handle, with no unprotected pointer
re-reads.

>
>> +				reset_queues_for_apids(q->matrix_mdev, apm_filtered);
>> +			}
>> +		}
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +Instead of calling vfio_ap_find_queue() without locks and reading q->matrix_mdev, vfio_ap_on_qstate_transition() now calls get_update_locks_by_apqn(apqn)
>> +	release_update_locks_for_mdev(q->matrix_mdev);
> [Severity: High]
> If q->matrix_mdev was modified between the initial acquisition and here,
> would this call mutex_unlock() on a lock it did not acquire, or leave
> an acquired lock perpetually locked?

As stated above, using matrix_mdev = get_update_locks_by_apqn(apqn) should
also resolve this issue. At the exit point, the locks can be freed with
release_update_locks_for_mdev(matrix_mdev), passing the exact same
matrix_mdev that was locked by get_update_locks_by_apqn(). This guarantees
100% symmetry between lock acquisition and release under all execution
paths.

>
>> +}