Re: [PATCH v2 3/3] of: reserved_mem: only support one <base size> entry in reg property

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unconditional read of the first `reg` property entry without verifying that `len > 0` leads to an out-of-bounds FDT read for empty `reg` properties.
--

commit def0743ccda5f4a97f9b1275e989c8ea559363b4
Author: Wandun Chen <chenwandun1@gmail.com>

of: reserved_mem: only support one <base size> entry in reg property

This commit simplifies reserved memory parsing by restricting support
to a single base/size entry in the reg property. It removes the loop
in the early pass and warns if multiple entries are present,
unconditionally reading only the first entry.

Link: https://lore.kernel.org/all/20260506014752.GA280279-robh@kernel.org/

> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 462e7c3078a3a..0e91c4a71d89d 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -134,37 +134,38 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
[ ... ]
>  	prop = of_flat_dt_get_addr_size_prop(node, "reg", &len);
>  	if (!prop)
>  		return -ENOENT;
>  
> +	if (len > 1)
> +		pr_warn("Reserved memory: node '%s' has %d <base size> entries, only the first is used\n",
> +			uname, len);
> +
>  	nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;
>  
>  	err = fdt_validate_reserved_mem_node(node, NULL);
>  	if (err && err != -ENODEV)
>  		return err;
>  
> -	for (i = 0; i < len; i++) {
> -		u64 b, s;
> -
> -		of_flat_dt_read_addr_size(prop, i, &b, &s);
> -
> -		base = b;
> -		size = s;
> +	of_flat_dt_read_addr_size(prop, 0, &b, &s);

If a device tree node contains an empty reg property,
of_flat_dt_get_addr_size_prop() returns a valid pointer to the empty
property data and sets len to 0.

Since the loop was removed, is there a risk of an out-of-bounds read here
because the code unconditionally calls of_flat_dt_read_addr_size()?

This could fetch garbage values for the physical base and size. If the
parsed garbage size happens to be non-zero, could the kernel attempt
to reserve unintended physical memory regions via
early_init_dt_reserve_memory()?

Would it be safer to verify len > 0 before reading the first entry?

> +	base = b;
> +	size = s;
[ ... ]
> @@ -277,20 +278,24 @@ void __init fdt_scan_reserved_mem_late(void)
>  	alloc_reserved_mem_array();
>  
>  	fdt_for_each_subnode(child, fdt, node) {
> +		const __be32 *prop;
[ ... ]
> -		if (!of_flat_dt_get_addr_size(child, "reg", &b, &s))
> +		prop = of_flat_dt_get_addr_size_prop(child, "reg", &len);
> +		if (!prop)
>  			continue;
>  
>  		ret = fdt_validate_reserved_mem_node(child, NULL);
>  		if (ret && ret != -ENODEV)
>  			continue;
>  
> +		of_flat_dt_read_addr_size(prop, 0, &b, &s);

Does this have the same out-of-bounds read potential when len == 0?

The previous of_flat_dt_get_addr_size() wrapper explicitly checked that
entries != 1, which inherently skipped empty reg properties.

>  		base = b;
>  		size = s;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519082427.4181476-1-chenwandun@lixiang.com?part=3