Re: [PATCH v7 5/6] iommu: Add Broadcom BCM2712 IOMMU driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] iommu: broken DMA isolation due to generic_single_device_group
- [High] iommu: uninitialized memory leak via incoherent default_page
- [Low] iommu: size_t overflow in bcm2712_iommu_sync_all()
--

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

This commit introduces the IOMMU driver for the Broadcom BCM2712 SoC used
on the Raspberry Pi 5. The driver uses the generic_pt framework to manage
2-level page tables and provides support for the centralized L2 TLB.

> --- /dev/null
> +++ b/drivers/iommu/bcm2712-iommu.c
> [ ... ]
> +static int bcm2712_iommu_attach_dev(struct iommu_domain *domain,
> +				    struct device *dev,
> +				    struct iommu_domain *old)
> +{
> [ ... ]
> +		pt_iommu_bcm2712_hw_info(&mydomain->pt, &info);
> +		bcm2712_iommu_writel(mmu, MMMU_PT_PA_BASE_OFFSET,
> +				     info.pt_base >> IOMMU_PAGE_SHIFT);

[Severity: High]
Since this IOMMU hardware shares a single translation aperture with no stream
ID tagging, what happens if multiple devices are attached to different IOMMU
domains simultaneously?

When a new domain is attached, this unconditionally updates the shared
MMMU_PT_PA_BASE_OFFSET register. Won't this silently hijack the hardware
translation context of any previously attached devices, forcing them into the
new domain and breaking DMA isolation?

> [ ... ]
> +static void bcm2712_iommu_sync_all(struct iommu_domain *domain)
> +{
> +	size_t aperture_size = domain->geometry.aperture_end -
> +			       domain->geometry.aperture_start + 1;
> +
> +	bcm2712_iommu_sync_range(domain, domain->geometry.aperture_start,
> +				 aperture_size);
> +}

[Severity: Low]
When this code is built on a 32-bit architecture (for instance, via
COMPILE_TEST), size_t is 32-bit. Since domain->geometry.aperture_end is
populated as 0xFFFFFFFF and aperture_start is 0, does the calculation
0xFFFFFFFF - 0 + 1 mathematically overflow to 0?

If aperture_size overflows to 0, bcm2712_iommu_sync_range() will return early
without flushing the hardware TLB. Could this be avoided by using a larger
type or adjusting the size calculation?

> [ ... ]
> +static struct iommu_domain *bcm2712_iommu_domain_alloc(struct device *dev)
> +{
> [ ... ]
> +	/* Set up a default (error) page used to catch illegal reads/writes */
> +	domain->default_page = iommu_alloc_pages_sz(GFP_KERNEL, PAGE_SIZE);
> +	if (!domain->default_page)
> +		goto err;

[Severity: High]
The default_page allocated here is zeroed by the CPU in its cache, but the
hardware is explicitly configured as DMA-incoherent (PT_FEAT_DMA_INCOHERENT).

When the physical address of this unflushed page is later provided to the
hardware's MMMU_ILLEGAL_ADR_OFFSET register, could the IOMMU fetch stale
physical memory contents by bypassing the CPU cache during an illegal read?
Should this page be explicitly flushed to main memory to prevent a potential
information leak?

> [ ... ]
> +static const struct iommu_ops bcm2712_iommu_ops = {
> +	.identity_domain     = &bcm2712_identity_domain,
> +	.domain_alloc_paging = bcm2712_iommu_domain_alloc,
> +	.probe_device        = bcm2712_iommu_probe_device,
> +	.device_group        = generic_single_device_group,

[Severity: High]
Does assigning generic_single_device_group improperly allocate an independent
IOMMU group for each device?

Since devices on this IOMMU lack hardware isolation and share the single
translation aperture updated in bcm2712_iommu_attach_dev(), shouldn't they
share a single IOMMU group to prevent userspace or VMs from assigning them
to separate domains?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260927-bcm2712-iommu-submit-v7-0-dfa2b975cbc9@reactivated.net?part=5