Re: [PATCH v8 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 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.

> +{
[ ... ]
> 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:
> + *
> + * 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.

> +
> +	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.

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

> +				reset_queues_for_apids(q->matrix_mdev, apm_filtered);
> +			}
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	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?

> +}

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