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

New issues:
- [High] Breaks backward compatibility for existing Device Trees using iommu-addresses without a reg property.
- [High] Silent fail-open behavior grants full IOVA access if iommu-ranges parsing or allocation fails.
- [High] Truncation of the remaining reserved region length on 32-bit systems with LPAE (64-bit phys_addr_t).
- [High] Mismatched direct mappings are discarded instead of being reserved, contradicting the emitted kernel warning.
- [High] of_translate_dma_region is called without validating that the property buffer contains enough remaining cells.
- [High] of_translate_dma_region improperly translates IOVAs into CPU physical addresses, resulting in incorrect IOMMU reservations.
- [High] The overflow check iova + length < iova incorrectly rejects a valid IOVA range that extends exactly to the maximum address.

Pre-existing issues:
- [High] Misparsing of multi-device iommu-addresses property due to incorrect iterator advancement.
- [High] Unchecked return value from of_translate_dma_region leads to a NULL pointer dereference.
--

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

    iommu: of_iommu: Add support for "iommu-ranges" on a device node
    
    Reserving an IOVA address using "iommu-addresses" requires it to be
    placed in the reserved-memory node. But when "iommu-addresses" is the
    only property being described and there is no backing "reg" (i.e. no
    actual reserved system memory), it does not really belong under
    /reserved-memory. A device IOVA range is specific to its own address
    space, and does not describe the system physical memory to make it
    qualify under reserved-memory. Given this, place "iommu-addresses"
    inside reserved-memory only when it is paired with a "reg", otherwise,
    define it within the device own node. It was discussed here:
    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..baf74db838d6b 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -190,6 +190,98 @@ 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
> + * @dev: device for which to get reserved regions
> + * @list: reserved region list
> + *
> + * A device can describe its own usable IOVA ranges directly on its node
> + * via "iommu-ranges". Everything not under those ranges is derived
> + * as a reserved region so the IOMMU allocator won't use it. Entries may
> + * appear in any order in the property.
> + */
> +static void of_iommu_derive_resv_regions(struct device *dev, struct list_head *list)
> +{
> +	struct of_iommu_range {
> +		struct list_head node;
> +		phys_addr_t start;
> +		phys_addr_t end;
> +	} *pos, *new, *next_range;
> +	int size, prot = IOMMU_READ | IOMMU_WRITE;
> +	struct iommu_resv_region *region;
> +	const __be32 *maps, *end;
> +	phys_addr_t next = 0;
> +	LIST_HEAD(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);

[Severity: High]
Does this code validate that the property buffer has enough remaining cells
before reading them? While the loop checks maps < end in
of_iommu_derive_resv_regions(), of_translate_dma_region() blindly reads
na + ns cells without bounds checking. Could a malformed iommu-ranges
property cause an out-of-bounds read of kernel memory?

Also, does this function incorrectly translate IOVAs into CPU physical
addresses? The iommu-ranges property specifies usable device IOVA ranges,
but of_translate_dma_region() translates the address through the parent
bus dma-ranges into a CPU physical address. Since IOMMU reserved regions
operate in the IOVA space, doesn't this corrupt the reservation targets
and leave the actual IOVA space unprotected?

> +		if (!maps) {
> +			dev_err(dev, "%pOF: failed to parse iommu-ranges\n",
> +				dev->of_node);
> +			break;
> +		}
> +
> +		if (!length)
> +			continue;
> +
> +		if (iova + length < iova) {

[Severity: High]
Does this overflow check reject a valid IOVA range that extends exactly to
the maximum physical address? In of_iommu_derive_resv_regions(), if a
valid hardware IOVA mapping spans exactly to the end of the address space
limit, iova + length will wrap around to exactly 0. Since 0 < iova
evaluates to true, wouldn't this falsely reject the valid maximum-address
range as an overflow and leave that IOVA space completely unprotected?

> +			dev_err(dev, "%pOF: iommu-ranges overflows address space\n",
> +				dev->of_node);
> +			continue;
> +		}
> +
> +		list_for_each_entry(pos, &ranges, node)
> +			if (pos->start > iova)
> +				break;
> +
> +		new = kmalloc_obj(*new);
> +		if (!new)
> +			continue;
> +
> +		new->start = iova;
> +		new->end = iova + length;
> +		list_add_tail(&new->node, &pos->node);
> +	}
> +
> +	if (list_empty(&ranges))
> +		return;

[Severity: High]
Does this fail-open instead of fail-secure if iommu-ranges parsing or
memory allocation fails? If of_translate_dma_region() returns NULL for a
malformed property or kmalloc_obj() fails in
of_iommu_derive_resv_regions(), the loop breaks or continues, leaving the
ranges list empty. This early return would then exit without adding any
reserved regions, effectively telling the IOMMU that the entire address
space is usable 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 > next || !pos->end)
> +			next = pos->end;
> +
> +		list_del(&pos->node);
> +		kfree(pos);
> +	}
> +
> +	if (!next)
> +		return;
> +
> +	region = iommu_alloc_resv_region(next, ~(phys_addr_t)0 - next + 1,
> +					 prot, IOMMU_RESV_RESERVED, GFP_KERNEL);

[Severity: High]
Can this truncate the length calculation on 32-bit systems with LPAE?
In of_iommu_derive_resv_regions(), the remaining address space length is
calculated as ~(phys_addr_t)0 - next + 1. On 32-bit LPAE systems,
phys_addr_t is 64-bit, but iommu_alloc_resv_region() expects a size_t
for length, which is 32-bit. Wouldn't this cast to size_t silently
truncate the 64-bit length, restricting the reservation up to the 4GB
boundary and leaving the remaining 64-bit IOVA space unreserved?

> +	if (region)
> +		list_add_tail(&region->list, list);
> +}

[ ... ]

> @@ -214,10 +306,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 +322,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 break backward compatibility for existing Device Trees?
The previous code in of_iommu_get_resv_regions() explicitly documented
and handled the reg property as optional for IOVA reservations. By
introducing a strict requirement and failing if reg is missing, wouldn't
older Device Trees that omitted reg now fail to parse, resulting in an
ABI breakage?

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

[Severity: High]
This is a pre-existing issue, but does this loop correctly advance the
maps pointer when np != dev->of_node? In of_iommu_get_resv_regions(), if
the current phandle does not match, it skips processing but fails to
advance maps by the size of the unhandled IOVA range (na + ns). Wouldn't
the next iteration then incorrectly read an IOVA cell as a phandle,
causing a parser desync?

>  				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 isn't a bug introduced by this patch, but does the return value of
of_translate_dma_region() need to be checked for NULL? In
of_iommu_get_resv_regions(), if it fails and returns NULL, it is assigned
to maps. The loop condition maps < end will evaluate to true, and the
next iteration will call be32_to_cpup(maps++), resulting in a NULL
pointer dereference.

>  				if (length == 0) {
>  					dev_warn(dev, "Cannot reserve IOVA region of 0 size\n");
>  					continue;
>  				}
>  				type = iommu_resv_region_get_type(dev, &phys, iova, length);
>  
> +				if (type != IOMMU_RESV_DIRECT)
> +					continue;

[Severity: High]
Does this skip creating the reservation when the mapping is not direct?
In of_iommu_get_resv_regions(), the iommu_resv_region_get_type() helper
detects non-direct mappings, warns the user it is treating them as
reservations, and returns IOMMU_RESV_RESERVED. By forcefully skipping
when type is not IOMMU_RESV_DIRECT, doesn't this entirely drop the region
instead of creating the reservation, contradicting the helper's intent
and breaking existing IOVA reservations?

> +
>  				region = iommu_alloc_resv_region(iova, length, prot, type,
>  								 GFP_KERNEL);
>  				if (region)
>  					list_add_tail(&region->list, list);
>  			}
>  		}
>  	}
> +
> +	of_iommu_derive_resv_regions(dev, list);
>  #endif
>  }
>  EXPORT_SYMBOL(of_iommu_get_resv_regions);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914-vpu_iommu_iova_handling-v4-0-9b9074a73c41@oss.qualcomm.com?part=4