Re: [PATCH v7 07/16] 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:
> - [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?
>

This is the VFIO device fd close stack:

  vfio_device_fops_release()
    vfio_df_unbind_iommufd()
      vfio_df_close()
        vfio_df_device_last_close()
          vfio_df_iommufd_unbind()
            vfio_iommufd_physical_unbind()
              iommufd_device_unbind()
                iommufd_object_destroy_user()
                  iommufd_object_remove(..., REMOVE_WAIT)
                    iommufd_object_dec_wait() /* timeout: -EBUSY */
                  WARN_ON(ret)
      vfio_device_put_kvm()
        fput(device->kvm)

IIUC, this is a real issue: in this case, we neither remove the idev
object from the xarray nor invoke its destroy callback. Is that correct?


After the REMOVE_WAIT timeout, unbind returns without removing the
device from the xarray. VFIO close then continues, but the final ioctl
put by the thread that called iommufd_get_device() only decrements
users and wait_cnt; it does not retry destruction. The surviving idev
also holds an iommufd file reference, so closing the userspace iommufd
descriptor cannot complete the cleanup.

-aneesh