Re: [PATCH 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 1 potential issue(s) to consider:
- [High] of: reserved_mem array allocated before static regions are reserved
--

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

of: reserved_mem: allocate and map the reserved_mem array early

This commit replaces the statically-sized reserved_mem array with a
dynamically allocated array using memblock_alloc_raw(). It accesses the
array through a temporary early mapping so that all regions can be processed
in a single early boot scan.

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
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c

[ ... ]

> @@ -381,143 +376,89 @@ static void __init __rmem_check_for_overlap(void)

[ ... ]

>  int __init fdt_scan_reserved_mem(void)
>  {
> -	int node, child;
> -	int dynamic_nodes_cnt = 0, count = 0;
> -	int dynamic_nodes[MAX_RESERVED_REGIONS];
> +	struct reserved_mem *array, *map;
> +	size_t array_bytes;
> +	int node, child, cnt;
>  	const void *fdt = initial_boot_params;
>  
>  	node = fdt_path_offset(fdt, "/reserved-memory");
> -	if (node < 0) {
> -		total_reserved_mem_cnt = 0;
> +	if (node < 0)
>  		return -ENODEV;
> -	}
>  
>  	if (__reserved_mem_check_root(node) != 0) {
>  		pr_err("Reserved memory: unsupported node format, ignoring\n");
> -		total_reserved_mem_cnt = 0;
>  		return -EINVAL;
>  	}
>  
> -	fdt_for_each_subnode(child, fdt, node) {
> -		const char *uname;
> -		int err;
> +	/* First pass: count the entries needed for the reserved_mem array */
> +	cnt = count_reserved_mem_nodes(fdt, node);
> +	if (!cnt)
> +		return 0;
>  
> -		if (!of_fdt_device_is_available(fdt, child))
> -			continue;
> +	array_bytes = array_size(cnt, sizeof(*array));
> +	array = memblock_alloc_raw(array_bytes, SMP_CACHE_BYTES);

[Severity: High]
Could this allocate the array inside one of the statically-placed reserved
memory regions?

At the time of this memblock_alloc_raw() call, statically-placed reserved
memory regions (defined by the reg property) have not yet been marked as
reserved in memblock. The allocator considers the physical memory intended
for these static regions as available.

> +	if (!array) {
> +		pr_err("Failed to allocate memory for reserved_mem array\n");
> +		return -ENOMEM;
> +	}

[ ... ]

> +	reserved_mem = map;
> +	total_reserved_mem_cnt = cnt;
>  
> -		uname = fdt_get_name(fdt, child, NULL);
> +	/* Second pass: reserve and initialize the static regions */
> +	fdt_for_each_subnode(child, fdt, node) {
> +		if (!of_fdt_device_is_available(fdt, child))
> +			continue;
>  
> -		err = __reserved_mem_reserve_reg(child, uname);
> -		if (!err)
> -			count++;
> +		__reserved_mem_reserve_reg(child,
> +					   fdt_get_name(fdt, child, NULL));

[Severity: High]
If the array was allocated inside a static region above, won't this late
reservation either overlap and fail, or silently succeed?

If it succeeds, it places the kernel's reserved_mem array in physical
memory expected to be exclusively owned by a hardware device or firmware.
When the device later uses this memory, it might overwrite the array,
potentially leading to memory corruption and boot failure.

> +	}

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