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, onlyattach 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-devicereserved
"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