Re: [PATCH 1/3] of: reserved_mem: skip init for regions whose early reservation failed

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Exceeding `MAX_RESERVED_REGIONS` failed memory reservations causes unrecorded failures to steal dynamic array slots from valid nodes, leading to skipped valid regions and potential arbitrary physical memory frees.
- [High] Redundant recording of natively skipped zero-sized nodes wastes capacity in the bounded `reserve_failed_nodes` array, artificially accelerating array slot stealing overflows.
- [High] Unconditionally skipping initialization for regions whose early memory reservation fails breaks valid legacy configurations.
--

--- Patch [1]: [PATCH 1/3] of: reserved_mem: skip init for regions whose early reservation failed ---
commit 89c6cc50df5c4bf222e4058d5f0ce76a20439009
Author: Wandun Chen <chenwandun@lixiang.com>

of: reserved_mem: skip init for regions whose early reservation failed

The patch modifies reserved memory initialization to propagate errors from
early memory reservation. It tracks nodes that failed to reserve and skips
them during the late scan, preventing devices from using memory that was
not actually protected.

> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 8c9d6395d6a39..f20747725de42 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c

[ ... ]

> @@ -167,14 +180,19 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
>  	base = b;
>  	size = s;
>  
> -	if (size && early_init_dt_reserve_memory(base, size, nomap) == 0) {
> -		fdt_fixup_reserved_mem_node(node, base, size);
> -		pr_debug("Reserved memory: reserved region for node '%s': base %pa, size %lu MiB\n",
> -			 uname, &base, (unsigned long)(size / SZ_1M));
> -	} else {
> +	if (!size)
> +		return -EINVAL;

[Severity: High]
Does returning -EINVAL here needlessly waste slots in the reserve_failed_nodes
array?

Since fdt_scan_reserved_mem_late() in drivers/of/of_reserved_mem.c naturally
skips zero-sized nodes with its own if (!size) continue check before
evaluating the failure array, returning an error here will consume one of the
limited slots in reserve_failed_nodes for a node that doesn't need tracking.

> +
> +	err = early_init_dt_reserve_memory(base, size, nomap);
> +	if (err) {
>  		pr_err("Reserved memory: failed to reserve memory for node '%s': base %pa, size %lu MiB\n",
>  		       uname, &base, (unsigned long)(size / SZ_1M));
> +		return err;

[Severity: High]
Does propagating the -EBUSY error from early_init_dt_reserve_memory() break
valid legacy configurations?

If a region is validly pre-reserved by EFI runtime services, a standard
/memreserve/ entry, or a bootloader, early_init_dt_reserve_memory() correctly
returns -EBUSY. 

Prior to this patch, the error was ignored, allowing the node to be
initialized so devices could still parse its properties like no-map or
reusable. By returning the error here and skipping the node during
fdt_scan_reserved_mem_late(), these valid regions are completely removed
from the subsystem, which could cause legitimate device drivers to fail to
probe.

>  	}
> +
> +	fdt_fixup_reserved_mem_node(node, base, size);

[ ... ]

> @@ -357,8 +379,15 @@ int __init fdt_scan_reserved_mem(void)
>  		uname = fdt_get_name(fdt, child, NULL);
>  
>  		err = __reserved_mem_reserve_reg(child, uname);
> -		if (!err)
> +		if (!err) {
>  			count++;
> +		} else if (err != -ENOENT) {
> +			if (reserve_failed_nodes_cnt < MAX_RESERVED_REGIONS)
> +				reserve_failed_nodes[reserve_failed_nodes_cnt++] = child;
> +			else
> +				pr_err("too many failed regions, '%s' reservation failed\n",
> +				       uname);
> +		}

[Severity: Critical]
Can overflowing the reserve_failed_nodes array cause arbitrary physical memory
frees and valid device reservations being skipped?

If more than the maximum limit of statically placed reserved memory regions fail
their early reservation, the reserve_failed_nodes array overflows. These
overflowed nodes are not recorded, and their count is not added to
total_reserved_mem_cnt.

During fdt_scan_reserved_mem_late() in drivers/of/of_reserved_mem.c, the array
is dynamically allocated based on total_reserved_mem_cnt. However, the
unrecorded failed nodes bypass the skip check:

drivers/of/of_reserved_mem.c:fdt_scan_reserved_mem_late() {
    ...
		if (reserved_mem_node_reserve_failed(child))
			continue;
    ...
}

These nodes then steal array slots from valid regions that appear later. If
the unrecorded region's driver node_init callback subsequently fails, the
error path in fdt_init_reserved_mem_node() will call memblock_phys_free()
on the region:

drivers/of/of_reserved_mem.c:fdt_init_reserved_mem_node() {
    ...
		if (nomap)
			memblock_clear_nomap(rmem->base, rmem->size);
		else
			memblock_phys_free(rmem->base, rmem->size);
    ...
}

Since the region originally failed reservation, this memory is owned by
another subsystem, resulting in an arbitrary free of in-use memory.

>  
>  		/*
>  		 * Save the nodes for the dynamically-placed regions

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