Re: [PATCH v2 2/2] of: reserved_mem: print skipped node name when too many regions are defined

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Out-of-bounds write to the static `reserved_mem_array` if `memblock_alloc` fails during dynamic array reallocation.
- [High] Ignored memory reservation failures allow unreserved memory to be successfully registered and handed out as exclusive reserved memory.
- [Medium] Dynamically allocated `no-map` reserved memory regions are leaked from `memblock.reserved` if their initialization fails.
--

commit f95986ab58870e3d79cb320c6dc73ef80982357e
Author: Sang-Heon Jeon <ekffu200098@gmail.com>

of: reserved_mem: print skipped node name when too many regions are defined

When too many reserved memory regions are defined,
fdt_init_reserved_mem_node() logs an error and skips the region without
naming the node.

Print the node name so the skipped node can be identified, matching
fdt_scan_reserved_mem().

> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 499b769e98096..df3d90cb8dbe1 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -623,7 +623,8 @@ static void __init fdt_init_reserved_mem_node(unsigned long node, const char *un
>  	struct reserved_mem *rmem = &reserved_mem[reserved_mem_count];
>  
>  	if (reserved_mem_count == total_reserved_mem_cnt) {
> -		pr_err("not enough space for all defined regions.\n");
> +		pr_err("not enough space for all defined regions, skip '%s'\n",
> +		       uname);
>  		return;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does alloc_reserved_mem_array()
fail to reset total_reserved_mem_cnt on error?

If memblock_alloc() fails during dynamic array reallocation, it returns early
without resetting total_reserved_mem_cnt back to its original value. When the
bounds check in fdt_init_reserved_mem_node() is reached later,
reserved_mem_count can increment beyond the statically-sized
reserved_mem_array limit, potentially causing an out-of-bounds write:

drivers/of/of_reserved_mem.c:alloc_reserved_mem_array() {
    ...
	new_array = memblock_alloc(alloc_size, SMP_CACHE_BYTES);
	if (!new_array) {
		pr_err("Failed to allocate memory for reserved_mem array...");
		return;
	}
    ...
}

[Severity: High]
This is a pre-existing issue, but does __reserved_mem_reserve_reg()
unconditionally return 0 even if early_init_dt_reserve_memory() fails?

If memory reservation fails, it logs an error but returns success, allowing
unreserved memory to be successfully registered and later handed out as
exclusive reserved memory:

drivers/of/of_reserved_mem.c:__reserved_mem_reserve_reg() {
    ...
		if (early_init_dt_reserve_memory(base, size, nomap)) {
			pr_err("Reserved memory: failed to reserve...");
		}
	}
	return 0;
}

[Severity: Medium]
This is a pre-existing issue, but does fdt_init_reserved_mem_node() leak
dynamically allocated no-map regions if initialization fails?

If __reserved_mem_init_node() returns an error, the cleanup path for no-map
regions clears the MEMBLOCK_NOMAP flag but skips calling memblock_phys_free(),
which permanently leaks the physical memory from memblock.reserved:

drivers/of/of_reserved_mem.c:fdt_init_reserved_mem_node() {
    ...
	err = __reserved_mem_init_node(rmem, node);
	if (err != 0 && err != -ENODEV) {
		pr_info("node %s compatible matching fail\n", rmem->name);
		rmem->name = NULL;

		if (nomap)
			memblock_clear_nomap(rmem->base, rmem->size);
		else
			memblock_phys_free(rmem->base, rmem->size);
		return;
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260614133807.2165124-1-ekffu200098@gmail.com?part=2