Re: [PATCH v2 2/3] of: reserved_mem: clean up redundant alloc_reserved_mem_array() call

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Missing bounds check on `dynamic_nodes_cnt` in `fdt_scan_reserved_mem()` allows out-of-bounds writes to the stack array `dynamic_nodes`.
- [High] Error handling in `alloc_reserved_mem_array()` fails to reset `total_reserved_mem_cnt` on allocation failure, leading to a buffer overflow on the static `reserved_mem_array`.
--

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

of: reserved_mem: clean up redundant alloc_reserved_mem_array() call

This commit skips calling alloc_reserved_mem_array() if total_reserved_mem_cnt
is 0 after fdt_scan_reserved_mem(). This avoids executing dynamic allocation
logic when there are no reserved memory regions to save.

> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 7856dc857d650..462e7c3078a3a 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -270,6 +270,9 @@ void __init fdt_scan_reserved_mem_late(void)
>  		return;
>  	}
>  
> +	if (!total_reserved_mem_cnt)
> +		return;
> +
>  	/* Attempt dynamic allocation of a new reserved_mem array */
>  	alloc_reserved_mem_array();

This problem wasn't introduced by this patch, but if alloc_reserved_mem_array()
fails (e.g. memblock_alloc() fails or alloc_size == SIZE_MAX), it returns
without resetting total_reserved_mem_cnt to MAX_RESERVED_REGIONS.

Because total_reserved_mem_cnt can exceed MAX_RESERVED_REGIONS, and
reserved_mem would still point to the static reserved_mem_array, would the
boundary check in fdt_init_reserved_mem_node() fail to stop insertions at
MAX_RESERVED_REGIONS?

Can this result in an out-of-bounds write into the static array?

Also not introduced by this patch, there appears to be a missing bounds
check in fdt_scan_reserved_mem():

drivers/of/of_reserved_mem.c:fdt_scan_reserved_mem() {
	int dynamic_nodes_cnt = 0, count = 0;
	int dynamic_nodes[MAX_RESERVED_REGIONS];
	...
		if (err == -ENOENT && of_get_flat_dt_prop(child, "size", NULL)) {
			dynamic_nodes[dynamic_nodes_cnt] = child;
			dynamic_nodes_cnt++;
		}
	...
}

If a device tree contains more dynamically sized reserved memory nodes than
MAX_RESERVED_REGIONS, could this overwrite the bounds of the dynamic_nodes
stack array?

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