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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] The driver assigns devices sharing the same IOMMU to separate groups via `generic_single_device_group`, breaking isolation and corrupting hardware state.
- [Critical] The blocking domain leaves the MMU enabled but does not clear `MMMU_ILLEGAL_ADR_OFFSET`, causing a Use-After-Free of the previous domain's default page.
- [High] Missing TLB and cache flush in `bcm2712_iommu_blocking_attach` allows DMA to bypass the blocking domain.
- [High] The default error page is allocated without `__GFP_ZERO`, leaking uninitialized kernel memory to devices on illegal DMA reads.
--

commit b630b8b804af580183dabdb17ea88c2d7bc1b777
Author: Daniel Drake <dan@reactivated.net>

iommu: Add Broadcom BCM2712 IOMMU driver

This commit adds a new IOMMU driver for the Broadcom BCM2712 SoC used in the
Raspberry Pi 5. It uses the generic_pt framework for 2-level page tables and
manages a centralized L2 TLB shared across the SoC's IOMMU instances.

> --- /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);
> +
> +	guard(spinlock_irqsave)(&mmu->hw_lock);
> +
> +	/*
> +	 * Completely block DMA by disabling both the bypass window and the
> +	 * translation aperture.
> +	 */
> +	MMU_WR(MMMU_BYPASS_START_OFFSET, 0);
> +	MMU_WR(MMMU_BYPASS_END_OFFSET, 0);
> +	MMU_WR(MMMU_ADDR_CAP_OFFSET, MMMU_ADDR_CAP_ENABLE);
> +	MMU_WR(MMMU_CTRL_OFFSET, MMMU_CTRL_OPERATING_FLAGS);
> +
> +	mmu->domain = NULL;

[Severity: High]
Does this leave stale translations in the hardware TLB indefinitely?
The IOMMU transitions to a blocking domain and the hardware remains
enabled with MMMU_CTRL_OPERATING_FLAGS, but there is no TLB clear or
cache flush performed before returning. Since mmu->domain is set to NULL
here, subsequent unmap or iotlb_sync calls on the detached paging domain
will return immediately without flushing, which might allow a blocked
device to perform DMA to previously mapped pages.

[Severity: Critical]
Could this also lead to a use-after-free of the default page? When
attaching the blocking domain, MMMU_ILLEGAL_ADR_OFFSET is not cleared,
but the MMU is left enabled. When the previous paging domain is
subsequently freed in bcm2712_iommu_domain_free(), it bypasses disabling
the hardware because mmu->domain is NULL. It will then free
mydomain->default_page, meaning the active hardware will redirect all
further illegal DMA accesses to the physical address of a freed page.

> +
> +	return 0;
> +}

[ ... ]

> +static int bcm2712_iommu_attach_dev(struct iommu_domain *domain,
> +				    struct device *dev,
> +				    struct iommu_domain *old)
> +{
> +	struct bcm2712_iommu *mmu = dev_iommu_priv_get(dev);
> +	struct bcm2712_iommu_domain *mydomain = to_bcm2712_domain(domain);
> +	struct pt_iommu_bcm2712_hw_info info;
> +	u32 default_page_pfn;
> +	int ret = 0;
> +
> +	/* all attached devices must belong to the same IOMMU instance */
> +	if (mydomain->mmu != mmu)
> +		return -EINVAL;
> +
> +	scoped_guard(spinlock_irqsave, &mmu->hw_lock) {
> +		if (mmu->domain == mydomain)
> +			break;
> +
> +		mmu->domain = mydomain;
> +
> +		/* Configure translation aperture */
> +		MMU_WR(MMMU_ADDR_CAP_OFFSET,
> +		       MMMU_ADDR_CAP_ENABLE +
> +		       (domain->geometry.aperture_end >> ADDR_CAP_SHIFT));

[Severity: Critical]
Will this break mappings for other devices sharing the IOMMU? The commit
states there is no stream ID tagging to differentiate between requests from
different devices. However, because bcm2712_iommu_ops configures
generic_single_device_group, the IOMMU core assigns a distinct group and
domain to each device. When a new device is attached here, it sequentially
overwrites shared registers like MMMU_ADDR_CAP_OFFSET and
MMMU_PT_PA_BASE_OFFSET, silently breaking DMA traffic for previously
attached devices.

> +
> +		/*
> +		 * When the IOMMU handles a request, it adds the

[ ... ]

> +static struct iommu_domain *bcm2712_iommu_domain_alloc(struct device *dev)
> +{
> +	struct bcm2712_iommu *mmu = dev_iommu_priv_get(dev);
> +	struct bcm2712_iommu_domain *domain;
> +	struct pt_iommu_bcm2712_cfg cfg;
> +	int ret;

[ ... ]

> +	ret = pt_iommu_bcm2712_init(&domain->pt, &cfg, GFP_KERNEL);
> +	if (ret)
> +		goto err;
> +
> +	/* Set up a default (error) page used to catch illegal reads/writes */
> +	domain->default_page = iommu_alloc_pages_sz(GFP_KERNEL, PAGE_SIZE);

[Severity: High]
Could this leak uninitialized kernel memory to devices? This page is
configured as the sink/source for illegal DMA reads in
MMMU_ILLEGAL_ADR_OFFSET. Without the __GFP_ZERO flag, the allocated page
might contain leftover kernel data that a device could exfiltrate by
issuing an illegal DMA read.

> +	if (!domain->default_page)
> +		goto err;
> +
> +	domain->base.ops = &bcm2712_paging_domain_ops;
> +	return &domain->base;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825-bcm2712-iommu-submit-v3-0-7f143e7289b6@reactivated.net?part=4