Re: [PATCH 2/5] iommu: Ensure device has the same iommu_ops as the domain

[Robin Murphy]

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.

> 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 :/

[...]
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 19cf28d40ebe..8a1f437a51f2 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1963,6 +1963,10 @@ static int __iommu_attach_device(struct iommu_domain *domain,
>   {
>   	int ret;
>   
> +	/* Ensure the device was probe'd onto the same driver as the domain */
> +	if (dev->bus->iommu_ops != domain->ops->iommu_ops)

Nope, dev_iommu_ops(dev) please. Furthermore I think the logical place 
to put this is in iommu_group_do_attach_device(), since that's the 
gateway for the public interfaces - we shouldn't need to second-guess 
ourselves for internal default-domain-related calls.

Thanks,
Robin.

> +		return -EMEDIUMTYPE;
> +
>   	if (unlikely(domain->ops->attach_dev == NULL))
>   		return -ENODEV;