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

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

sashiko-bot@kernel.org writes:

> 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?
>

I guess we can fix this by

modified   drivers/vfio/device_cdev.c
@@ -115,8 +115,8 @@ long vfio_df_ioctl_bind_iommufd(struct vfio_device_file *df,
 		return ret;
 
 	mutex_lock(&device->dev_set->lock);
-	/* one device cannot be bound twice */
-	if (df->access_granted) {
+	/* The cdev path only supports one bound/open device fd. */
+	if (df->access_granted || device->open_count) {
 		ret = -EINVAL;
 		goto out_unlock;
 	}