> From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Sent: Friday, March 24, 2023 11:03 PM
>
> 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
>
iommufd_object_destroy_user() decrements the count twice if the value
is two:
refcount_dec(&obj->users);
if (!refcount_dec_if_one(&obj->users)) {
