1. Patchew
  2. linux
  3. [PATCH v3 0/7] Iommu: Retire bus ops
  4. View patch

[PATCH v3 3/7] iommu: Validate that devices match domains

Robin Murphy posted 7 patches 2 years, 4 months ago
[PATCH v3 3/7] iommu: Validate that devices match domains
Posted by Robin Murphy 2 years, 4 months ago 
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@linux.intel.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.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;
+
 	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;
-- 
2.39.2.101.g768bb238c484.dirty
Re: [PATCH v3 3/7] iommu: Validate that devices match domains
Posted by Baolu Lu 2 years, 4 months ago 
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@linux.intel.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.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
Re: [PATCH v3 3/7] iommu: Validate that devices match domains
Posted by Robin Murphy 2 years, 4 months ago 
On 2023-09-18 06:49, Baolu Lu wrote:
> 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@linux.intel.com>
>> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.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()?

Oh, probably - I don't think I've really noticed that we've grown yet 
another attach interface since I started this. Looks like current users 
are passing appropriate domains by construction, but I concur that that 
path wants covering as well for completeness.

Thanks,
Robin.

> 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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.