Re: [PATCH v5 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:
- [High] The driver incorrectly uses `generic_single_device_group` for multiple devices sharing a single translation domain, violating hardware isolation semantics.
- [High] Missing device link between the IOMMU and its cache device allows Use-After-Free on device removal.
- [High] Missing `.remove` callback leaves a dangling IOMMU device in the global list upon device removal.
- [High] The `domain->geometry` structure is left uninitialized, restricting the translation aperture to 0 and breaking TLB syncs.
--

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

iommu: Add Broadcom BCM2712 IOMMU driver

This IOMMU translates memory access requests for the VC6 display
pipeline and various multimedia devices in the Broadcom BCM2712 SoC used
on Raspberry Pi 5.

> diff --git a/drivers/iommu/bcm2712-iommu.c b/drivers/iommu/bcm2712-iommu.c
> new file mode 100644
> index 0000000000000..301855475c88b
> --- /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)
> +{
> +	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;
> +
> +		/* Configure translation aperture */
> +		bcm2712_iommu_writel(mmu, MMMU_ADDR_CAP_OFFSET,
> +				     MMMU_ADDR_CAP_ENABLE |
> +					     (domain->geometry.aperture_end >>
> +					      ADDR_CAP_SHIFT));

[Severity: High]
Is domain->geometry.aperture_end left uninitialized here?

In bcm2712_iommu_domain_alloc(), domain is allocated with kzalloc_obj(), which
zeroes the structure, but geometry.aperture_end is never explicitly initialized.

Would this configure a 0-sized translation aperture when writing to the
hardware register?

> +
> +		/*
> +		 * When the IOMMU handles a request, it adds the
> +		 * PT_PA_BASE_OFFSET to (IOVA>>32) to calculate the PFN of the
> +		 * corresponding L1 directory page.
> +		 * IOVA bits [31:22] are then used to fetch the L1 descriptor
> +		 * within (which in turn points to the L2 table).
> +		 * This clever logic would allow for a L1 table larger than 4kb
> +		 * (and hence a larger aperture).
> +		 */
> +		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]
Can this violate IOMMU isolation and corrupt DMA?

The hardware seems to share a single translation table across all connected
devices, as noted in the commit message. However, the driver configures
.device_group = generic_single_device_group, allowing the IOMMU framework to
assign a unique group and potentially a different domain to each device.

When bcm2712_iommu_attach_dev() is called for a subsequent device, won't it
unconditionally reprogram the shared MMMU_PT_PA_BASE_OFFSET register, silently
hijacking and breaking DMA translations for previously attached devices?

[ ... ]

> +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: High]
Since domain->geometry.aperture_end is uninitialized and zero, wouldn't
aperture_size evaluate to 1 byte, failing to invalidate the full TLB mapping?

[ ... ]

> +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;
> +
> +	domain = kzalloc_obj(*domain);
> +	if (!domain)
> +		return NULL;

[Severity: High]
Does this code miss initializing the domain->base.geometry fields, which
results in the issues pointed out in bcm2712_iommu_attach_dev() and
bcm2712_iommu_sync_all()?

[ ... ]

> +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,
> +	.get_resv_regions    = iommu_dma_get_resv_regions,
> +	.of_xlate            = bcm2712_iommu_of_xlate,
> +};

[Severity: High]
Does configuring generic_single_device_group incorrectly isolate devices that
share a single hardware translation table, leading to the domain hijacking in
bcm2712_iommu_attach_dev()?

[ ... ]

> +static int bcm2712_iommu_init_cache(struct bcm2712_iommu *mmu,
> +				    struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +
> +	struct device_node *cache_np __free(device_node) =
> +		of_parse_phandle(dev->of_node, "brcm,iommu-cache", 0);
> +	if (!cache_np)
> +		return dev_err_probe(dev, -ENOENT,
> +				     "missing brcm,iommu-cache property\n");
> +
> +	struct platform_device *cache_pdev __free(platform_device_put) =
> +		of_find_device_by_node(cache_np);
> +	if (!cache_pdev)
> +		return dev_err_probe(dev, -EPROBE_DEFER,
> +				     "waiting for cache device\n");
> +
> +	mmu->cache = platform_get_drvdata(cache_pdev);

[Severity: High]
Does this leave a dangling pointer if the cache device is unbound?

The reference to cache_pdev is automatically put when leaving this function
due to __free(platform_device_put), and no device link is established.

If the cache device is dynamically destroyed, its devres allocations will be
freed, leaving mmu->cache pointing to freed memory which could cause a
use-after-free during a TLB flush.

[ ... ]

> +static struct platform_driver bcm2712_iommu_driver = {
> +	.driver	= {
> +		.name		     = "bcm2712-iommu",
> +		.of_match_table	     = bcm2712_iommu_of_match,
> +		.suppress_bind_attrs = true,
> +	},
> +	.probe	= bcm2712_iommu_probe,
> +};

[Severity: High]
Can removing the device cause a use-after-free in the IOMMU subsystem?

Because this driver registers the IOMMU device using iommu_device_register()
but does not provide a .remove callback, iommu_device_unregister() is never
called. 

If the platform device is removed, the driver core frees the devm_kzalloc
allocated struct bcm2712_iommu, but it remains in the global iommu_device_list,
leading to a use-after-free when traversed by other parts of the kernel.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-bcm2712-iommu-submit-v5-0-2539c52b249e@reactivated.net?part=4