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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 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.




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux