Re: [PATCH v2 1/3] of: reserved_mem: avoid unnecessary memory allocation when __reserved_mem_check_root() fails

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Error paths in alloc_reserved_mem_array() fail to reset total_reserved_mem_cnt, causing a deterministic buffer overflow.
--

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

of: reserved_mem: avoid unnecessary memory allocation when __reserved_mem_check_root() fails

This commit delays the dynamic allocation of the reserved memory array
until after checking the root node format. It aims to prevent memory
allocation and potential waste if the reserved memory root format is
unsupported.

> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -265,14 +265,14 @@ void __init fdt_scan_reserved_mem_late(void)
[ ... ]
>  	if (__reserved_mem_check_root(node)) {
>  		pr_err("Reserved memory: unsupported node format, ignoring\n");
>  		return;
>  	}
>  
> +	/* Attempt dynamic allocation of a new reserved_mem array */
> +	alloc_reserved_mem_array();

This isn't a bug introduced by this commit, but while looking at the
relocation of alloc_reserved_mem_array(), I noticed a pre-existing flaw
in its error paths.

If total_reserved_mem_cnt exceeds MAX_RESERVED_REGIONS (64), we attempt
to allocate a new array. If memblock_alloc() fails or alloc_size == SIZE_MAX,
alloc_reserved_mem_array() logs an error and returns early without resetting
total_reserved_mem_cnt back to MAX_RESERVED_REGIONS.

Since alloc_reserved_mem_array() returns void and doesn't propagate the
error, fdt_scan_reserved_mem_late() continues iterating over the remaining
nodes and calling fdt_init_reserved_mem_node().

Does this lead to an out-of-bounds write?

In fdt_init_reserved_mem_node(), the bounds check compares against the active
total_reserved_mem_cnt:

drivers/of/of_reserved_mem.c:fdt_init_reserved_mem_node() {
    ...
    if (reserved_mem_count == total_reserved_mem_cnt) {
        pr_err("not enough space for all defined regions.\n");
        return -ENOSPC;
    }
    ...
}

If alloc_reserved_mem_array() failed, reserved_mem still points to the
static 64-element array, but total_reserved_mem_cnt is still set to the
larger requested size. This appears to allow reserved_mem_count to exceed
64, deterministically writing past the end of the static array.

Could the error paths in alloc_reserved_mem_array() be updated to always
reset total_reserved_mem_cnt to MAX_RESERVED_REGIONS?

> +
>  	fdt_for_each_subnode(child, fdt, node) {
>  		const char *uname;
>  		u64 b, s;

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