Re: [PATCH v5 04/13] iommu: of_iommu: Add support for "iommu-ranges" on a device node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] iommu: size_t truncation of derived reserved region length on 32-bit systems
- [Medium] iommu: DT ABI breakage by rejecting iommu-addresses without reg
- [Medium] iommu: OOB read in of_iommu_derive_resv_regions when parsing malformed iommu-ranges
--

commit 08a09989ffefcbf5d38171159dcc911ca993ff18
Author: Vikash Garodia <vikash.garodia@oss.qualcomm.com>

iommu: of_iommu: Add support for "iommu-ranges" on a device node

Add support for "iommu-ranges" on device nodes to specify usable IOVA
ranges directly when there is no backing "reg". Introduce logic to derive
reserved regions outside of "iommu-ranges" and restrict "iommu-addresses"
to be paired with a "reg" property.

Link: https://lore.kernel.org/all/662f7093-fb0a-4564-9ca0-98e03e68ba9a@kernel.org

> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index a18bb60f6f3df..9766a6944d0e1 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
[ ... ]
> +static void of_iommu_derive_resv_regions(struct device *dev, struct list_head *list)
> +{
[ ... ]
> +	maps = of_get_property(dev->of_node, "iommu-ranges", &size);
> +	if (!maps)
> +		return;
> +
> +	end = maps + size / sizeof(__be32);
> +
> +	while (maps < end) {
> +		phys_addr_t iova, iova_end;
> +		size_t length;
> +
> +		maps = of_translate_dma_region(dev->of_node, maps, &iova, &length);

[Severity: Medium]
Could this result in an out-of-bounds read if the "iommu-ranges" property is
malformed?

The loop condition checks `maps < end`, but of_translate_dma_region()
implicitly reads `na + ns` cells from the maps array.

Does this loop need to ensure there are at least `na + ns` cells remaining
before calling the translation function to prevent reading past the buffer?

[ ... ]
> +	list_for_each_entry_safe(pos, next_range, &ranges, node) {
> +		if (pos->start > next) {
> +			region = iommu_alloc_resv_region(next, pos->start - next, prot,
> +							 IOMMU_RESV_RESERVED, GFP_KERNEL);

[Severity: High]
Can the length argument overflow on 32-bit systems?

On 32-bit systems with 64-bit physical addresses (like ARM LPAE), address
differences can easily exceed 4GB. Because iommu_alloc_resv_region() takes
a 32-bit size_t length, passing a 64-bit gap size causes it to truncate.

Will this truncation result in a reservation of at most 4GB, leaving the
IOVA address space above that unprotected?

[ ... ]
> +	region = iommu_alloc_resv_region(next, PHYS_ADDR_MAX - next + 1,
> +					 prot, IOMMU_RESV_RESERVED, GFP_KERNEL);

[Severity: High]
Similarly, can passing `PHYS_ADDR_MAX - next + 1` here truncate the length
to its lower 32 bits on 32-bit systems?

If the remaining address space exceeds 4GB, this truncation could leave high
IOVA addresses unprotected, allowing the IOMMU allocator to hand out reserved
IOVAs.

[ ... ]
> @@ -214,10 +319,14 @@ void of_iommu_get_resv_regions(struct device *dev, struct list_head *list)
[ ... ]
> -		 * The "reg" property is optional and can be omitted by reserved-memory regions
> -		 * that represent reservations in the IOVA space, which are regions that should
> -		 * not be mapped.
> +		 * "iommu-addresses" must be used in combination with a "reg" that provides
> +		 * the physical address and size of this memory region, for an identity 1:1
> +		 * IOVA mapping to that physical memory.
>  		 */
>  		if (of_property_present(it.node, "reg")) {
>  			err = of_address_to_resource(it.node, 0, &phys);
[ ... ]
> -		}
> -
> -		maps = of_get_property(it.node, "iommu-addresses", &size);
> -		if (!maps)
> +		} else {
> +			dev_err(dev, "%pOF: iommu-addresses requires a reg property\n",
> +				it.node);
>  			continue;
> +		}

[Severity: Medium]
Will this break existing device trees?

This explicitly rejects the previously valid 'no reg' device tree
configurations. Existing device trees that validly relied on
"iommu-addresses" without a "reg" property to create IOVA reservations will
now have their reservations silently ignored.

Could this lead to the IOMMU allocating IOVAs in reserved spaces, causing
DMA faults?

[ ... ]
> @@ -258,6 +367,9 @@ void of_iommu_get_resv_regions(struct device *dev, struct list_head *list)
>  				}
>  				type = iommu_resv_region_get_type(dev, &phys, iova, length);
>  
> +				if (type != IOMMU_RESV_DIRECT)
> +					continue;
> +
>  				region = iommu_alloc_resv_region(iova, length, prot, type,
>  								 GFP_KERNEL);

[Severity: Medium]
Does this newly added check actively block pure reservations?

By enforcing that only identity mappings are honored, does this violate
backward compatibility for device trees that used pure reservations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260926-vpu_iommu_iova_handling-v5-0-0322ca5dc10c@oss.qualcomm.com?part=4