Re: [PATCH v2 5/5] of: reserved_mem: retain static no-map memory on init failure

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

Pre-existing issues:
- [High] In `__reserved_mem_alloc_size`, an allocation error when marking a region as `NOMAP` is ignored, leading to a use-after-free of memblock memory.
--

--- Patch [5]: [PATCH v2 5/5] of: reserved_mem: retain static no-map memory on init failure ---
commit 35ad8d6e4b86dc63d754a8415950fc7ab54dfce4
Author: Wandun Chen <[email protected]>

of: reserved_mem: retain static no-map memory on init failure

Clear MEMBLOCK_NOMAP only for dynamically allocated regions on init failure.
Static regions are initialized after paging_init() and should retain their
memory states.

Link: https://sashiko.dev/#/message/20260814090305.4C8741F00A3D%40smtp.kernel.org [1]

> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index a0df075a9902e..0c5163ce601ea 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -708,7 +708,7 @@ static void __init fdt_init_reserved_mem_node(unsigned long node, const char *un
>  		pr_info("node %s compatible matching fail\n", rmem->name);
>  		rmem->name = NULL;
>  
> -		if (nomap)
> +		if (dynamic && nomap)
>  			memblock_clear_nomap(rmem->base, rmem->size);
>  
>  		if (dynamic || !nomap)

[Severity: High]
This is a pre-existing issue, but while looking at reserved memory error
handling in this file, I noticed a potential memory corruption when dynamic
allocation fails.

In early_init_dt_alloc_reserved_memory_arch(), the base address is assigned
before the nomap check:

base = memblock_phys_alloc_range(size, align, start, end);
...
*res_base = base;
if (nomap) {
        err = memblock_mark_nomap(base, size);
        if (err)
                memblock_phys_free(base, size);
}

If memblock_mark_nomap() fails, the memory is freed, but *res_base still
holds the freed address instead of being cleared.

Then in __reserved_mem_alloc_size(), the code checks the base address instead
of the return value ret:

ret = early_init_dt_alloc_reserved_memory_arch(size, align,
                                               0, 0, nomap, &base);
if (ret == 0)
        pr_debug("...");

if (base == 0) {
        pr_err("failed to allocate memory...");
        return -ENOMEM;
}

Since base contains the freed address instead of 0, does this allow the
freed memory to be registered and later handed out by the buddy allocator
while still being incorrectly used as reserved memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5