> From: Jason Gunthorpe <jgg@xxxxxxxxxx> > Sent: Tuesday, March 28, 2023 7:39 PM > > On Tue, Mar 28, 2023 at 02:32:22AM +0000, Tian, Kevin wrote: > > > 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)) { > > Ohh, it isn't allowed to call iommufd_object_destroy_user() until > finalize has passed.. > ah you are right. In this case iommufd_get_object() will fail in the first place.