Re: [PATCH v4 4/5] iommu: Add Broadcom BCM2712 IOMMU driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] bcm2712_iommu_blocking_attach leaves a 256MB translation aperture open and the MMU enabled, causing a hardware Use-After-Free of the IOMMU page tables.
--

commit 62f0e0782086d4384ce15907946195c825fc95b3
Author: Daniel Drake <dan@reactivated.net>
iommu: Add Broadcom BCM2712 IOMMU driver

This commit introduces a new IOMMU driver for the Broadcom BCM2712 SoC used
on Raspberry Pi 5. It uses the generic_pt framework for its 2-level page tables
and supports the centralized L2 TLB.

> diff --git a/drivers/iommu/bcm2712-iommu.c b/drivers/iommu/bcm2712-iommu.c
> new file mode 100644
> index 0000000000000..27dad375b7d4b
> --- /dev/null
> +++ b/drivers/iommu/bcm2712-iommu.c
[ ... ]
> +static int bcm2712_iommu_blocking_attach(struct iommu_domain *blocking_domain,
> +					 struct device *dev,
> +					 struct iommu_domain *old)
> +{
> +	struct bcm2712_iommu *mmu = dev_iommu_priv_get(dev);
> +	int ret = 0;
> +
> +	scoped_guard(spinlock_irqsave, &mmu->hw_lock) {
> +		/*
> +		 * Completely block DMA by disabling both the bypass window
> +		 * and the translation aperture.
> +		 */
> +		bcm2712_iommu_writel(mmu, MMMU_BYPASS_START_OFFSET, 0);
> +		bcm2712_iommu_writel(mmu, MMMU_BYPASS_END_OFFSET, 0);
> +		bcm2712_iommu_writel(mmu, MMMU_ADDR_CAP_OFFSET,
> +				     MMMU_ADDR_CAP_ENABLE);
> +		bcm2712_iommu_writel(mmu, MMMU_ILLEGAL_ADR_OFFSET, 0);
> +		ret = bcm2712_iommu_clear_and_enable(mmu);
> +		mmu->domain = NULL;

[Severity: Critical]
Does this mistakenly enable a 256MB translation aperture rather than
disabling it?

The code writes MMMU_ADDR_CAP_ENABLE to the MMMU_ADDR_CAP_OFFSET register
and leaves the MMU enabled via bcm2712_iommu_clear_and_enable(), without
safely resetting the page table base register.

Also, clearing mmu->domain to NULL here appears to prevent the old domain
from disabling the hardware before its page tables are freed.

> +	}
> +
> +	if (ret)
> +		dev_err_ratelimited(mmu->dev,
> +				    "TLB clear timed out during blocking attach\n");
> +
> +	return ret;
> +}
[ ... ]
> +static void bcm2712_iommu_domain_free(struct iommu_domain *domain)
> +{
> +	struct bcm2712_iommu_domain *mydomain = to_bcm2712_domain(domain);
> +	struct bcm2712_iommu *mmu = mydomain->mmu;
> +
> +	scoped_guard(spinlock_irqsave, &mmu->hw_lock) {
> +		if (mmu->domain == mydomain) {
> +			bcm2712_iommu_writel(mmu, MMMU_CTRL_OFFSET, 0);
> +			mmu->domain = NULL;
> +		}
> +	}
> +
> +	pt_iommu_deinit(&mydomain->pt.iommu);

[Severity: Critical]
Can this cause a hardware use-after-free of the IOMMU page tables?

When a device is detached or bound to a blocking domain,
bcm2712_iommu_blocking_attach() clears mmu->domain to NULL. When the old
domain is subsequently freed here, the mmu->domain == mydomain check
evaluates to false.

Because the MMU hardware is not disabled, it can continue fetching page
table entries from memory while they are being freed back to the kernel
allocator via pt_iommu_deinit().

> +	if (mydomain->default_page)
> +		iommu_free_pages(mydomain->default_page);
> +	kfree(mydomain);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902-bcm2712-iommu-submit-v4-0-9dbb657578c1@reactivated.net?part=4