Re: [PATCH v3 2/2] of: reserved_mem: allocate and map the reserved_mem array early

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] of: memory corruption due to initializing unreserved memory regions
- [Medium] of: memory leak on dynamically-placed nomap region init failure
--

commit cc45d1ed473ae1f1390908145c8217f0d9614ac0
Author: Marek Szyprowski <m.szyprowski@samsung.com>

    of: reserved_mem: allocate and map the reserved_mem array early
    
    Get rid of the static, limited-size reserved_mem array and replace it
    with array allocated by memblock_alloc_raw() and accessed directly or
    through a temporary early_memremap() mapping.

    Link: https://lore.kernel.org/all/asc6brs_g0ky0wOM@kernel.org/

> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 300fb236ab59f..0fd1683cba011 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -381,143 +384,109 @@ static void __init __rmem_check_for_overlap(void)
[ ... ]
> +	/* Second pass: initialize the static regions */
> +	fdt_for_each_subnode(child, fdt, node) {
>  		if (!of_fdt_device_is_available(fdt, child))
>  			continue;
>  
> -		uname = fdt_get_name(fdt, child, NULL);
> -
> -		err = __reserved_mem_reserve_reg(child, uname);
> -		if (!err)
> -			count++;
> +		__reserved_mem_init_reg(child, fdt_get_name(fdt, child, NULL));
> +	}

[Severity: High]
Does this unconditionally initialize memory regions even if they failed
reservation in the first pass?

If __reserved_mem_reserve_reg() fails for a region, it sets
reserved_mem_reserve_failed = true but does not exclude the node. When
__reserved_mem_init_reg() is called here, it will invoke
fdt_init_reserved_mem_node() for that node anyway.

Could this lead to blindly adding unreserved physical memory to the
reserved_mem array, allowing drivers to silently overwrite standard memory
that the page allocator considers free?

Additionally, if the node initialization subsequently fails (e.g. due to a
missing compatible match), could it execute memblock_phys_free() or
memblock_clear_nomap() on memory it never successfully owned?

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;
    ...
}

>  
> -		/*
> -		 * Save the nodes for the dynamically-placed regions
> -		 * into an array which will be used for allocation right
> -		 * after all the statically-placed regions are reserved
> -		 * 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))
> -			continue;
> +	/*
> +	 * Allocate the dynamically-placed regions only after all the
> +	 * statically-placed ones are reserved, to avoid overlapping with them.
> +	 */
> +	fdt_for_each_subnode(child, fdt, node) {
> +		int len;
>  
> -		if (dynamic_nodes_cnt == MAX_RESERVED_REGIONS) {
> -			pr_err("too many defined dynamic regions, skip '%s'\n",
> -			       uname);
> +		if (!of_fdt_device_is_available(fdt, child) ||
> +		    (of_flat_dt_get_addr_size_prop(child, "reg", &len) && len))
>  			continue;
> -		}
>  
> -		dynamic_nodes[dynamic_nodes_cnt] = child;
> -		dynamic_nodes_cnt++;
> -	}
> -	for (int i = 0; i < dynamic_nodes_cnt; i++) {
> -		const char *uname;
> -		int err;
> -
> -		child = dynamic_nodes[i];
> -		uname = fdt_get_name(fdt, child, NULL);
> -		err = __reserved_mem_alloc_size(child, uname);
> -		if (!err)
> -			count++;
> +		__reserved_mem_alloc_size(child,
> +					  fdt_get_name(fdt, child, NULL));
>  	}

[Severity: Medium]
If a dynamically-placed 'no-map' reserved memory region fails initialization
here, does it permanently leak memory from memblock.reserved?

When __reserved_mem_alloc_size() allocates memory via
memblock_phys_alloc_range(), the memory is added to memblock.reserved. It
then calls fdt_init_reserved_mem_node(), which has the following error
handling path:

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;
    ...
}

For 'no-map' regions, this clears the nomap flag but appears to omit
memblock_phys_free(rmem->base, rmem->size), which would leave the dynamically
allocated region permanently trapped in memblock.reserved.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015082315.56876-1-m.szyprowski@samsung.com?part=2