Re: [PATCH v2] vfio: Use file-based reference counting for KVM

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Untyped `struct file *` for KVM VM leads to type confusion and out-of-bounds access in VFIO device drivers when multiple concurrent KVM modules are loaded.

Pre-existing issues:
- [High] Pre-existing bug in the cdev IOMMUFD binding path allows concurrent opens to leak the KVM file reference and prematurely clear `device->kvm`.
--

commit dd4fd68a7d7d9888bc552f388401d95d1f137c5f
Author: Steffen Eiden <seiden@linux.ibm.com>

vfio: Use file-based reference counting for KVM

This patch replaces manual module reference counting with file-based reference
counting for KVM integration in VFIO. It passes struct file pointers instead
of struct kvm pointers across the interface to leverage the kernel's existing
file reference counting mechanism. This removes exported symbols and supports
multiple concurrent KVM modules.

Link: https://lore.kernel.org/all/20260831144802.834315-1-seiden@linux.ibm.com/
Link: https://lore.kernel.org/r/20260812-vfio-v1-1-5cfe0b1fa4e7@linux.ibm.com

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 940c0ff668bee..f160e1945fbce 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c

[ ... ]

> @@ -1822,17 +1822,27 @@ static const struct attribute_group *vfio_ap_mdev_attr_groups[] = {
>  
>  /**
>   * vfio_ap_mdev_set_kvm - sets all data for @matrix_mdev that are needed
> - * to manage AP resources for the guest whose state is represented by @kvm
> + * to manage AP resources for the guest whose state is represented by
> + * @kvm_file
>   *
>   * @matrix_mdev: a mediated matrix device
> - * @kvm: reference to KVM instance
> + * @kvm_file: the KVM VM file this vfio device is associated with
>   *
> - * Return: 0 if no other mediated matrix device has a reference to @kvm;
> + * Return: 0 if no other mediated matrix device has a reference to the VM;
>   * otherwise, returns an -EPERM.
>   */
>  static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
> -				struct kvm *kvm)
> +				struct file *kvm_file)
>  {
> +	struct kvm *kvm;
> +
> +	if (!kvm_file)
> +		return -ENOENT;
> +
> +	kvm = kvm_file->private_data;
> +	if (!kvm)
> +		return -ENOENT;
> +
>  	if (kvm->arch.crypto.crycbd) {

[Severity: Critical]
Does this code adequately verify that the file is an s390 KVM file before
casting its private_data in vfio_ap_mdev_set_kvm() inside
drivers/s390/crypto/vfio_ap_ops.c?

If a user instantiates a VM using a concurrent KVM-arm64 module and attaches
an s390-specific VFIO device to its KVM device, could this driver extract the
untyped private_data and mistakenly treat an arm64 struct kvm as an s390 one?

Because the memory layouts of struct kvm_arch differ across architectures,
would accessing s390-specific fields in the arch member (like
kvm->arch.crypto.pqap_hook) result in arbitrary memory corruption?

>  		get_update_locks_for_kvm(kvm);
>  		if (kvm->arch.crypto.pqap_hook) {

[ ... ]

> diff --git a/drivers/vfio/pci/vfio_pci_zdev.c b/drivers/vfio/pci/vfio_pci_zdev.c
> index f47f36314a1cc..045132e90bfee 100644
> --- a/drivers/vfio/pci/vfio_pci_zdev.c
> +++ b/drivers/vfio/pci/vfio_pci_zdev.c
> @@ -184,6 +184,7 @@ int vfio_pci_zdev_feature_err(struct vfio_device *device, u32 flags,
>  int vfio_pci_zdev_open_device(struct vfio_pci_core_device *vdev)
>  {
>  	struct zpci_dev *zdev = to_zpci(vdev->pdev);
> +	struct kvm *kvm;
>  	int ret;
>  
>  	if (!zdev)
> @@ -195,9 +196,14 @@ int vfio_pci_zdev_open_device(struct vfio_pci_core_device *vdev)
>  		return 0;
>  
>  	ret = -ENOENT;
> +	kvm = vdev->vdev.kvm->private_data;
> +	if (!kvm)
> +		goto recovery;
> +
>  	if (zpci_kvm_hook.kvm_register)
> -		ret = zpci_kvm_hook.kvm_register(zdev, vdev->vdev.kvm);
> +		ret = zpci_kvm_hook.kvm_register(zdev, kvm);

[Severity: Critical]
Is it possible for a similar regression to occur here in
vfio_pci_zdev_open_device() within drivers/vfio/pci/vfio_pci_zdev.c?

If an arm64 VM file is passed via vdev->vdev.kvm, passing it to
zpci_kvm_hook.kvm_register() could potentially cause type confusion in the
host kernel.

>  
> +recovery:
>  	if (ret)
>  		zpci_stop_mediated_recovery(zdev);

[ ... ]

> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index 423ead48aafe3..ed96acfa86358 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c

[ ... ]

> @@ -511,14 +489,7 @@ void vfio_device_put_kvm(struct vfio_device *device)
>  	if (!device->kvm)
>  		return;
>  
> -	if (WARN_ON(!device->put_kvm))
> -		goto clear;
> -
> -	device->put_kvm(device->kvm);
> -	device->put_kvm = NULL;
> -	symbol_put(kvm_put_kvm);
> -
> -clear:
> +	fput(device->kvm);
>  	device->kvm = NULL;

[Severity: High]
This is a pre-existing issue, but does this error path leave the first active
file descriptor with a NULL KVM pointer during concurrent cdev IOMMUFD binds?

Looking at vfio_df_ioctl_bind_iommufd() in drivers/vfio/device_cdev.c, if a
user opens a VFIO cdev twice, obtaining two vfio_device_file (df) instances:

1. bind_iommufd on the first df succeeds, setting device->kvm and
device->open_count.

2. bind_iommufd on the second df bypasses the df->access_granted check and
unconditionally calls vfio_df_get_kvm_safe(df). If a KVM file was linked to
the second df, this overwrites device->kvm with the second file's reference
without releasing the first, causing a resource leak.

3. The subsequent vfio_df_open(df) fails because device->open_count != 0.

4. The error path then calls vfio_device_put_kvm() in
drivers/vfio/vfio_main.c, which drops the reference and sets
device->kvm = NULL.

Could this sequence leave the first df operating with a NULL device->kvm,
leading to potential NULL pointer dereferences in drivers?

>  }
>  #endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903-vfio-v2-1-ef4cd4190ae7@linux.ibm.com?part=1