Re: [PATCH v2 1/2] of: reserved_mem: prevent OOB when too many dynamic regions are defined

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Global buffer overflow on static reserved_mem_array when dynamic allocation fails
--

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

of: reserved_mem: prevent OOB when too many dynamic regions are defined

This commit adds bounds checking for dynamically-placed /reserved-memory
subnodes to prevent out-of-bounds writes to the local dynamic_nodes array
in fdt_scan_reserved_mem().

> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 8d5777cb5d1b3..499b769e98096 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
[ ... ]
> @@ -348,10 +349,17 @@ int __init fdt_scan_reserved_mem(void)
>  		 * or marked as no-map. This is done to avoid dynamically
>  		 * allocating from one of the statically-placed regions.
>  		 */
> -		if (err == -ENOENT && of_get_flat_dt_prop(child, "size", NULL)) {
> -			dynamic_nodes[dynamic_nodes_cnt] = child;
> -			dynamic_nodes_cnt++;
> +		if (err != -ENOENT || !of_get_flat_dt_prop(child, "size", NULL))
> +			continue;
> +
> +		if (dynamic_nodes_cnt == MAX_RESERVED_REGIONS) {
> +			pr_err("too many defined dynamic regions, skip '%s'\n",
> +			       uname);
> +			continue;
>  		}
> +
> +		dynamic_nodes[dynamic_nodes_cnt] = child;
> +		dynamic_nodes_cnt++;
>  	}
>  	for (int i = 0; i < dynamic_nodes_cnt; i++) {
>  		const char *uname;

[Severity: Critical]
This is a pre-existing issue, but could the initialization sequence still
leave the kernel vulnerable to a global buffer overflow later on?

In fdt_scan_reserved_mem(), total_reserved_mem_cnt is updated to the
total count of regions, which could exceed MAX_RESERVED_REGIONS due to
statically-placed regions.

Later, fdt_scan_reserved_mem_late() calls alloc_reserved_mem_array() to
dynamically allocate a larger array:

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 with err: %d", -ENOMEM);
        return;
    }
    ...
}

If memblock_alloc() fails, it returns early but does not reset
total_reserved_mem_cnt back to MAX_RESERVED_REGIONS.

When iterating over the regions, fdt_init_reserved_mem_node() relies on
total_reserved_mem_cnt for its bounds checking:

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, skip '%s'\n",
               uname);
        return;
    }
    ...
}

Since total_reserved_mem_cnt remains inflated, this check will fail to protect
the statically-sized reserved_mem_array, leading to an out-of-bounds write.

Should alloc_reserved_mem_array() reset total_reserved_mem_cnt to
MAX_RESERVED_REGIONS on the error path to avoid overflowing the static
reserved_mem_array?

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