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

[sashiko-bot@kernel.org]

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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525154816.1029642-1-aneesh.kumar@kernel.org?part=2