Re: [PATCH v3 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 5 potential issue(s) to consider:

New issues:
- [High] Integer overflow when computing `pos->end + 1` causes incorrect overlapping reserved regions or zero-length reservations.
- [High] Breaks backward compatibility for `iommu-addresses` nodes without a `reg` property or with pure IOVA reservations.
- [Medium] Error paths fail-open and silently grant the device unrestricted IOVA access instead of restricting it.

Pre-existing issues:
- [High] Desynced DT property parsing when a phandle does not match `dev->of_node` causes missing reservations.
- [High] Missing check for NULL return from `of_translate_dma_region` causes NULL pointer dereference on the next iteration.
--

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

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

This commit introduces support for the iommu-ranges property to allow
devices to specify their IOVA ranges without a backing reg property. It
derives implicit reserved regions for anything outside these ranges and
restricts iommu-addresses to only be used when paired with a reg property.

Link: https://lore.kernel.org/all/662f7093-fb0a-4564-9ca0-98e03e68ba9a@kernel.org
Link: https://github.com/devicetree-org/dt-schema/pull/207

> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index a18bb60f6f3df..1b014666d6b8e 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -190,6 +190,90 @@ iommu_resv_region_get_type(struct device *dev,
>  	return IOMMU_RESV_RESERVED;
>  }
>  
> +/**
> + * of_iommu_derive_resv_regions - derive reserved regions which
> + *                                are outside of iommu-ranges

[ ... ]

> +	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;
> +		size_t length;
> +
> +		maps = of_translate_dma_region(dev->of_node, maps, &iova, &length);
> +		if (!maps) {
> +			dev_err(dev, "failed to parse iommu-ranges\n");
> +			break;
> +		}

[ ... ]

> +	}
> +
> +	if (list_empty(&ranges))
> +		return;

[Severity: Medium]
If parsing fails and the loop breaks early, or if the ranges evaluate to
0 length, the ranges list will be empty and this path returns early without
deriving any reserved regions. Does this silently fail-open and grant the
device unrestricted IOVA access instead of restricting it?

> +
> +	if (of_dma_is_coherent(dev->of_node))
> +		prot |= IOMMU_CACHE;
> +
> +	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);
> +			if (region)
> +				list_add_tail(&region->list, list);
> +		}
> +		if (pos->end + 1 > next)
> +			next = pos->end + 1;

[Severity: High]
If an explicitly allowed iommu-ranges entry extends to the end of the
physical address space (for example, 0xFFFFFFFF on a 32-bit system), could
pos->end + 1 wrap around to 0?

If it wraps to 0, the condition (0 > next) evaluates to false, leaving
the next variable stale.

> +
> +		list_del(&pos->node);
> +		kfree(pos);
> +	}
> +
> +	region = iommu_alloc_resv_region(next, ~(phys_addr_t)0 - next + 1,
> +					 prot, IOMMU_RESV_RESERVED, GFP_KERNEL);

[Severity: High]
If next is left stale due to the wrap-around above, will this final
allocation use the stale next value to reserve the remainder of the address
space, erroneously covering the explicitly allowed IOVA space?

> +	if (region)
> +		list_add_tail(&region->list, list);
> +}
> +
>  /**
>   * of_iommu_get_resv_regions - reserved region driver helper for device tree

[ ... ]

> @@ -214,10 +298,14 @@ void of_iommu_get_resv_regions(struct device *dev, struct list_head *list)
>  
>  		memset(&phys, 0, sizeof(phys));
>  
> +		maps = of_get_property(it.node, "iommu-addresses", &size);
> +		if (!maps)
> +			continue;
> +
>  		/*
> -		 * 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);
> @@ -226,11 +314,11 @@ void of_iommu_get_resv_regions(struct device *dev, struct list_head *list)
>  					it.node, err);
>  				continue;
>  			}
> -		}
> -
> -		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: High]
Does this explicitly reject legacy Device Tree nodes that lack a reg property?
Existing Device Trees using iommu-addresses to describe pure IOVA reservations
without a reg property might fail to parse here, leaving the IOVA space
unreserved. Could this break backward compatibility and violate the DT stable
ABI guarantee?

>  		end = maps + size / sizeof(__be32);
>  
>  		while (maps < end) {
>  			struct device_node *np;
>  			u32 phandle;
>  
>  			phandle = be32_to_cpup(maps++);

[Severity: High]
This is a pre-existing issue, but if of_translate_dma_region() returns NULL
during the previous iteration, does this dereference a NULL pointer?

>  			np = of_find_node_by_phandle(phandle);
>  
>  			if (np == dev->of_node) {
>  				int prot = IOMMU_READ | IOMMU_WRITE;
>  				struct iommu_resv_region *region;
>  				enum iommu_resv_type type;
>  				phys_addr_t iova;
>  				size_t length;
>  
>  				if (of_dma_is_coherent(dev->of_node))
>  					prot |= IOMMU_CACHE;
>  
>  				maps = of_translate_dma_region(np, maps, &iova, &length);

[Severity: High]
This is a pre-existing issue, but does this overwrite the loop iterator maps
with a potentially NULL return value without validation? If it returns NULL
due to a parsing error, the next iteration will evaluate maps < end as true
since NULL is less than the end pointer boundary.

[Severity: High]
Also a pre-existing issue, but if np does not match dev->of_node, the code
skips processing the region. Is there a missing else block here to advance the
maps pointer over the iova and length cells?

Without it, the next iteration will read the first cell of the ignored iova
as the next phandle, throwing the parser out of sync and causing subsequent
reservations to be silently ignored.

> @@ -258,6 +346,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;

[Severity: High]
Does this drop any pure IOMMU_RESV_RESERVED regions that were previously
supported and parseable before this patch?

> +
>  				region = iommu_alloc_resv_region(iova, length, prot, type,
>  								 GFP_KERNEL);
>  				if (region)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-vpu_iommu_iova_handling-v3-0-2fe43ae45495@oss.qualcomm.com?part=4