Re: [PATCH 4/6] iommu/generic_pt: Add Broadcom BCM2712 page table format

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Physical addresses for page tables are silently truncated if they exceed 40 bits.
- [High] Installing a contiguous block for an unsupported page size results in mismatched PAGESIZE hints and partial unmapping.
- [Low] Unnecessary Kconfig dependency on !GENERIC_ATOMIC64 restricts compilation on 32-bit architectures.
- [Low] Missing module test dependency in IOMMU_PT_KUNIT_TEST.
--

--- Patch [4]: [PATCH 4/6] iommu/generic_pt: Add Broadcom BCM2712 page table format ---
commit de6027129434bac44f0964115b67d7adb9dca294
Author: Daniel Drake <dan@reactivated.net>

iommu/generic_pt: Add Broadcom BCM2712 page table format

The BCM2712 IOMMU implements a 2-level page table format. It is relatively
simple, with one unusual aspect: leaf entries can only be installed at
Level 0.

> diff --git a/drivers/iommu/generic_pt/Kconfig b/drivers/iommu/generic_pt/Kconfig
> index f4ed1add58b74..5c01cebb365fa 100644
> --- a/drivers/iommu/generic_pt/Kconfig
> +++ b/drivers/iommu/generic_pt/Kconfig
[ ... ]
> +config IOMMU_PT_BCM2712
> +	tristate "IOMMU page table for Broadcom BCM2712"
> +	depends on !GENERIC_ATOMIC64 # for cmpxchg64

[Severity: Low]
Is this dependency necessary? The bcm2712 format appears to use 32-bit entries
(pt_bcm2712_entry_t) and pt_table_install32(), which only requires standard
32-bit cmpxchg.

It doesn't seem to use cmpxchg64, and this dependency artificially restricts
compilation on 32-bit architectures during COMPILE_TEST.

> +	help
> +	  iommu_domain implementation for the Broadcom BCM2712 IOMMU found on
> +	  Raspberry Pi 5. It supports 4K page sizes over a 2-level page table
> +	  format.
> +
> +	  Selected automatically by an IOMMU driver that uses this format.
> +
>  config IOMMU_PT_KUNIT_TEST
>  	tristate "IOMMU Page Table KUnit Test" if !KUNIT_ALL_TESTS
>  	depends on KUNIT

[Severity: Low]
Does this test need a conditional dependency for the new BCM2712 format?

If IOMMU_PT_BCM2712 is compiled as a module, but this test is built-in (=y),
it might cause a linker failure. Other formats typically have a dependency
list like "depends on IOMMU_PT_X86_64 || !IOMMU_PT_X86_64" to ensure the test
degrades to a module if any supported format is a module.

> diff --git a/drivers/iommu/generic_pt/fmt/bcm2712.h b/drivers/iommu/generic_pt/fmt/bcm2712.h
> new file mode 100644
> index 0000000000000..e62d59181bfe5
> --- /dev/null
> +++ b/drivers/iommu/generic_pt/fmt/bcm2712.h
[ ... ]
> +static inline void
> +bcm2712pt_install_leaf_entry(struct pt_state *pts, pt_oaddr_t oa,
> +			     unsigned int oasz_lg2,
> +			     const struct pt_write_attrs *attrs)
> +{
[ ... ]
> +	if (oasz_lg2 == PT_GRANULE_LG2SZ) {
> +		WRITE_ONCE(tablep[pts->index], entry);
> +		pts->entry = entry;
> +	} else {
> +		struct pt_bcm2712 *table =
> +			common_to_bcm2712pt(pts->range->common);
> +		u32 *end;
> +
> +		tablep += pts->index;
> +		end = tablep + log2_to_int(oasz_lg2 - PT_GRANULE_LG2SZ);
> +
> +		/*
> +		 * Leaf entries can contain hints indicating multi-page
> +		 * contiguous mappings, presumably to permit TLB optimization
> +		 */
> +		if (oasz_lg2 == ilog2(SZ_4M))
> +			entry |= FIELD_PREP(BCM2712PT_PAGESIZE, 3);
> +		else if (oasz_lg2 == table->superpage_lg2)
> +			entry |= FIELD_PREP(BCM2712PT_PAGESIZE, 2);
> +		else if (oasz_lg2 == table->bigpage_lg2)
> +			entry |= FIELD_PREP(BCM2712PT_PAGESIZE, 1);
> +
> +		pts->entry = entry;
> +		for (; tablep != end; tablep++, entry++)
> +			WRITE_ONCE(*tablep, entry);
> +	}
> +}

[Severity: High]
If a driver requests a contiguous leaf entry for an unsupported size, does
this logic leave the PAGESIZE hint as 0 while still writing the full block
of PTEs?

If the layout is initialized with an incoming cfg->pgsize_bitmap containing
sizes this format cannot encode, the hint is never set here. Later, when
unmapping, bcm2712pt_entry_num_contig_lg2() will read the 0 hint, assume it
is a single 4KB page, and fail to clear the remainder of the block.

Could this result in stale mappings remaining active in the hardware and
causing memory corruption or use-after-free via DMA?

[ ... ]
> +static inline bool bcm2712pt_install_table(struct pt_state *pts,
> +					   pt_oaddr_t table_pa,
> +					   const struct pt_write_attrs *attrs)
> +{
> +	pt_bcm2712_entry_t entry =
> +		BCM2712PT_VALID |
> +		FIELD_PREP(BCM2712PT_PFN,
> +			   oalog2_div(table_pa, PT_GRANULE_LG2SZ));
> +
> +	return pt_table_install32(pts, entry);
> +}

[Severity: High]
Can this silently truncate physical addresses larger than 40 bits?

The framework allocates page tables using standard GFP_KERNEL memory, which
can exceed 1TB on generic ARM64 kernels. Since the BCM2712 format encodes
a 28-bit PFN, and FIELD_PREP silently masks out upper bits, an address
above 1TB could be truncated. This could result in programming an incorrect
physical address into the hardware table.

[ ... ]
> +static inline int bcm2712_pt_fmt_init(struct pt_iommu_table *fmt_table,
> +				      const struct pt_iommu_bcm2712_cfg *cfg)
> +{
> +	fmt_table->bcm2712pt.pgsize_bitmap = cfg->pgsize_bitmap;
> +	fmt_table->bcm2712pt.bigpage_lg2 = cfg->bigpage_lg2;
> +	fmt_table->bcm2712pt.superpage_lg2 = cfg->superpage_lg2;
> +	pt_top_set_level(&fmt_table->bcm2712pt.common, PT_MAX_TOP_LEVEL);
> +	return 0;
> +}

[Severity: High]
Should this pgsize_bitmap be sanitized or masked against the specific sizes
the bcm2712 format can actually encode (4K, bigpage, superpage, 4M)?

Implicitly trusting the incoming configuration is what allows the unsupported
page size issue in bcm2712pt_install_leaf_entry() to occur.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260712-bcm2712-iommu-submit-v1-0-80e10cdde2ea@reactivated.net?part=4