Re: [PATCH v5 2/5] iommufd/device: Associate KVM file pointer with iommufd_device

[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:
> - [Critical] Use-After-Free / Lifetime Mismatch due to borrowed KVM file reference
> --
>
> commit e5661ddf7c17ac12b18e45c9b4e38b80511ba1c0
> Author: Shameer Kolothum <skolothumtho@nvidia.com>
>
> iommufd/device: Associate KVM file pointer with iommufd_device
>     
> This commit extends iommufd_device_bind() to accept the device's KVM file
> and stores it in the iommufd_device struct. This provides TSM vDevice support
> access to the KVM associated with a VFIO device.
>
>> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
>> index 170a7005f0bc8..718abdc0e627a 100644
>> --- a/drivers/iommu/iommufd/device.c
>> +++ b/drivers/iommu/iommufd/device.c
>> @@ -216,7 +217,9 @@ void iommufd_device_destroy(struct iommufd_object *obj)
>>   * The caller must undo this with iommufd_device_unbind()
>>   */
>>  struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx,
>> -					   struct device *dev, u32 *id)
>> +					   struct device *dev,
>> +					   struct file *kvm_file,
>> +					   u32 *id)
>>  {
>>  	struct iommufd_device *idev;
>>  	struct iommufd_group *igroup;
>
> [ ... ]
>
>> @@ -266,6 +269,8 @@ struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx,
>>  	if (!iommufd_selftest_is_mock_dev(dev))
>>  		iommufd_ctx_get(ictx);
>>  	idev->dev = dev;
>> +	/* reference is already taken in vfio_df_ioctl_bind_iommufd() */
>> +	idev->kvm_file = kvm_file;
>
> [Severity: Critical]
> Does this borrowed reference create a potential use-after-free regression if
> the iommufd_device outlives the VFIO device?
>
> If a concurrent ioctl on the iommufd device is maliciously stalled (for
> example, using userfaultfd during a copy_to_user in IOMMU_GET_HW_INFO),
> iommufd_object_remove() can time out after 60 seconds and return -EBUSY.
> This would fail to remove the idev object, leaving it alive in the iommufd
> xarray.
>
> VFIO owns the KVM file reference and unconditionally drops it when the VFIO
> device file is closed. If VFIO drops this reference while idev remains alive,
> the file could be freed, leaving idev->kvm_file as a dangling pointer.
>
> If the stalled ioctl is subsequently allowed to finish, a new
> IOMMU_VIOMMU_ALLOC ioctl could look up the leaked idev and attempt to call
> get_file(idev->kvm_file), resulting in a use-after-free on the freed struct
> file.
>
> Should this function take its own independent reference to kvm_file to
> ensure the file remains valid for the entire lifetime of the iommufd_device?
>

I don’t think we need to handle this because closing a VFIO cdev will
result in an iommufd unbind. That is, we should not find an idev that
outlives a VFIO cdev.

Shouldn’t iommufd_object_remove() failing to remove an object be
considered a bug?

-aneesh