Re: [PATCH v7 07/16] 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:
- [High] iommufd: UAF of kvm_file due to iommufd_device outliving VFIO unbind
--

commit ddde31c06624779bad69fb77ae7b9503b553dacc
Author: Shameer Kolothum <skolothumtho@nvidia.com>

iommufd/device: Associate KVM file pointer with iommufd_device

TSM vDevice support needs access to the KVM associated with a VFIO device
after the device has been bound to iommufd. Extend iommufd_device_bind() to
accept the device's KVM file and store it in the iommufd_device.

> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index a664c70a6fe73..f99f7bee2e774 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
[ ... ]
> @@ -308,7 +309,9 @@ static int iommufd_bind_noiommu(struct iommufd_device *idev)
>   * 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;
>  	int rc;
> @@ -319,6 +322,8 @@ struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx,
>  
>  	idev->ictx = ictx;
>  	idev->dev = dev;
> +	/* VFIO holds the file reference while the device is open. */
> +	idev->kvm_file = kvm_file;

[Severity: High]
Does this code introduce a use-after-free regression for kvm_file when a
concurrent ioctl is accessing the device?

If an untrusted user thread executes an ioctl that calls iommufd_get_device(),
it will increment idev->obj.users to 2.

If another thread concurrently closes the VFIO device fd, it calls
vfio_df_unbind_iommufd() which leads to iommufd_device_unbind() and then
iommufd_object_destroy_user().

Because users is 2, iommufd_object_remove() will return -EBUSY:

drivers/iommu/iommufd/main.c:iommufd_object_remove() {
        ...
        if (!refcount_dec_if_one(&obj->users)) {
                ret = -EBUSY;
                goto err_xa;
        }
        ...
}

The unbind operation ignores this error, leaving the idev alive:

drivers/iommu/iommufd/iommufd_private.h:iommufd_object_destroy_user() {
        ...
        ret = iommufd_object_remove(ictx, obj, obj->id, REMOVE_WAIT);
        WARN_ON(ret);
}

The VFIO device then drops its KVM reference during the close process, freeing
the kvm_file. When the first thread resumes, will it access the freed kvm_file
stored here?

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