Re: [PATCH 3/3] iommu/mediatek-v1: add mt6572 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Insufficient hardware synchronization for MT6572 TLB flush completion

Pre-existing issues:
- [High] NULL pointer dereference in mtk_iommu_v1_domain_free() if domain was never attached
- [High] Race condition causing use-after-disable in hardware interrupt handling during teardown
- [High] Data corruption and Use-After-Free due to missing lock in TLB flush sequence
--

commit 29773ece1b87d664d6aa369fe95265b2f2898794
Author: Roman Vivchar <rva333@protonmail.com>

iommu/mediatek-v1: add mt6572 support

This commit adds support for the mt6572 SoC, which utilizes an earlier
version of the IOMMU that shares the same v1 architecture but has minor
hardware differences compared to mt2701. It introduces mtk_iommu_type to
distinguish between the mt2701 and mt6572 variants.

> diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
> index 72355f41c36af..b6f547b100d10 100644
> --- a/drivers/iommu/mtk_iommu_v1.c
> +++ b/drivers/iommu/mtk_iommu_v1.c

[ ... ]

> @@ -180,25 +199,33 @@ static void mtk_iommu_v1_tlb_flush_range(struct mtk_iommu_v1_data *data,
>  					 unsigned long iova, size_t size)
>  {
>  	int ret;
> -	u32 tmp;
> +	u32 tmp, val = F_INVLD_EN0;
>  
> -	writel_relaxed(F_INVLD_EN1 | F_INVLD_EN0,
> -		data->base + REG_MMU_INV_SEL);
> +	if (data->type == MTK_IOMMU_V1)
> +		val |= F_INVLD_EN1;
> +
> +	writel_relaxed(val, data->base + REG_MMU_INV_SEL);
>  	writel_relaxed(iova & F_MMU_FAULT_VA_MSK,
>  		data->base + REG_MMU_INVLD_START_A);
>  	writel_relaxed((iova + size - 1) & F_MMU_FAULT_VA_MSK,
>  		data->base + REG_MMU_INVLD_END_A);
>  	writel_relaxed(F_MMU_INV_RANGE, data->base + REG_MMU_INVALIDATE);

[Severity: High]
This isn't a bug introduced by this patch, but does this sequence of global
register writes lack a lock?

In mtk_iommu_v1_unmap() and mtk_iommu_v1_map(), pgtlock is dropped before
calling mtk_iommu_v1_tlb_flush_range():

drivers/iommu/mtk_iommu_v1.c:mtk_iommu_v1_unmap() {
    ...
	spin_unlock_irqrestore(&dom->pgtlock, flags);

	mtk_iommu_v1_tlb_flush_range(dom->data, iova, size);
    ...
}

If multiple CPUs concurrently perform map/unmap operations, can the
non-atomic sequence of writel_relaxed() calls to these shared global MMIO
registers interleave, causing the IOMMU to flush incorrect IOVA ranges?

> -	ret = readl_poll_timeout_atomic(data->base + REG_MMU_CPE_DONE,
> -				tmp, tmp != 0, 10, 100000);
> -	if (ret) {
> -		dev_warn(data->dev,
> -			 "Partial TLB flush timed out, falling back to full flush\n");
> -		mtk_iommu_v1_tlb_flush_all(data);
> +	if (data->type == MTK_IOMMU_V1) {
> +		ret = readl_poll_timeout_atomic(data->base + REG_MMU_CPE_DONE,
> +						tmp, tmp != 0, 10, 100000);
> +		if (ret) {
> +			dev_warn(data->dev,
> +				 "Partial TLB flush timed out, falling back to full flush\n");
> +			mtk_iommu_v1_tlb_flush_all(data);
> +		}
> +
> +		/* Clear the CPE status */
> +		writel_relaxed(0, data->base + REG_MMU_CPE_DONE);
> +	} else {
> +		/* Make sure the TLB flush is done */
> +		wmb();
>  	}
> -	/* Clear the CPE status */
> -	writel_relaxed(0, data->base + REG_MMU_CPE_DONE);
>  }

[Severity: High]
Does a wmb() CPU barrier provide actual hardware synchronization for the
MT6572 TLB flush completion?

A wmb() only orders CPU memory writes; it does not ensure the write has
reached the IOMMU hardware, nor does it poll the hardware status to verify
the TLB flush has finished.

If the software unmap path returns and allows memory to be freed while the
hardware is still performing the TLB invalidation, can a device DMA into
freed memory before the TLB entry actually invalidates, causing
Use-After-Free?

[Severity: High]
This is a pre-existing issue, but is there a race condition during hardware
interrupt handling when tearing down the driver?

In the probe error path and in mtk_iommu_v1_remove(), clk_disable_unprepare()
is called while the hardware IRQ is still actively registered:

drivers/iommu/mtk_iommu_v1.c:mtk_iommu_v1_probe() {
    ...
out_clk_unprepare:
	clk_disable_unprepare(data->bclk);
out_put_larbs:
	for (i = 0; i < MTK_LARB_NR_MAX; i++)
		put_device(data->larb_imu[i].dev);

	return ret;
}

drivers/iommu/mtk_iommu_v1.c:mtk_iommu_v1_remove() {
    ...
	clk_disable_unprepare(data->bclk);
	devm_free_irq(&pdev->dev, data->irq, data);
    ...
}

If a hardware interrupt fires between the clock disable and the IRQ being
freed (or cleaned up by devres), could mtk_iommu_v1_isr() execute and
perform readl_relaxed() on the unclocked hardware, resulting in a
synchronous external abort?

[Severity: High]
This is a pre-existing issue, but can a NULL pointer dereference happen in
mtk_iommu_v1_domain_free() if the domain was never attached?

If an IOMMU domain is successfully allocated via
mtk_iommu_v1_domain_alloc_paging() but device attachment fails or is
skipped, dom->data remains NULL because it is only populated inside
mtk_iommu_v1_attach_device() (via mtk_iommu_v1_domain_finalise).

When the IOMMU core subsequently invokes iommu_domain_free(), it calls
mtk_iommu_v1_domain_free():

drivers/iommu/mtk_iommu_v1.c:mtk_iommu_v1_domain_free() {
    ...
	struct mtk_iommu_v1_data *data = dom->data;

	dma_free_coherent(data->dev, M2701_IOMMU_PGT_SIZE,
			dom->pgt_va, dom->pgt_pa);
    ...
}

Does unconditionally dereferencing data->dev here cause a kernel panic if
data is NULL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-6572-iommu-v1-0-de261da09fb2@protonmail.com?part=3