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

From: "Tian, Kevin" <kevin.tian@intel.com>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: "iommu@lists.linux.dev" <iommu@lists.linux.dev>,
	"linux-kselftest@vger.kernel.org"
	<linux-kselftest@vger.kernel.org>,
	Lu Baolu <baolu.lu@linux.intel.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	Nicolin Chen <nicolinc@nvidia.com>,
	"Liu, Yi L" <yi.l.liu@intel.com>
Subject: RE: [PATCH v3 03/17] iommufd: Replace the hwpt->devices list with iommufd_group
Date: Fri, 24 Mar 2023 01:37:51 +0000	[thread overview]
Message-ID: <BN9PR11MB52767DA03C240F040929A2398C849@BN9PR11MB5276.namprd11.prod.outlook.com> (raw)
In-Reply-To: <ZBxg9cRIpsozB15G@nvidia.com>

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, March 23, 2023 10:24 PM
> 
> On Thu, Mar 23, 2023 at 07:21:42AM +0000, Tian, Kevin wrote:
> > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > Sent: Wednesday, March 22, 2023 3:15 AM
> > >
> > >  	/*
> > > -	 * FIXME: Hack around missing a device-centric iommu api, only
> > > attach to
> > > -	 * the group once for the first device that is in the group.
> > > +	 * Only attach to the group once for the first device that is in the
> > > +	 * group. All the other devices will follow this attachment. The user
> > > +	 * should attach every device individually to as the per-device
> > > reserved
> >
> > "individually to the hwpt"
> 
> Done
> 
> > I thought about whether holding igroup->lock is necessary here.
> >
> > The caller should avoid racing attach/detach/replace on the same device.
> 
> I think even if the caller races we should be fine

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.

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

From past discussion we assumed the calling driver i.e. vfio should do
the right thing e.g. by holding dev_set->lock otherwise itself is already
messed.

igroup->lock is really for protection cross devices in the group. But as
pointed out below we can narrow its scope in this function as another
device cannot detach from this hwpt w/o first attaching to it which is
already protected by ioas->mutex.

> 
> The point of the lock scope was the capture these lines:
> 
> 	rc = iopt_table_add_domain(&hwpt->ioas->iopt, hwpt->domain);
> 	if (rc)
> 		goto out_detach;
> 	list_add_tail(&hwpt->hwpt_item, &hwpt->ioas->hwpt_list);
> 
> But based on the current arrangement none of them rely on the igroup
> mutex so it does seem we can narrow it
> 