linux-kernel - Re: [PATCH] iommu: Clear IOMMU DMA ops when detaching a device from its IOMMU group

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: <a0a9861e-699e-4bb9-9e30-09d70cb83644@linux.intel.com>
Date: Fri, 11 Apr 2025 09:30:34 +0800
From: Baolu Lu <baolu.lu@...ux.intel.com>
To: Chen-Yu Tsai <wenst@...omium.org>, Robin Murphy <robin.murphy@....com>,
 Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>
Cc: iommu@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iommu: Clear IOMMU DMA ops when detaching a device from
 its IOMMU group

On 4/10/25 18:09, Chen-Yu Tsai wrote:
> In recent changes to the IOMMU subsystem seen in v6.15-rc1, the process
> and order of how IOMMU linked devices are brought up has changed. Now
> when the IOMMU is probed, the IOMMU subsystem core will attempt to link
> or attach devices described in the device tree as using the IOMMU to
> the IOMMU's associated group or domain. If any part of this fails, the
> whole process is (mostly) undone. The IOMMU failure is then treated as
> a deferred probe; after sufficient time, the deferred probe times out
> and the attached devices are brought out without the IOMMU.
> 
> In the process of undoing changes, one part was missed. When a device
> is initialized for use with the IOMMU, iommu_setup_dma_ops() is also
> called, which signals that DMA operations should go through the IOMMU.
> This part was not reverted when the IOMMU changes were undone. When
> the device later probes without the IOMMU, DMA operations would attempt
> to use IOMMU operations with no IOMMU domain present, causing a crash.
> 
> The above was observed on an MT8188 Chromebook. The MT8188 device tree
> had incorrectly described the IOMMU endpoint for part of its display
> pipeline; the IOMMU driver would successfully attach a couple hardware
> blocks, fail at the incorrectly described one, and roll back all
> changes. Later when the deferred probe times out, the display pipleine
> probes without the IOMMU, but when a framebuffer is allocated, it goes
> through the IOMMU DMA ops, causing a NULL pointer dereference crash.
> 
> Add a helper that is the opposite of iommu_setup_dma_ops(), and call it
> when the IOMMU-enabled device is being detached from its IOMMU.
> 
> Closes:https://lore.kernel.org/all/CAGXv+5HJpTYmQ2h- 
> GD7GjyeYT7bL9EBCvu0mz5LgpzJZtzfW0w@...l.gmail.com/
> Signed-off-by: Chen-Yu Tsai<wenst@...omium.org>
> ---
> This patch should be applied for v6.15. It's not immediately clear to
> me which commit is the actual cause, so I did not add a Fixes tag.

I guess it might be:

Fixes: bcb81ac6ae3c ("iommu: Get DT/ACPI parsing into the proper probe 
path")

Or, something near that. Please verify before apply.

> ---
>   drivers/iommu/dma-iommu.c | 5 +++++
>   drivers/iommu/dma-iommu.h | 5 +++++
>   drivers/iommu/iommu.c     | 2 ++
>   3 files changed, 12 insertions(+)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index cb7e29dcac15..62a51d84ffe1 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -1739,6 +1739,11 @@ void iommu_setup_dma_ops(struct device *dev)
>   	dev->dma_iommu = false;
>   }
>   
> +void iommu_clear_dma_ops(struct device *dev)
> +{
> +	dev->dma_iommu = false;
> +}
> +
>   static bool has_msi_cookie(const struct iommu_domain *domain)
>   {
>   	return domain && (domain->cookie_type == IOMMU_COOKIE_DMA_IOVA ||
> diff --git a/drivers/iommu/dma-iommu.h b/drivers/iommu/dma-iommu.h
> index eca201c1f963..dfd31cb9e685 100644
> --- a/drivers/iommu/dma-iommu.h
> +++ b/drivers/iommu/dma-iommu.h
> @@ -10,6 +10,7 @@
>   #ifdef CONFIG_IOMMU_DMA
>   
>   void iommu_setup_dma_ops(struct device *dev);
> +void iommu_clear_dma_ops(struct device *dev);
>   
>   int iommu_get_dma_cookie(struct iommu_domain *domain);
>   void iommu_put_dma_cookie(struct iommu_domain *domain);
> @@ -30,6 +31,10 @@ static inline void iommu_setup_dma_ops(struct device *dev)
>   {
>   }
>   
> +static inline void iommu_clear_dma_ops(struct device *dev)
> +{
> +}
> +
>   static inline int iommu_dma_init_fq(struct iommu_domain *domain)
>   {
>   	return -EINVAL;
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index c8033ca66377..498f8f48394c 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -677,6 +677,8 @@ static void __iommu_group_remove_device(struct device *dev)
>   	struct iommu_group *group = dev->iommu_group;
>   	struct group_device *device;
>   
> +	iommu_clear_dma_ops(dev);
> +
>   	mutex_lock(&group->mutex);
>   	for_each_group_device(group, device) {
>   		if (device->dev != dev)

I don't think there needs to be an inline helper for this one-time call.
Perhaps you can simply do it like this:

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index c8033ca66377..cd9d7a0fcaa0 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -536,6 +536,7 @@ static void iommu_deinit_device(struct device *dev)

         /* Caller must put iommu_group */
         dev->iommu_group = NULL;
+       dev->dma_iommu = false;
         module_put(ops->owner);
         dev_iommu_free(dev);
  }

?

Thanks,
baolu