Re: [PATCH v4 03/27] KVM, vfio: remove symbol_get(kvm_put_kvm) from vfio

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Concurrent or subsequent bindings of cdev file descriptors to the same VFIO device overwrite the `device->kvm` and `device->kvm_module` pointers without dropping the previous reference, causing a KVM instance memory leak and premature KVM reference drop.
- [High] The architecture-specific KVM module (e.g., kvm-intel.ko) can be unloaded while VFIO still holds a reference to a KVM instance, leading to execution of freed memory when the instance is eventually destroyed.
--

commit 31b847131145e41ad6d50358b1c90ea462e38022
Author: Paolo Bonzini <pbonzini@redhat.com>

KVM, vfio: remove symbol_get(kvm_put_kvm) from vfio
    
Right now, KVM and VFIO are using symbol_get to access each other's
symbols because of a circular reference between the modules, as well
as to avoid loading them unnecessarily.
    
The remaining use in VFIO is for kvm_put_kvm, which is not inline
because it needs to call kvm_destroy_vm.  However, storing the
address of kvm_destroy_vm in the "struct kvm" is enough to remove
the dependency from VFIO.

> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index b7b8b45f1cec2..6bddea4d384ff 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c

[ ... ]

> @@ -451,9 +451,6 @@ EXPORT_SYMBOL_GPL(vfio_unregister_group_dev);
>  void vfio_device_get_kvm_safe(struct vfio_device *device, struct kvm *kvm,
>  			      struct module *kvm_module)
>  {
> -	void (*pfn)(struct kvm *kvm);
> -	bool ret;
> -
>  	lockdep_assert_held(&device->dev_set->lock);
>  
>  	if (!kvm)
> @@ -462,21 +459,13 @@ void vfio_device_get_kvm_safe(struct vfio_device *device, struct kvm *kvm,
>  	if (!try_module_get(kvm_module))
>  		return;

[Severity: High]
This isn't a bug introduced by this patch, but does pinning only the generic
kvm.ko module leave us vulnerable to the architecture-specific module being
unloaded?

When VFIO acquires a reference via try_module_get(kvm_module), it doesn't
pin the architecture module (e.g., kvm-intel.ko). If userspace closes the
KVM fds, the VFS reference count drops to 0, allowing an administrator to
unload the arch module. 

Since VFIO still holds a reference to the struct kvm, when VFIO later drops
its reference, KVM destruction proceeds:

kvm_destroy_vm()
    kvm_arch_pre_destroy_vm(kvm);
    ...
    kvm_arch_destroy_vm(kvm);

Could these calls into the architecture-specific teardown execute unmapped
memory if the arch module has already been unloaded?

> -	pfn = symbol_get(kvm_put_kvm);
> -	if (WARN_ON(!pfn))
> +	if (!kvm_get_kvm_safe(kvm))
>  		goto out_put_mod;
>  
> -	ret = kvm_get_kvm_safe(kvm);
> -	if (!ret)
> -		goto out_put_sym;
> -
> -	device->put_kvm = pfn;
>  	device->kvm = kvm;
>  	device->kvm_module = kvm_module;

[Severity: High]
This is a pre-existing issue, but if multiple cdev file descriptors are
opened for the same VFIO device, does this overwrite device->kvm without
dropping the previous reference?

For instance, if vfio_df_get_kvm_safe() is called successfully on a second
file descriptor, but then vfio_df_open() fails because
device->open_count != 0:

drivers/vfio/device_cdev.c:vfio_df_ioctl_bind_iommufd() {
    ...
    vfio_df_get_kvm_safe(df);
    
    ret = vfio_df_open(df);
    if (ret)
        goto out_put_kvm;
    ...
}

Would this leak the first KVM instance reference since device->kvm is
unconditionally overwritten here?

Additionally, if the second fd has no KVM instance associated, wouldn't the
error path's call to vfio_device_put_kvm() prematurely drop the original
reference and clear device->kvm?

>  	return;

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