[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a39dcdcb-e904-a09d-f553-66bac5257205@linux.intel.com>
Date: Mon, 18 Sep 2023 13:49:42 +0800
From: Baolu Lu <baolu.lu@...ux.intel.com>
To: Robin Murphy <robin.murphy@....com>, joro@...tes.org,
will@...nel.org
Cc: baolu.lu@...ux.intel.com, iommu@...ts.linux.dev,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
jgg@...dia.com
Subject: Re: [PATCH v3 3/7] iommu: Validate that devices match domains
On 9/16/23 12:58 AM, Robin Murphy wrote:
> Before we can allow drivers to coexist, we need to make sure that one
> driver's domain ops can't misinterpret another driver's dev_iommu_priv
> data. To that end, add a token to the domain so we can remember how it
> was allocated - for now this may as well be the device ops, since they
> still correlate 1:1 with drivers. We can trust ourselves for internal
> default domain attachment, so add the check where it covers both the
> external attach interfaces.
>
> Reviewed-by: Lu Baolu <baolu.lu@...ux.intel.com>
> Reviewed-by: Jason Gunthorpe <jgg@...dia.com>
> Signed-off-by: Robin Murphy <robin.murphy@....com>
> ---
> drivers/iommu/iommu.c | 13 +++++++++----
> include/linux/iommu.h | 1 +
> 2 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 2f29ee9dea64..f4cc91227b22 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2000,26 +2000,28 @@ EXPORT_SYMBOL_GPL(iommu_set_fault_handler);
> static struct iommu_domain *__iommu_domain_alloc(const struct bus_type *bus,
> unsigned type)
> {
> + const struct iommu_ops *ops = bus ? bus->iommu_ops : NULL;
> struct iommu_domain *domain;
> unsigned int alloc_type = type & IOMMU_DOMAIN_ALLOC_FLAGS;
>
> - if (bus == NULL || bus->iommu_ops == NULL)
> + if (!ops)
> return NULL;
>
> - domain = bus->iommu_ops->domain_alloc(alloc_type);
> + domain = ops->domain_alloc(alloc_type);
> if (!domain)
> return NULL;
>
> domain->type = type;
> + domain->owner = ops;
> /*
> * If not already set, assume all sizes by default; the driver
> * may override this later
> */
> if (!domain->pgsize_bitmap)
> - domain->pgsize_bitmap = bus->iommu_ops->pgsize_bitmap;
> + domain->pgsize_bitmap = ops->pgsize_bitmap;
>
> if (!domain->ops)
> - domain->ops = bus->iommu_ops->default_domain_ops;
> + domain->ops = ops->default_domain_ops;
>
> if (iommu_is_dma_domain(domain) && iommu_get_dma_cookie(domain)) {
> iommu_domain_free(domain);
> @@ -2176,6 +2178,9 @@ static int __iommu_attach_group(struct iommu_domain *domain,
> group->domain != group->blocking_domain)
> return -EBUSY;
>
> + if (dev_iommu_ops(iommu_group_first_dev(group)) != domain->owner)
> + return -EINVAL;
Should we apply this check in iommu_attach_device_pasid()?
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 3bfc56df4f78..43acf1b8ed56 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3414,6 +3414,9 @@ int iommu_attach_device_pasid(struct iommu_domain
*domain,
if (!group)
return -ENODEV;
+ if (dev_iommu_ops(dev) != domain->owner)
+ return -EINVAL;
+
mutex_lock(&group->mutex);
curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain,
GFP_KERNEL);
if (curr) {
> +
> return __iommu_group_set_domain(group, domain);
> }
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index a249e10c8e9f..75ffcac199e3 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -95,6 +95,7 @@ struct iommu_domain_geometry {
> struct iommu_domain {
> unsigned type;
> const struct iommu_domain_ops *ops;
> + const struct iommu_ops *owner; /* Whose domain_alloc we came from */
> unsigned long pgsize_bitmap; /* Bitmap of page sizes in use */
> struct iommu_domain_geometry geometry;
> struct iommu_dma_cookie *iova_cookie;
Best regards,
baolu
Powered by blists - more mailing lists