On 2022-06-06 17:51, Nicolin Chen wrote:
Hi Robin,
On Mon, Jun 06, 2022 at 03:33:42PM +0100, Robin Murphy wrote:
On 2022-06-06 07:19, Nicolin Chen wrote:
The core code should not call an iommu driver op with a struct device
parameter unless it knows that the dev_iommu_priv_get() for that struct
device was setup by the same driver. Otherwise in a mixed driver system
the iommu_priv could be casted to the wrong type.
We don't have mixed-driver systems, and there are plenty more
significant problems than this one to solve before we can (but thanks
for pointing it out - I hadn't got as far as auditing the public
interfaces yet). Once domains are allocated via a particular device's
IOMMU instance in the first place, there will be ample opportunity for
the core to stash suitable identifying information in the domain for
itself. TBH even the current code could do it without needing the
weirdly invasive changes here.
Do you have an alternative and less invasive solution in mind?
Store the iommu_ops pointer in the iommu_domain and use it as a check to
validate that the struct device is correct before invoking any domain op
that accepts a struct device.
In fact this even describes exactly that - "Store the iommu_ops pointer
in the iommu_domain", vs. the "Store the iommu_ops pointer in the
iommu_domain_ops" which the patch is actually doing :/
Will fix that.
Well, as before I'd prefer to make the code match the commit message -
if I really need to spell it out, see below - since I can't imagine that
we should ever have need to identify a set of iommu_domain_ops in
isolation, therefore I think it's considerably clearer to use the
iommu_domain itself. However, either way we really don't need this yet,
so we may as well just go ahead and remove the redundant test from VFIO
anyway, and I can add some form of this patch to my dev branch for now.
Thanks,
Robin.
----->8-----
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index cde2e1d6ab9b..72990edc9314 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1902,6 +1902,7 @@ static struct iommu_domain
*__iommu_domain_alloc(struct device *dev,
domain->type = type;
/* Assume all sizes by default; the driver may override this later */
domain->pgsize_bitmap = ops->pgsize_bitmap;
+ domain->owner = ops;
if (!domain->ops)
domain->ops = ops->default_domain_ops;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 6f64cbbc6721..79e557207f53 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -89,6 +89,7 @@ struct iommu_domain_geometry {
struct iommu_domain {
unsigned type;
+ const struct iommu_ops *owner; /* Who allocated this domain */
const struct iommu_domain_ops *ops;
unsigned long pgsize_bitmap; /* Bitmap of page sizes in use */
iommu_fault_handler_t handler;
