Re: [PATCH 03/10] iommu: Add generic_single_device_group()

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Jul 20, 2023 at 03:39:27PM +0800, Baolu Lu wrote:
> On 2023/7/19 3:05, Jason Gunthorpe wrote:
> > This implements the common pattern seen in drivers of a single
> > iommu_group for the entire iommu driver. Implement this in core code
> > so the drivers that want this can select it from their ops.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> >   drivers/iommu/iommu.c | 25 +++++++++++++++++++++++++
> >   include/linux/iommu.h |  3 +++
> >   2 files changed, 28 insertions(+)
> > 
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 9e41ad4e3219b6..1e0c5d9a0370fb 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -289,6 +289,9 @@ void iommu_device_unregister(struct iommu_device *iommu)
> >   	spin_lock(&iommu_device_lock);
> >   	list_del(&iommu->list);
> >   	spin_unlock(&iommu_device_lock);
> > +
> > +	/* Pairs with the alloc in generic_single_device_group() */
> > +	iommu_group_put(iommu->singleton_group);
> >   }
> >   EXPORT_SYMBOL_GPL(iommu_device_unregister);
> > @@ -1595,6 +1598,28 @@ struct iommu_group *generic_device_group(struct device *dev)
> >   }
> >   EXPORT_SYMBOL_GPL(generic_device_group);
> > +/*
> > + * Generic device_group call-back function. It just allocates one
> > + * iommu-group per iommu driver.
> > + */
> > +struct iommu_group *generic_single_device_group(struct device *dev)
> > +{
> > +	struct iommu_device *iommu = dev->iommu->iommu_dev;
> > +
> > +	lockdep_assert_held(&dev_iommu_group_lock);
> > +
> > +	if (!iommu->singleton_group) {
> > +		struct iommu_group *group;
> > +
> > +		group = iommu_group_alloc();
> > +		if (IS_ERR(group))
> > +			return group;
> > +		iommu->singleton_group = group;
> > +	}
> > +	return iommu_group_ref_get(iommu->singleton_group);
> > +}
> > +EXPORT_SYMBOL_GPL(generic_single_device_group);
> 
> When allocating the singleton group for the first time, the group's
> refcount is taken twice.

Yes, that is correct.

The refcount from alloc belongs to iommu->singleton_group and the
pair'd put is here:

@@ -289,6 +289,9 @@ void iommu_device_unregister(struct iommu_device *iommu)
 	spin_lock(&iommu_device_lock);
 	list_del(&iommu->list);
 	spin_unlock(&iommu_device_lock);
+
+	/* Pairs with the alloc in generic_single_device_group() */
+	iommu_group_put(iommu->singleton_group);
 }

The refcount from iommu_group_ref_get() belongs to the caller and the
caller must have a paired put.

> struct iommu_group *generic_single_device_group(struct device *dev)
> {
>         struct iommu_device *iommu = dev->iommu->iommu_dev;
>         struct iommu_group *group;
> 
>         lockdep_assert_held(&dev_iommu_group_lock);
> 
>         if (iommu->singleton_group)
>                 return iommu_group_ref_get(iommu->singleton_group);
> 
>         group = iommu_group_alloc();
>         if (!IS_ERR(group))
>                 iommu->singleton_group = group;
> 
>         return group;

This will UAF the iommu->singleton_group, consider a caller that does:

   iommu_group_put(generic_single_device_group(dev))

Jason