Re: [PATCH v5 1/5] vfio: cache KVM VM file references instead of raw struct kvm pointers

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Concurrent BIND_IOMMUFD ioctls on multiple cdev file descriptors for the same device unconditionally overwrite device->kvm_file, leading to reference leaks and Use-After-Free.
--

commit 9a5907f1fb1069cc250703cfb404e5a7be33bf23
Author: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>

vfio: cache KVM VM file references instead of raw struct kvm pointers

This commit switches VFIO to track the VM's struct file instead of raw
struct kvm pointers to manage the KVM VM lifetime. This change allows
VFIO and iommufd to use normal file references for VM lifetime rather
than relying on KVM's internal refcounting.

> diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
> index 54abf312cf049..ca75ab8eb7bd7 100644
> --- a/drivers/vfio/device_cdev.c
> +++ b/drivers/vfio/device_cdev.c

[ ... ]

> @@ -133,10 +133,10 @@ long vfio_df_ioctl_bind_iommufd(struct vfio_device_file *df,
>  	}
>  
>  	/*
> -	 * Before the device open, get the KVM pointer currently
> -	 * associated with the device file (if there is) and obtain
> -	 * a reference.  This reference is held until device closed.
> -	 * Save the pointer in the device for use by drivers.
> +	 * Before the device open, get the VM struct file currently
> +	 * associated with the device file (if there is one) and obtain a
> +	 * reference. This reference is held until the device is closed.
> +	 * Save the file in the device for use by drivers.
>  	 */
>  	vfio_df_get_kvm_safe(df);
>  

[Severity: High]
This isn't a bug introduced by this patch, but does this unconditionally
overwrite device->kvm_file if concurrent BIND_IOMMUFD ioctls are called on
multiple cdev file descriptors for the same device?

If a user opens a VFIO device cdev twice, getting two distinct file
descriptors (df1 and df2) for the same device, and successfully binds df1
to IOMMUFD:

- device->kvm_file is set (with an extra refcount)
- device->open_count increments to 1

When the user attempts to bind df2, the check for df->access_granted only
checks df2, so execution proceeds.

Calling vfio_df_get_kvm_safe(df2) will overwrite device->kvm_file with
df2's KVM file. Could the previous file reference from df1 be leaked because
no fput() is called?

If vfio_df_open(df2) then returns an error (because device->open_count is 1),
the error path jumps to out_put_kvm and calls vfio_device_put_kvm(device).
This drops the new reference and clears device->kvm_file to NULL.

This leaves df1 bound and functional, but its KVM association is destroyed.
When df1 is eventually closed, hardware drivers like vfio_pci_zdev that check
vfio_device_get_kvm() will see a NULL pointer and skip their KVM
unregistration hooks. Can this leave KVM hooked to a closed device,
potentially leading to a use-after-free when KVM accesses it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525154816.1029642-1-aneesh.kumar@kernel.org?part=1