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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] VFIO retains a dangling weak pointer to the KVM VM `struct file` when the VM fd is closed, leading to use-after-free and type confusion if the file memory is reallocated.
- [Medium] Direct, unsafe access to the RCU-protected pointer `kvm->file` without using RCU primitives.
--

--- Patch [1]: [PATCH v6 01/33] vfio: Use file-based reference counting for KVM ---
commit fbb73952530ea819e76b37873395b8b6ce2e0444
Author: Steffen Eiden <[email protected]>

vfio: Use file-based reference counting for KVM

This commit replaces manual module reference counting with file-based
reference counting for KVM integration. It updates the VFIO-KVM interface
to pass struct file pointers instead of struct kvm pointers, relying on
get_file_active() and fput() for reference management.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 99a0efd999ef9..875e67063d8e5 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
[ ... ]
> @@ -1822,12 +1823,25 @@ 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 kvm *kvm;
> +
> +	kvm_file_ref = get_file_active(kvm_file);
> +	if (!kvm_file_ref)
> +		return -ENOENT;
> +
> +	kvm = kvm_file_ref->private_data;
> +	if (!kvm)
> +		return -ENOENT;

[Severity: Critical]
If kvm_file_ref points to a file that was freed and reallocated to a
completely different type of file (due to SLAB_TYPESAFE_BY_RCU), could
this cause type confusion?

Since struct file memory can be reused, extracting private_data without first
verifying the file's operations (f_op) could lead to arbitrary memory
corruption. Should this code use file_is_kvm() or file_to_kvm() before
dereferencing private_data?

> diff --git a/drivers/vfio/pci/vfio_pci_zdev.c b/drivers/vfio/pci/vfio_pci_zdev.c
> index 0990fdb146b78..d3a1101013531 100644
> --- a/drivers/vfio/pci/vfio_pci_zdev.c
> +++ b/drivers/vfio/pci/vfio_pci_zdev.c
[ ... ]
> @@ -144,6 +144,7 @@ int vfio_pci_info_zdev_add_caps(struct vfio_pci_core_device *vdev,
>  int vfio_pci_zdev_open_device(struct vfio_pci_core_device *vdev)
>  {
>  	struct zpci_dev *zdev = to_zpci(vdev->pdev);
> +	struct kvm *kvm;
>  
>  	if (!zdev)
>  		return -ENODEV;
> @@ -151,8 +152,12 @@ int vfio_pci_zdev_open_device(struct vfio_pci_core_device *vdev)
>  	if (!vdev->vdev.kvm)
>  		return 0;
>  
> +	kvm = vdev->vdev.kvm->private_data;
> +	if (!kvm)
> +		return -ENOENT;

[Severity: Critical]
Does this code also need to validate that vdev->vdev.kvm is actually a KVM
file before interpreting its private_data as a struct kvm pointer?

> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index 423ead48aafe3..b06d1a9116051 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
[ ... ]
> @@ -472,36 +472,17 @@ 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;
> +	struct file *kvm_file_ref __free(fput) = NULL;
>  
>  	lockdep_assert_held(&device->dev_set->lock);
>  
> -	if (!kvm)
> +	kvm_file_ref = get_file_active(kvm);
> +	if (!kvm_file_ref)
>  		return;

[Severity: Critical]
Can this result in getting a reference to a completely unrelated file object?

If userspace opens a KVM VM and creates a KVM VFIO device (taking a reference
to the kvm object), VFIO stores a weak pointer to the KVM struct file.

If userspace then closes the KVM VM fd while keeping the KVM VFIO device fd
open:

1. The VM fd closes, dropping the f_count to 0 and freeing the struct file
   via RCU.
2. The kvm->users_count remains > 0 (due to the open KVM VFIO device fd), so
   kvm_destroy_vm() is never called to clear the VFIO weak pointer.
3. Because struct file uses SLAB_TYPESAFE_BY_RCU, the freed memory can be
   reallocated to a completely unrelated file.

When get_file_active() is later called here on the dangling pointer, wouldn't
it successfully take a reference on the reused struct file?

> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> index 6cdc4e9a333a3..0166cc4d39b31 100644
> --- a/virt/kvm/vfio.c
> +++ b/virt/kvm/vfio.c
[ ... ]
> @@ -168,7 +168,7 @@ static int kvm_vfio_file_add(struct kvm_device *dev, unsigned int fd)
>  	kvf->file = get_file(filp);
>  	list_add_tail(&kvf->node, &kv->file_list);
>  
> -	kvm_vfio_file_set_kvm(kvf->file, dev->kvm);
> +	kvm_vfio_file_set_kvm(kvf->file, dev->kvm->file);

[Severity: Medium]
Is it safe to access dev->kvm->file directly here?

Since kvm->file is explicitly annotated as a __rcu pointer in struct kvm,
shouldn't this be accessed within an RCU read-side critical section using
rcu_dereference() to avoid data races with kvm_vm_release()?

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