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 checks to cover all the public attach
interfaces.
Reviewed-by: Lu Baolu <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
Reviewed-by: Jerry Snitselaar <[email protected]>
Signed-off-by: Robin Murphy <[email protected]>
---
v4: Cover iommu_attach_device_pasid() as well, and improve robustness
against theoretical attempts to attach a noiommu group.
v6: Cover new iommu_domain_alloc_user() sites as well. I don't entirely
dislike the idea of tying this into the domain ops, but I'd rather
do the simple thing for now and revisit that in future, since domain
ops also deserve some other cleanup.
---
drivers/iommu/iommu.c | 10 ++++++++++
drivers/iommu/iommufd/hw_pagetable.c | 2 ++
include/linux/iommu.h | 2 +-
3 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 7fafd073c33e..8e4436c606d3 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2117,6 +2117,7 @@ static struct iommu_domain *__iommu_domain_alloc(const struct iommu_ops *ops,
return NULL;
domain->type = type;
+ domain->owner = ops;
/*
* If not already set, assume all sizes by default; the driver
* may override this later
@@ -2282,10 +2283,16 @@ struct iommu_domain *iommu_get_dma_domain(struct device *dev)
static int __iommu_attach_group(struct iommu_domain *domain,
struct iommu_group *group)
{
+ struct device *dev;
+
if (group->domain && group->domain != group->default_domain &&
group->domain != group->blocking_domain)
return -EBUSY;
+ dev = iommu_group_first_dev(group);
+ if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner)
+ return -EINVAL;
+
return __iommu_group_set_domain(group, domain);
}
@@ -3477,6 +3484,9 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
if (!group)
return -ENODEV;
+ if (!dev_has_iommu(dev) || 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) {
diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
index 2abbeafdbd22..5be7f513b622 100644
--- a/drivers/iommu/iommufd/hw_pagetable.c
+++ b/drivers/iommu/iommufd/hw_pagetable.c
@@ -135,6 +135,7 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
hwpt->domain = NULL;
goto out_abort;
}
+ hwpt->domain->owner = ops;
} else {
hwpt->domain = iommu_domain_alloc(idev->dev->bus);
if (!hwpt->domain) {
@@ -233,6 +234,7 @@ iommufd_hwpt_nested_alloc(struct iommufd_ctx *ictx,
hwpt->domain = NULL;
goto out_abort;
}
+ hwpt->domain->owner = ops;
if (WARN_ON_ONCE(hwpt->domain->type != IOMMU_DOMAIN_NESTED)) {
rc = -EINVAL;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 7d87423663d4..51b18a4b5834 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -106,7 +106,7 @@ struct iommu_domain {
unsigned type;
const struct iommu_domain_ops *ops;
const struct iommu_dirty_ops *dirty_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
On Tue, Nov 21, 2023 at 06:03:59PM +0000, 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 checks to cover all the public attach
> interfaces.
>
> Reviewed-by: Lu Baolu <[email protected]>
> Reviewed-by: Jason Gunthorpe <[email protected]>
> Reviewed-by: Jerry Snitselaar <[email protected]>
> Signed-off-by: Robin Murphy <[email protected]>
>
> ---
>
> v4: Cover iommu_attach_device_pasid() as well, and improve robustness
> against theoretical attempts to attach a noiommu group.
> v6: Cover new iommu_domain_alloc_user() sites as well. I don't entirely
> dislike the idea of tying this into the domain ops, but I'd rather
> do the simple thing for now and revisit that in future, since domain
> ops also deserve some other cleanup.
Looks good
Jason