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

On Thu, Jul 20, 2023 at 03:39:27PM +0800, Baolu Lu wrote:

On 2023/7/19 3:05, Jason Gunthorpe wrote:

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel