Re: [PATCH v7 01/23] vfio: Use file-based reference counting for KVM

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Unconditional error-path clearing of `device->kvm` in `vfio_df_ioctl_bind_iommufd` causes Use-After-Free of KVM VM file.
--

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

vfio: Use file-based reference counting for KVM

Replace manual module reference counting with file-based reference
counting for KVM integration. Previously, VFIO used symbol_get() to
obtain function pointers for kvm_get_kvm_safe() and kvm_put_kvm(),
then manually tracked module references through these symbols. This
approach required storing the put_kvm function pointer in each device
and carefully managing symbol references. Remove the put_kvm field in
struct vfio_device as is it no longer used.

> 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: Critical]
This is a pre-existing issue, but looking at how this function is called
from the error path in vfio_df_ioctl_bind_iommufd(), could this cause a
use-after-free of the KVM VM file?

If a process opens a VFIO cdev (df1) and successfully binds it via
VFIO_DEVICE_BIND_IOMMUFD, device->kvm is set to the KVM file and
device->open_count is 1.

If the process then opens a second cdev fd (df2) for the same device and
calls VFIO_DEVICE_BIND_IOMMUFD again without associating a KVM VM:

df2->access_granted is false, bypassing the already bound check.
vfio_df_get_kvm_safe(df2) does nothing because df2->kvm is NULL, leaving
device->kvm intact.
vfio_df_open(df2) then fails because the device is already open
(device->open_count != 0).

The error path in vfio_df_ioctl_bind_iommufd() then unconditionally drops
the KVM reference:

drivers/vfio/device_cdev.c:vfio_df_ioctl_bind_iommufd() {
    ...
out_put_kvm:
    vfio_device_put_kvm(device);
    ...
}

This calls fput(device->kvm) and sets device->kvm = NULL here in
vfio_device_put_kvm(), dropping the legitimate KVM reference held by df1.
When userspace subsequently closes the KVM fd, the VM is freed, but the
active VFIO driver (e.g., vfio_ap) retains a dangling pointer to it.

Could this lead to a use-after-free upon the next operation?

Should the error path in vfio_df_ioctl_bind_iommufd() avoid calling
vfio_device_put_kvm() if the failed caller wasn't the one that set
device->kvm?

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