Re: [PATCH v5 3/9] s390/vfio-ap: Fix missing lock required to access list of ap_matrix_mdev objects

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Registering the emulated IOMMU device before adding it to the mdev list allows a race condition where multiple mdevs can be attached to the same KVM instance, bypassing the strict 1-to-1 isolation check.
- [High] Eventfd context references (`req_trigger` and `cfg_chg_trigger`) are leaked if the VFIO device is closed or removed without explicitly unregistering the IRQs.
--

--- Patch [3]: [PATCH v5 3/9] s390/vfio-ap: Fix missing lock required to access list of ap_matrix_mdev objects ---
commit 225a264b9dad5df5a833e518a4dd594cafd35d03
Author: Anthony Krowiak <[email protected]>

s390/vfio-ap: Fix missing lock required to access list of ap_matrix_mdev objects

This commit fixes a locking issue where the mdev_list was accessed without
holding the matrix_dev->guests_lock mutex. It ensures the lock is properly
acquired during device probe and status show operations. Unnecessary NULL
initializations for triggers are also removed.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index c6bee69cc22f6..f2d662e388bdc 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -800,12 +800,17 @@ static int vfio_ap_mdev_probe(struct mdev_device *mdev)
>  	ret = vfio_register_emulated_iommu_dev(&matrix_mdev->vdev);
>  	if (ret)
>  		goto err_put_vdev;

[Severity: Critical]
This is a pre-existing issue, but does registering the emulated IOMMU
device here expose it to userspace before it is added to the mdev list?

If userspace concurrently opens the device and calls vfio_ap_mdev_set_kvm(),
that function traverses the mdev list to check for duplicate KVM references.
Since the device is not in the list yet, this check can succeed. Could this
allow multiple mdevs to attach to the same KVM instance and bypass the
isolation check?

> -	matrix_mdev->req_trigger = NULL;
> -	matrix_mdev->cfg_chg_trigger = NULL;

[Severity: High]
This is a pre-existing issue, but does this code leak the eventfd contexts
if userspace sets them and then closes the device without explicitly
unregistering the IRQs?

When userspace sets triggers via VFIO_DEVICE_SET_IRQS, eventfd_ctx_fdget()
takes a reference. If the device fd is later closed, the cleanup sequence
happens:

vfio_ap_mdev_close_device()
vfio_ap_mdev_remove()
   vfio_put_device(&matrix_mdev->vdev);

Neither function appears to release the req_trigger or cfg_chg_trigger
references. Can this lead to an indefinite leak of the eventfd_ctx memory
since there is no release callback to clean them up when the matrix_mdev
is freed?

> +
> +	/*
> +	 * Take the matrix_dev->guests_lock mutex before adding the matrix_mdev
> +	 * to the mdev_list. All functions that traverse the list must also hold
> +	 * this lock to guard against additions to or removals from the list
> +	 * while it is being traversed.
> +	 */
> +	mutex_lock(&matrix_dev->guests_lock);
>  	dev_set_drvdata(&mdev->dev, matrix_mdev);
> -	mutex_lock(&matrix_dev->mdevs_lock);
>  	list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
> -	mutex_unlock(&matrix_dev->mdevs_lock);
> +	mutex_unlock(&matrix_dev->guests_lock);
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3