| Message ID | 1-v1-3c8177327a47+256-iommu_group_locking_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refine the locking for dev->iommu_group | expand |
On 2023/7/19 3:05, Jason Gunthorpe wrote: > Several functions obtain the group reference and then release it before > returning. This gives the impression that the refcount is protecting > something for the duration of the function. > > In truth all of these functions are called in places that know a device > driver is probed to the device and our locking rules already require > that dev->iommu_group cannot change while a driver is attached to the > struct device. > > If this was not the case then this code is already at risk of triggering > UAF as it is racy if the dev->iommu_group is concurrently going to > NULL/free. refcount debugging will throw a WARN if kobject_get() is > called on a 0 refcount object to highlight the bug. > > Remove the confusing refcounting and leave behind a comment about the > restriction. > > Signed-off-by: Jason Gunthorpe<jgg@nvidia.com> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com> Best regards, baolu
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Wednesday, July 19, 2023 3:06 AM > > int iommu_device_use_default_domain(struct device *dev) > { > - struct iommu_group *group = iommu_group_get(dev); > + /* Caller must be a probed driver on dev */ > + struct iommu_group *group = dev->iommu_group; > int ret = 0; this is called in the probing path by .dma_configure(). the driver hasn't been probed yet hence the rationale to not refcount is due to device_lock() instead?
On Fri, Jul 21, 2023 at 07:10:57AM +0000, Tian, Kevin wrote: > > From: Jason Gunthorpe <jgg@nvidia.com> > > Sent: Wednesday, July 19, 2023 3:06 AM > > > > int iommu_device_use_default_domain(struct device *dev) > > { > > - struct iommu_group *group = iommu_group_get(dev); > > + /* Caller must be a probed driver on dev */ > > + struct iommu_group *group = dev->iommu_group; > > int ret = 0; > > this is called in the probing path by .dma_configure(). > > the driver hasn't been probed yet hence the rationale to not > refcount is due to device_lock() instead? At this point the driver core has partially attached/detached a driver so I'm considering it part of the probed driver explanation. Really during anything protected by the driver core probe path is assured that the group cannot change. Jason
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Friday, July 21, 2023 8:02 PM > > On Fri, Jul 21, 2023 at 07:10:57AM +0000, Tian, Kevin wrote: > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > Sent: Wednesday, July 19, 2023 3:06 AM > > > > > > int iommu_device_use_default_domain(struct device *dev) > > > { > > > - struct iommu_group *group = iommu_group_get(dev); > > > + /* Caller must be a probed driver on dev */ > > > + struct iommu_group *group = dev->iommu_group; > > > int ret = 0; > > > > this is called in the probing path by .dma_configure(). > > > > the driver hasn't been probed yet hence the rationale to not > > refcount is due to device_lock() instead? > > At this point the driver core has partially attached/detached a driver > so I'm considering it part of the probed driver explanation. > > Really during anything protected by the driver core probe path is > assured that the group cannot change. > Probably this worths a slightly different comment then?
On Mon, Jul 24, 2023 at 02:11:44AM +0000, Tian, Kevin wrote: > > From: Jason Gunthorpe <jgg@nvidia.com> > > Sent: Friday, July 21, 2023 8:02 PM > > > > On Fri, Jul 21, 2023 at 07:10:57AM +0000, Tian, Kevin wrote: > > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > > Sent: Wednesday, July 19, 2023 3:06 AM > > > > > > > > int iommu_device_use_default_domain(struct device *dev) > > > > { > > > > - struct iommu_group *group = iommu_group_get(dev); > > > > + /* Caller must be a probed driver on dev */ > > > > + struct iommu_group *group = dev->iommu_group; > > > > int ret = 0; > > > > > > this is called in the probing path by .dma_configure(). > > > > > > the driver hasn't been probed yet hence the rationale to not > > > refcount is due to device_lock() instead? > > > > At this point the driver core has partially attached/detached a driver > > so I'm considering it part of the probed driver explanation. > > > > Really during anything protected by the driver core probe path is > > assured that the group cannot change. > > > > Probably this worths a slightly different comment then? How about /* Caller is the driver core during the post-probe path */ Jason
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Tuesday, July 25, 2023 2:06 AM > > On Mon, Jul 24, 2023 at 02:11:44AM +0000, Tian, Kevin wrote: > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > Sent: Friday, July 21, 2023 8:02 PM > > > > > > On Fri, Jul 21, 2023 at 07:10:57AM +0000, Tian, Kevin wrote: > > > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > > > Sent: Wednesday, July 19, 2023 3:06 AM > > > > > > > > > > int iommu_device_use_default_domain(struct device *dev) > > > > > { > > > > > - struct iommu_group *group = iommu_group_get(dev); > > > > > + /* Caller must be a probed driver on dev */ > > > > > + struct iommu_group *group = dev->iommu_group; > > > > > int ret = 0; > > > > > > > > this is called in the probing path by .dma_configure(). > > > > > > > > the driver hasn't been probed yet hence the rationale to not > > > > refcount is due to device_lock() instead? > > > > > > At this point the driver core has partially attached/detached a driver > > > so I'm considering it part of the probed driver explanation. > > > > > > Really during anything protected by the driver core probe path is > > > assured that the group cannot change. > > > > > > > Probably this worths a slightly different comment then? > > How about > > /* Caller is the driver core during the post-probe path */ > sounds good
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 4352a149a935e8..2f6eb781dfc317 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -2014,10 +2014,10 @@ static int __iommu_attach_device(struct iommu_domain *domain, */ int iommu_attach_device(struct iommu_domain *domain, struct device *dev) { - struct iommu_group *group; + /* Caller must be a probed driver on dev */ + struct iommu_group *group = dev->iommu_group; int ret; - group = iommu_group_get(dev); if (!group) return -ENODEV; @@ -2034,8 +2034,6 @@ int iommu_attach_device(struct iommu_domain *domain, struct device *dev) out_unlock: mutex_unlock(&group->mutex); - iommu_group_put(group); - return ret; } EXPORT_SYMBOL_GPL(iommu_attach_device); @@ -2050,9 +2048,9 @@ int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain) void iommu_detach_device(struct iommu_domain *domain, struct device *dev) { - struct iommu_group *group; + /* Caller must be a probed driver on dev */ + struct iommu_group *group = dev->iommu_group; - group = iommu_group_get(dev); if (!group) return; @@ -2064,24 +2062,18 @@ void iommu_detach_device(struct iommu_domain *domain, struct device *dev) out_unlock: mutex_unlock(&group->mutex); - iommu_group_put(group); } EXPORT_SYMBOL_GPL(iommu_detach_device); struct iommu_domain *iommu_get_domain_for_dev(struct device *dev) { - struct iommu_domain *domain; - struct iommu_group *group; + /* Caller must be a probed driver on dev */ + struct iommu_group *group = dev->iommu_group; - group = iommu_group_get(dev); if (!group) return NULL; - domain = group->domain; - - iommu_group_put(group); - - return domain; + return group->domain; } EXPORT_SYMBOL_GPL(iommu_get_domain_for_dev); @@ -3044,7 +3036,8 @@ static bool iommu_is_default_domain(struct iommu_group *group) */ int iommu_device_use_default_domain(struct device *dev) { - struct iommu_group *group = iommu_group_get(dev); + /* Caller must be a probed driver on dev */ + struct iommu_group *group = dev->iommu_group; int ret = 0; if (!group) @@ -3063,8 +3056,6 @@ int iommu_device_use_default_domain(struct device *dev) unlock_out: mutex_unlock(&group->mutex); - iommu_group_put(group); - return ret; } @@ -3078,7 +3069,8 @@ int iommu_device_use_default_domain(struct device *dev) */ void iommu_device_unuse_default_domain(struct device *dev) { - struct iommu_group *group = iommu_group_get(dev); + /* Caller must be a probed driver on dev */ + struct iommu_group *group = dev->iommu_group; if (!group) return; @@ -3088,7 +3080,6 @@ void iommu_device_unuse_default_domain(struct device *dev) group->owner_cnt--; mutex_unlock(&group->mutex); - iommu_group_put(group); } static int __iommu_group_alloc_blocking_domain(struct iommu_group *group) @@ -3175,13 +3166,13 @@ EXPORT_SYMBOL_GPL(iommu_group_claim_dma_owner); */ int iommu_device_claim_dma_owner(struct device *dev, void *owner) { - struct iommu_group *group; + /* Caller must be a probed driver on dev */ + struct iommu_group *group = dev->iommu_group; int ret = 0; if (WARN_ON(!owner)) return -EINVAL; - group = iommu_group_get(dev); if (!group) return -ENODEV; @@ -3198,8 +3189,6 @@ int iommu_device_claim_dma_owner(struct device *dev, void *owner) ret = __iommu_take_dma_ownership(group, owner); unlock_out: mutex_unlock(&group->mutex); - iommu_group_put(group); - return ret; } EXPORT_SYMBOL_GPL(iommu_device_claim_dma_owner); @@ -3237,7 +3226,8 @@ EXPORT_SYMBOL_GPL(iommu_group_release_dma_owner); */ void iommu_device_release_dma_owner(struct device *dev) { - struct iommu_group *group = iommu_group_get(dev); + /* Caller must be a probed driver on dev */ + struct iommu_group *group = dev->iommu_group; mutex_lock(&group->mutex); if (group->owner_cnt > 1) @@ -3245,7 +3235,6 @@ void iommu_device_release_dma_owner(struct device *dev) else __iommu_release_dma_ownership(group); mutex_unlock(&group->mutex); - iommu_group_put(group); } EXPORT_SYMBOL_GPL(iommu_device_release_dma_owner); @@ -3306,14 +3295,14 @@ static void __iommu_remove_group_pasid(struct iommu_group *group, int iommu_attach_device_pasid(struct iommu_domain *domain, struct device *dev, ioasid_t pasid) { - struct iommu_group *group; + /* Caller must be a probed driver on dev */ + struct iommu_group *group = dev->iommu_group; void *curr; int ret; if (!domain->ops->set_dev_pasid) return -EOPNOTSUPP; - group = iommu_group_get(dev); if (!group) return -ENODEV; @@ -3331,8 +3320,6 @@ int iommu_attach_device_pasid(struct iommu_domain *domain, } out_unlock: mutex_unlock(&group->mutex); - iommu_group_put(group); - return ret; } EXPORT_SYMBOL_GPL(iommu_attach_device_pasid); @@ -3349,14 +3336,13 @@ EXPORT_SYMBOL_GPL(iommu_attach_device_pasid); void iommu_detach_device_pasid(struct iommu_domain *domain, struct device *dev, ioasid_t pasid) { - struct iommu_group *group = iommu_group_get(dev); + /* Caller must be a probed driver on dev */ + struct iommu_group *group = dev->iommu_group; mutex_lock(&group->mutex); __iommu_remove_group_pasid(group, pasid); WARN_ON(xa_erase(&group->pasid_array, pasid) != domain); mutex_unlock(&group->mutex); - - iommu_group_put(group); } EXPORT_SYMBOL_GPL(iommu_detach_device_pasid); @@ -3378,10 +3364,10 @@ struct iommu_domain *iommu_get_domain_for_dev_pasid(struct device *dev, ioasid_t pasid, unsigned int type) { + /* Caller must be a probed driver on dev */ + struct iommu_group *group = dev->iommu_group; struct iommu_domain *domain; - struct iommu_group *group; - group = iommu_group_get(dev); if (!group) return NULL; @@ -3390,7 +3376,6 @@ struct iommu_domain *iommu_get_domain_for_dev_pasid(struct device *dev, if (type && domain && domain->type != type) domain = ERR_PTR(-EBUSY); xa_unlock(&group->pasid_array); - iommu_group_put(group); return domain; }
Several functions obtain the group reference and then release it before returning. This gives the impression that the refcount is protecting something for the duration of the function. In truth all of these functions are called in places that know a device driver is probed to the device and our locking rules already require that dev->iommu_group cannot change while a driver is attached to the struct device. If this was not the case then this code is already at risk of triggering UAF as it is racy if the dev->iommu_group is concurrently going to NULL/free. refcount debugging will throw a WARN if kobject_get() is called on a 0 refcount object to highlight the bug. Remove the confusing refcounting and leave behind a comment about the restriction. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/iommu/iommu.c | 57 ++++++++++++++++--------------------------- 1 file changed, 21 insertions(+), 36 deletions(-)