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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Architectural Use-After-Free and Type Confusion due to dangling `kvm->file` and `get_file_active` API misuse.
- [High] The KVM file reference is permanently leaked in `vfio_ap_mdev_set_kvm()` when KVM crypto is unavailable.
- [Low] Unused parameter 'file' added to `kvm_ioctl_create_device`.
--

--- Patch [1]: [PATCH v5 01/31] vfio: Use file-based reference counting for KVM ---
commit 0d6533aa3c5f21c080ec0d95c626b95fd5fb711d
Author: Steffen Eiden <[email protected]>

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.

Pass struct file pointers instead of struct kvm pointers throughout the
VFIO-KVM interface. This leverages the kernel's existing file reference
counting mechanism via get_file_active() and fput(), eliminating the
need for manual module reference tracking. The file->private_data field
provides access to the underlying struct kvm when needed.

This simplifies the code and removes all remaining externally exported
symbols for KVM, paving the path for a second concurrent KVM module.

Suggested-by: Jason Gunthorpe <[email protected]>
Suggested-by: Sean Christopherson <[email protected]>
Signed-off-by: Steffen Eiden <[email protected]>

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 44b3a1dcc1b36..177c8f5ab6b91 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
[ ... ]
> @@ -1815,13 +1816,26 @@ static const struct attribute_group *vfio_ap_mdev_attr_groups[] = {
>   * @matrix_mdev: a mediated matrix device
>   * @kvm: reference to KVM instance
>   *
> - * Return: 0 if no other mediated matrix device has a reference to @kvm;
> - * otherwise, returns an -EPERM.
> + * Returns: 0 if the reference to kvm is successfully retrieved from @kvm_file
> + * and set into @matrix_mdev; otherwise, returns:
> + *	-ENOENT if a reference to kvm could not be retrieved from @kvm_file
> + *	-EPERM if another mediated matrix device already has a reference to the same kvm instance
>   *
>   */
>  static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
> -				struct kvm *kvm)
> +				struct file *kvm_file)
>  {
> +	struct file *kvm_file_ref __free(fput) = NULL;
>  	struct ap_matrix_mdev *m;
> +	struct kvm *kvm;
> +
> +	kvm_file_ref = get_file_active(&kvm_file);

[Severity: Critical]
Because kvm_file is passed by value, &kvm_file is a stack address. Doesn't
this bypass the RCU validation in get_file_active()? Since it checks the
address on the stack, the RCU validation will always succeed even if the
underlying struct file was reallocated.

> +	if (!kvm_file_ref)
> +		return -ENOENT;
> +
> +	kvm = kvm_file->private_data;
> +	if (!kvm)
> +		return -ENOENT;
>  
>  	if (kvm->arch.crypto.crycbd) {
>  		down_write(&kvm->arch.crypto.pqap_hook_rwsem);
[ ... ]
> @@ -1837,13 +1851,13 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
>  			}
>  		}
>  
> -		kvm_get_kvm(kvm);
>  		matrix_mdev->kvm = kvm;
>  		vfio_ap_mdev_update_guest_apcb(matrix_mdev);
>  
>  		release_update_locks_for_kvm(kvm);
>  	}
>  
> +	no_free_ptr(kvm_file_ref);
>  	return 0;
>  }

[Severity: High]
Does this unconditionally leak the file reference if kvm->arch.crypto.crycbd
is false?

In the false case, matrix_mdev->kvm is not set, but no_free_ptr(kvm_file_ref)
is still called, which inhibits the __free(fput) cleanup. Later, when the
device is released, vfio_ap_mdev_unset_kvm() will see a NULL matrix_mdev->kvm
and do nothing, permanently leaking the struct file reference.

> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index ed538aebb0b8e..8fad1ca1189f8 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
[ ... ]
> @@ -448,35 +448,13 @@ void vfio_unregister_group_dev(struct vfio_device *device)
>  EXPORT_SYMBOL_GPL(vfio_unregister_group_dev);
>  
>  #if IS_ENABLED(CONFIG_KVM)
> -void vfio_device_get_kvm_safe(struct vfio_device *device, struct kvm *kvm)
> +void vfio_device_get_kvm_safe(struct vfio_device *device, struct file *kvm)
>  {
> -	void (*pfn)(struct kvm *kvm);
> -	bool (*fn)(struct kvm *kvm);
> -	bool ret;
> -
>  	lockdep_assert_held(&device->dev_set->lock);
>  
> -	if (!kvm)
> -		return;
> -
> -	pfn = symbol_get(kvm_put_kvm);
> -	if (WARN_ON(!pfn))
> +	if (!get_file_active(&kvm))
>  		return;

[Severity: Critical]
Does this usage of get_file_active() defeat its RCU reallocation safety
checks? Similar to the vfio_ap_mdev_set_kvm() case above, kvm is a stack
variable here, so get_file_active() is checking a stack address rather than
the original pointer location.

If the struct file is reallocated (which can happen because the weak back
reference kvm->file can outlive the file descriptor, as noted below),
couldn't this increment the f_count of an arbitrarily reallocated file and
lead to type confusion?

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 45e784462ec6e..994cb40cf2efe 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
[ ... ]
> @@ -4802,7 +4802,7 @@ void kvm_unregister_device_ops(u32 type)
>  		kvm_device_ops_table[type] = NULL;
>  }
>  
> -static int kvm_ioctl_create_device(struct kvm *kvm,
> +static int kvm_ioctl_create_device(struct file *file, struct kvm *kvm,
>  				   struct kvm_create_device *cd)
>  {
>  	const struct kvm_device_ops *ops;

[Severity: Low]
Is the new file parameter intended to be used in this function? It looks
like the function body still relies on kvm_get_kvm(kvm) and ignores this
argument.

[ ... ]
> @@ -5508,6 +5517,10 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
>  	 * cases it will be called by the final fput(file) and will take
>  	 * care of doing kvm_put_kvm(kvm).
>  	 */
> +
> +	/* Store back-reference for VFIO and other subsystems */
> +	kvm->file = file;
> +
>  	kvm_uevent_notify_change(KVM_EVENT_CREATE_VM, kvm);
>  
>  	fd_install(fd, file);

[Severity: Critical]
Could this lead to a use-after-free or type confusion?

When userspace closes the KVM VM fd, the file is destroyed and freed by RCU.
However, the kvm object remains alive if kvm_vfio holds a reference to it
(since kvm_vfio_file_add passes dev->kvm->file to VFIO). This would leave
kvm->file as a dangling pointer.

If a VFIO operation later calls vfio_device_get_kvm_safe() on that dangling
pointer, it might operate on a reallocated struct file.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1