linux-kernel - Re: [PATCH RFC] iommu: fix wrong DMA ops for iommu device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <91d15d11-ec53-4ca2-a385-9fa69d861f2c@quicinc.com>
Date: Thu, 2 Jan 2025 16:09:46 +0530
From: Charan Teja Kalla <quic_charante@...cinc.com>
To: <joro@...tes.org>, <will@...nel.org>, <robin.murphy@....com>,
        <jgg@...pe.ca>
CC: <iommu@...ts.linux.dev>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC] iommu: fix wrong DMA ops for iommu device

Just a quick ask if you have any comments for the below. BTW, in
internal tests, when we revert the commit f188056352bc ("iommu: Avoid
locking/unlocking for iommu_probe_device()", it is helping.

I can share more details If the below is not sufficient.

Thanks in advance...

On 12/13/2024 8:34 PM, Charan Teja Kalla wrote:
> Race between smmu device probe and iommu device probe is resulting into
> wrong DMA ops used for the device.
> 
> bus_iommu_probe(P1)                  of_iommu_configure(P2)
> (from iommu_device_register)        (from insmod of a driver)
> --------------------------         --------------------------
> 1) probe_iommu_group()
> (Fills just dev->iommu_group
> under iommu_probe_device_lock)
> 				2) iommu_probe_device():
> 				  (acquires the iommu_probe_device_lock,
> 				   but since dev->iommu_group is already
> 				   set, it just returns without
> 				   allocating the domain.)
> 
> 				3) of_dma_configure_id()->
> 				   arch_setup_dma_ops() gets called for
> 				   this device, but returns with the
> 				   error message:
> 				   "Failed to set up IOMMU for device;
> 				    retaining platform DMA ops"
> 
> 				4) device probe is succeeded where it
> 				   can now setup iommu mappings using
> 				   wrong ->dma_ops
> 
> 5) domain will be allocated later
> and fills iommu_group->domain.
> 
> Step 3) and 4) denies iommu device from using 'iommu_dma_ops'.
> 
> This issue does exists on 6.6 because of commit f188056352bc ("iommu:
> Avoid locking/unlocking for iommu_probe_device()") without which 2)
> returns only after filling the domain under group->mutex, thus no issue.
> 
> With commit b5c58b2fdc42("dma-mapping: direct calls for dma-iommu"),
> ->iommu_dma_ops is removed altogether and iommu api are directly being
> called under "use_dma_iommu(): return dev->dma_iommu". Again,
> ->dma_iommu flag is set only after domain allocation. So, If
> there is a sufficient delay between steps 4) and 5), issue is still
> exists but that seems very narrow to me.
> 
> I am thinking, setup of the domain before returning from step2) is the
> way to fix this issue. Can you please help if you have any suggestions?
> 
> Signed-off-by: Charan Teja Kalla <quic_charante@...cinc.com>
> ---
>  drivers/iommu/iommu.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 3f1029c0825e..61fc8145f378 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -561,19 +561,43 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
>  int iommu_probe_device(struct device *dev)
>  {
>  	const struct iommu_ops *ops;
> +	struct iommu_group *group;
>  	int ret;
>  
>  	mutex_lock(&iommu_probe_device_lock);
>  	ret = __iommu_probe_device(dev, NULL);
>  	mutex_unlock(&iommu_probe_device_lock);
>  	if (ret)
> -		return ret;
> +		goto err_out;
> +
> +	group = iommu_group_get(dev);
> +	if (!group) {
> +		ret = -ENODEV;
> +		goto err_release;
> +	}
> +
> +	mutex_lock(&group->mutex);
> +	if (!group->default_domain) {
> +		ret = iommu_setup_default_domain(group, 0);
> +		if (ret)
> +			goto err_unlock;
> +	}
> +	mutex_unlock(&group->mutex);
> +	iommu_group_put(group);
>  
>  	ops = dev_iommu_ops(dev);
>  	if (ops->probe_finalize)
>  		ops->probe_finalize(dev);
>  
>  	return 0;
> +
> +err_unlock:
> +	mutex_unlock(&group->mutex);
> +	iommu_group_put(group);
> +err_release:
> +	iommu_release_device(dev);
> +err_out:
> +	return ret;
>  }
>  
>  static void __iommu_group_free_device(struct iommu_group *group,