Re: [PATCH v3 03/17] iommufd: Replace the hwpt->devices list with iommufd_group

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Mar 24, 2023 at 01:37:51AM +0000, Tian, Kevin wrote:

> If vfio races attach/detach then lots of things are messed.
> 
> e.g. iommufd_device_detach() directly calls list_del(&idev->group_item)
> w/o checking whether the device has been attached.

Yeah, you obviously can't race attach/detach or detach/replace

> And with that race UAF could occur if we narrow down the lock scope
> to iommufd_hw_pagetable_attach():
> 
>               cpu0                                cpu1
> vfio_iommufd_attach()
>   iommufd_device_attach()
>     iommufd_device_auto_get_domain()
>       mutex_lock(&ioas->mutex);
>       iommufd_hw_pagetable_alloc()
>         hwpt = iommufd_object_alloc() //hwpt.users=1
>         hwpt->domain = iommu_domain_alloc(idev->dev->bus);
>         iommufd_hw_pagetable_attach() //hwpt.users=2
>                                           vfio_iommufd_detach()
>                                             iommufd_device_detach()
>                                               mutex_lock(&idev->igroup->lock);
>                                               hwpt = iommufd_hw_pagetable_detach()
>                                               mutex_unlock(&idev->igroup->lock);
>                                               iommufd_hw_pagetable_put(hwpt)
>                                                 iommufd_object_destroy_user(hwpt) //hwpt.users=0
>                                                   iommufd_hw_pagetable_destroy(hwpt)
>                                                     iommu_domain_free(hwpt->domain);
>         iopt_table_add_domain(&hwpt->ioas->iopt, hwpt->domain); //UAF

You didn't balance the refcounts properly, the cpu1 put will get to
hwpt.users=1

There is a refcount_inc in iommufd_hw_pagetable_attach(), the
iommufd_hw_pagetable_alloc() retains its reference and so the domain
is guarenteed valid

Jason