Re: [PATCH v5 03/15] s390/vfio-ap: Functions to initialize/release vfio device migration data

[sashiko-bot@kernel.org]

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

New issues:
- [High] ABBA deadlock (circular locking dependency) between `matrix_dev->mdevs_lock` and the VFIO core's `dev_set->lock`.

Pre-existing issues:
- [Critical] Failing to attach a second mdev to a KVM instance corrupts `kvm->arch.crypto.pqap_hook`, leading to a Use-After-Free.
--

--- Patch [3]: [PATCH v5 03/15] s390/vfio-ap: Functions to initialize/release vfio device migration data ---
commit ab0b2af7d9f1c49f7cea7a7d2157bcc5a0bb9b7a
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390/vfio-ap: Functions to initialize/release vfio device migration data

Provides the functions that initialize and release the data structures
used during live guest migration:

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 71d3e2c2e07c6..47aca908701d1 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
[ ... ]
> @@ -797,18 +809,29 @@ static int vfio_ap_mdev_probe(struct mdev_device *mdev)
>  	if (IS_ERR(matrix_mdev))
>  		return PTR_ERR(matrix_mdev);
>  
> +	mutex_lock(&matrix_dev->mdevs_lock);
> +
> +	/*
> +	 * Migration capabilities must be initialized before calling
> +	 * vfio_register_emulated_iommu_dev; otherwise, the VFIO core
> +	 * will see mig_ops as NULL during the registration. This could
> +	 * prevent the VFIO core from properly setting up migration
> +	 * infrastructure like debugfs entries.
> +	 */
> +	vfio_ap_init_migration_capabilities(matrix_mdev);
> +
>  	ret = vfio_register_emulated_iommu_dev(&matrix_mdev->vdev);

[Severity: High]
Does taking mdevs_lock before calling vfio_register_emulated_iommu_dev()
introduce an ABBA deadlock?

In vfio_ap_mdev_probe(), we now hold mdevs_lock when calling
vfio_register_emulated_iommu_dev(), which takes dev_set->lock inside the VFIO
core via vfio_assign_device_set().

However, in vfio_ap_mdev_open_device() below, which is called by the VFIO core
with dev_set->lock already held, we now acquire mdevs_lock.

Will this reverse lock ordering trigger a lockdep circular dependency splat
when userspace probes and opens the device?

[ ... ]
> @@ -2052,19 +2075,39 @@ static int vfio_ap_mdev_reset_qlist(struct list_head *qlist)
>  
>  static int vfio_ap_mdev_open_device(struct vfio_device *vdev)
>  {
> -	struct ap_matrix_mdev *matrix_mdev =
> -		container_of(vdev, struct ap_matrix_mdev, vdev);
> +	struct ap_matrix_mdev *matrix_mdev;
> +	int ret;
>  
>  	if (!vdev->kvm)
>  		return -EINVAL;
>  
> -	return vfio_ap_mdev_set_kvm(matrix_mdev, vdev->kvm);
> +	mutex_lock(&matrix_dev->mdevs_lock);
> +	matrix_mdev = container_of(vdev, struct ap_matrix_mdev, vdev);
> +	ret = vfio_ap_init_migration_data(matrix_mdev);
> +	mutex_unlock(&matrix_dev->mdevs_lock);
> +
> +	if (ret)
> +		return ret;
> +
> +	ret = vfio_ap_mdev_set_kvm(matrix_mdev, vdev->kvm);

[Severity: Critical]
This is a pre-existing issue, but I noticed that if vfio_ap_mdev_set_kvm()
fails (for instance, returning -EPERM because another mdev is already attached
to this KVM), does it leave kvm->arch.crypto.pqap_hook pointing to the new
mdev's pqap_hook?

Looking at vfio_ap_mdev_set_kvm() in drivers/s390/crypto/vfio_ap_ops.c, it
unconditionally overwrites KVM's pqap_hook before checking if another mdev is
already attached:

	down_write(&kvm->arch.crypto.pqap_hook_rwsem);
	kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook;
	up_write(&kvm->arch.crypto.pqap_hook_rwsem);
	
	get_update_locks_for_kvm(kvm);
	
	list_for_each_entry(m, &matrix_dev->mdev_list, node) {
		if (m != matrix_mdev && m->kvm == kvm) {
			release_update_locks_for_kvm(kvm);
			return -EPERM;
		}
	}

If this fails with -EPERM, the pointer is not restored. If this new mdev is
subsequently closed and freed, could a malicious guest executing a PQAP
instruction trigger a use-after-free in the host when handle_pqap()
dereferences this freed pointer?

> +	if (ret) {
> +		/* Clean up migration data on failure */
> +		mutex_lock(&matrix_dev->mdevs_lock);
> +		vfio_ap_release_migration_data(matrix_mdev);
> +		mutex_unlock(&matrix_dev->mdevs_lock);
> +	}
> +
> +	return ret;
>  }

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