Re: [PATCH AUTOSEL 6.18] drm/msm: detach the ARM DMA mapping before attaching our own domain

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm: NULL dereference in __free_iova() via msm_iommu_new() detach
--

commit 8a10d4b676f6bf2e4f498788e23c54cc872ffbc5
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/msm: detach the ARM DMA mapping before attaching our own domain

This commit detaches and releases the default ARM32 DMA IOMMU mapping in
msm_iommu_new() before attaching the driver's own IOMMU domain so that
__iommu_attach_group() does not fail with -EBUSY on ARM32 platforms with
CONFIG_ARM_DMA_USE_IOMMU enabled.

Link: https://lore.kernel.org/r/20260730-fix-qcom-smmu-v2-3-18e0daf2d836@oss.qualcomm.com

> diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
> index 82a172f21a3ec..41a4eaa1b8fd9 100644
> --- a/drivers/gpu/drm/msm/msm_iommu.c
> +++ b/drivers/gpu/drm/msm/msm_iommu.c

[ ... ]

> @@ -746,6 +755,19 @@ struct msm_mmu *msm_iommu_new(struct device *dev, unsigned long quirks)
>  
>  	mutex_init(&iommu->init_lock);
>  
> +	/*
> +	 * ARM32 attaches a DMA mapping domain to every IOMMU-backed device,
> +	 * which would make attaching our own domain fail with -EBUSY.
> +	 */
> +	if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) {
> +		struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
> +
> +		if (mapping) {
> +			arm_iommu_detach_device(dev);
> +			arm_iommu_release_mapping(mapping);
> +		}
> +	}

[Severity: High]
Could backporting this patch without the rest of its upstream series cause a
NULL pointer dereference in __free_iova() when probing the GPU or display
controller on APQ8064?

Upstream commit 140b13475302 ("drm/msm: detach the ARM DMA mapping before
attaching our own domain") is patch 3/3 of the series at
20260730-fix-qcom-smmu-v2-3-18e0daf2d836@oss.qualcomm.com, alongside two
prerequisite fixes in drivers/iommu/msm_iommu.c:

  1/3: "iommu/msm: track a context master per device and IOMMU"
  2/3: "iommu/msm: use the IOMMU device for page table allocation"

Neither of those prerequisite patches is present in this tree.

During initial DMA setup in arm_setup_iommu_dma_ops(),
__arm_iommu_attach_device(dev, mapping) runs before set_dma_ops(dev,
&iommu_ops). Without patch 2/3, msm_iommu_attach_dev() and
msm_iommu_domain_config() set priv->cfg.iommu_dev to the client device (dev)
instead of the IOMMU hardware device:

drivers/iommu/msm_iommu.c:msm_iommu_domain_config() {
	priv->cfg = (struct io_pgtable_cfg) {
		.pgsize_bitmap = priv->domain.pgsize_bitmap,
		.ias = 32,
		.oas = 32,
		.tlb = &msm_iommu_flush_ops,
		.iommu_dev = priv->dev,
	};
}

When alloc_io_pgtable_ops() allocates the L1 page table in
__arm_v7s_alloc_table(), dev->dma_ops is still NULL, so dma_map_single() uses
direct DMA and returns the physical RAM address virt_to_phys(data->pgd)
without allocating an IOVA in mapping. Afterward, arm_setup_iommu_dma_ops()
sets dev->dma_ops = &iommu_ops.

When msm_iommu_new() now calls arm_iommu_detach_device(dev),
iommu_detach_device() runs while dev->dma_ops is still &iommu_ops:

arch/arm/mm/dma-mapping.c:arm_iommu_detach_device() {
	...
	iommu_detach_device(mapping->domain, dev);
	kref_put(&mapping->kref, release_iommu_mapping);
	to_dma_iommu_mapping(dev) = NULL;
	set_dma_ops(dev, NULL);
}

This triggers the following call chain:

  iommu_detach_device(mapping->domain, dev)
    -> msm_iommu_identity_attach()
      -> free_io_pgtable_ops(priv->iop)
        -> arm_v7s_free_pgtable()
          -> __arm_v7s_free_table()
            -> dma_unmap_single(cfg->iommu_dev, __arm_v7s_dma_addr(table), ...)
              -> arm_iommu_unmap_page()
                -> __free_iova(mapping, iova, len)

Because cfg->iommu_dev is the client device and dev->dma_ops is still
&iommu_ops, arm_iommu_unmap_page() treats the page table's physical RAM
address (for example >= 0x80000000 on APQ8064) as an IOVA handle in
mapping->domain, hits WARN_ON(!ARM_V7S_PTE_IS_VALID(pte[i])) in iommu_unmap(),
and passes that address to __free_iova():

arch/arm/mm/dma-mapping.c:__free_iova() {
	...
	spin_lock_irqsave(&mapping->lock, flags);
	bitmap_clear(mapping->bitmaps[bitmap_index], start, count);
	spin_unlock_irqrestore(&mapping->lock, flags);
}

Since arm_iommu_create_mapping() initializes mapping with base = 0,
mapping_size = 128 MB, extensions = 32, and nr_bitmaps = 1 (leaving
mapping->bitmaps[1..31] NULL), a physical address >= 0x80000000 yields
bitmap_index >= 16 and dereferences a NULL mapping->bitmaps[bitmap_index] (or
hits BUG_ON(bitmap_index >= mapping->nr_bitmaps) if out of range).

In addition, this tree includes commit da33e87bd2bf ("iommu: Handle yet
another race around registration") without patch 1/3 ("iommu/msm: track a
context master per device and IOMMU"), so find_iommu_for_dev() and
msm_iommu_attach_dev() call list_first_entry(&iommu->ctx_list, ...) on IOMMU
instances whose ctx_list is still empty, and insert_iommu_master() does not
track per-IOMMU context masters for devices attached to two IOMMU instances
(gfx3d/gfx3d1 for the GPU and mdp_port0/mdp_port1 for MDP4).

Should patches 1/3 and 2/3 from the same series also be backported before
this change?

> +
>  	ret = iommu_attach_device(iommu->domain, dev);
>  	if (ret) {
>  		iommu_domain_free(domain);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019135441.1420968-276-sashal@kernel.org?part=1