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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Race condition in cache initialization exposes uninitialized memory to IOMMU devices.
- [High] TLB synchronization only flushes the original MMU when a domain is shared across multiple MMUs.
- [High] Generic page table is misconfigured with a 32-bit VASZ for 36-bit IOVAs.
- [High] Missing device links for devres-managed dependencies leading to Use-After-Free.
- [High] Missing MMU local TLB flush when attaching a domain.
- [High] TLB synchronization corrupts live hardware state for unattached domains.
- [Medium] Format string vulnerability via dynamic device name in `iommu_device_sysfs_add`.
- [Low] COMPILE_TEST allows 32-bit builds, causing 64-bit IOVA constants to truncate.
--

--- Patch [4]: [PATCH v2 4/5] iommu: Add Broadcom BCM2712 IOMMU driver ---
commit cf5d6742acaca9334ddee5d5953d5aba3f7cdcb7
Author: Daniel Drake <dan@reactivated.net>

iommu: Add Broadcom BCM2712 IOMMU driver

This commit introduces the Broadcom BCM2712 IOMMU driver used on Raspberry Pi
5, enabling memory translation for the VC6 display pipeline and multimedia
devices. It supports a 4GB translation aperture at a high memory address and
includes a shared IOMMU cache.

> diff --git a/drivers/iommu/bcm2712-iommu-cache.c b/drivers/iommu/bcm2712-iommu-cache.c
> new file mode 100644
> index 0000000000000..d97a8cda448ee
> --- /dev/null
> +++ b/drivers/iommu/bcm2712-iommu-cache.c
[ ... ]
> +static int bcm2712_iommu_cache_probe(struct platform_device *pdev)
> +{
> +	struct bcm2712_iommu_cache *cache;
> +
> +	cache = devm_kzalloc(&pdev->dev, sizeof(*cache), GFP_KERNEL);
> +	if (!cache)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, cache);
> +	spin_lock_init(&cache->hw_lock);
> +
> +	cache->reg_base = devm_platform_ioremap_resource(pdev, 0);

[Severity: High]
Is there a race condition here during parallel device probing? By
publishing the cache driver data globally before the IO memory is mapped,
a concurrent probe of the IOMMU driver could retrieve the cache device and
call bcm2712_iommu_cache_flush() on an uninitialized cache->reg_base. If
the IO remap fails, could this also leave the IOMMU driver with a dangling
pointer?

> diff --git a/drivers/iommu/bcm2712-iommu.c b/drivers/iommu/bcm2712-iommu.c
> new file mode 100644
> index 0000000000000..55678009f9c45
> --- /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)
> +{
[ ... ]
> +	/* Flush (and enable) the shared TLB cache; enable this MMU. */
> +	bcm2712_iommu_cache_flush(mmu->cache);
> +	MMU_WR(MMMU_CTRL_OFFSET, MMMU_CTRL_OPERATING_FLAGS);

[Severity: High]
Does this miss flushing the local TLB on the MMU when attaching a domain?
The hardware is configured with MMMU_CTRL_OPERATING_FLAGS but omits the
MMMU_CTRL_TLB_CLEAR bit. If the device was previously attached to a
different domain or identity mode, will the local TLB retain stale
translation entries?

> +unlock:
> +	spin_unlock_irqrestore(&mmu->hw_lock, flags);
> +	return 0;
> +}
[ ... ]
> +static int bcm2712_iommu_sync_range(struct iommu_domain *domain,
> +				    unsigned long iova, size_t size)
> +{
> +	struct bcm2712_iommu *mmu = domain_to_mmu(domain);

[Severity: High]
If a domain is shared across multiple IOMMU hardware instances, will this
only retrieve and flush the original MMU it was allocated with? The IOMMU
framework can attach a single domain to multiple devices. Since
bcm2712_iommu_domain_alloc() hardcodes domain->mmu to the first MMU, will
all other attached MMU instances retain stale TLB entries?

> +	unsigned long flags;
> +	u32 val;
> +
> +	spin_lock_irqsave(&mmu->hw_lock, flags);
> +	bcm2712_iommu_cache_flush(mmu->cache);
> +
> +	/* If invalidating more than 16MB, just do a full TLB clear */
> +	if (size >= SZ_16M) {
> +		MMU_WR(MMMU_CTRL_OFFSET,
> +		       MMMU_CTRL_OPERATING_FLAGS | MMMU_CTRL_TLB_CLEAR);

[Severity: High]
Could this corrupt live hardware state for unattached domains?
The IOMMU API allows mapping memory into a domain before it is attached to
a device. When iommu_map() calls bcm2712_iommu_sync_map(), will this
unconditionally overwrite the active hardware state of an MMU that might
currently be operating in identity mode or disabled?

[ ... ]
> +static struct iommu_domain *bcm2712_iommu_domain_alloc(struct device *dev)
> +{
[ ... ]
> +	/* 2-level format: 10-bit L1 + 10-bit L2 + 12-bit page offset */
> +	cfg.common.hw_max_vasz_lg2 =
> +		(2 * PTES_PER_IOPG_SHIFT) + IOMMU_PAGE_SHIFT;
> +
> +	/* PTEs encode a 28-bit output address PFN */
> +	cfg.common.hw_max_oasz_lg2 = 28 + IOMMU_PAGE_SHIFT;
> +
> +	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);
> +	if (!domain->default_page)
> +		goto err;
> +
> +	domain->base.geometry.aperture_start = BCM2712_APERTURE_BASE;

[Severity: High]
Does this misconfigure the generic page table for 36-bit IOVAs?
The driver sets cfg.common.hw_max_vasz_lg2 to 32 (4GB capacity), but the
aperture is placed at 40GB (BCM2712_APERTURE_BASE), which requires 36
bits.

Since the generic PT operations are wired up directly via
IOMMU_PT_DOMAIN_OPS(bcm2712) without masking the 40GB base offset from the
incoming IOVAs, will the generic PT allocator process 36-bit IOVAs against
a 32-bit configured capacity and cause out-of-bounds indexing or map
rejections?

[Severity: Low]
Can this cause 64-bit IOVA constants to truncate on 32-bit architectures?
The driver assigns BCM2712_APERTURE_BASE to
domain->base.geometry.aperture_start. Since COMPILE_TEST in Kconfig allows
building without 64BIT guarantees, dma_addr_t might be 32-bit. Will this
assignment overflow and break compile testing correctness?

> +	domain->base.geometry.aperture_end = BCM2712_APERTURE_END - 1;
> +	domain->base.geometry.force_aperture = true;
> +	domain->base.ops = &bcm2712_paging_domain_ops;
> +	return &domain->base;
> +
> +err:
> +	bcm2712_iommu_domain_free(&domain->base);
> +	return NULL;
> +}
> +
> +static struct bcm2712_iommu *
> +bcm2712_iommu_get_by_fwnode(struct fwnode_handle *fwnode)
> +{
> +	struct device *dev =
> +		bus_find_device_by_fwnode(&platform_bus_type, fwnode);
> +	struct bcm2712_iommu *mmu;
> +
> +	if (!dev)
> +		return NULL;
> +
> +	mmu = dev_get_drvdata(dev);
> +	put_device(dev);
> +	return mmu;
> +}

[Severity: High]
Does dropping the device reference here without a device link risk a
use-after-free? The driver returns a pointer to devres-managed private
data. If the underlying device is unbound via sysfs, devres will free the
object, leaving a dangling pointer.

[ ... ]
> +static int bcm2712_iommu_init_cache(struct bcm2712_iommu *mmu,
> +				    struct platform_device *pdev)
> +{
[ ... ]
> +	mmu->cache = platform_get_drvdata(cache_pdev);
> +	if (!mmu->cache) {
> +		put_device(&cache_pdev->dev);
> +		return -EPROBE_DEFER;
> +	}
> +
> +	put_device(&cache_pdev->dev);

[Severity: High]
Similarly, does dropping the cache device reference here require
establishing a device link? If the cache device is unbound, devres will
immediately free the cache object. Any subsequent DMA unmap operation on
the IOMMU side will attempt to flush the cache and dereference the
dangling mmu->cache pointer.

> +	return 0;
> +}
> +
> +static int bcm2712_iommu_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	ret = iommu_device_sysfs_add(&mmu->iommu, dev, NULL, dev_name(dev));

[Severity: Medium]
Is it safe to pass a dynamic string directly as the format string
parameter to iommu_device_sysfs_add()? If the device tree node name
happens to contain format specifiers, kobject_set_name_vargs() will
misinterpret them. Could this be updated to pass "%s", dev_name(dev)?

> +	if (ret)
> +		return ret;
> +
> +	ret = iommu_device_register(&mmu->iommu, &bcm2712_iommu_ops, dev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-bcm2712-iommu-submit-v2-0-0247b5c03de8@reactivated.net?part=4