Re: [PATCH v2 3/5] of: reserved_mem: reject statically placed regions overlapping existing reservations

[Wandun <chenwandun1@gmail.com>]

On 8/18/26 17:41, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> 
> New issues:
> - [High] Rejecting overlapping memblock reservations up-front breaks platforms where memory is legitimately reserved early (e.g., by EFI or architecture code) and then described by a DT reserved-memory node.
> - [High] An array overflow in `reserve_failed_nodes` bypasses the failed-node skip logic, leading to memory corruption.
> 
> Pre-existing issues:
> - [Critical] Dynamic allocation of reserved memory regions ignores internal failures, leading to initialization of freed memory.
> - [High] The patch fails to prevent overlaps with `/memreserve/` entries, leaving the memory corruption bug unfixed for those regions due to execution ordering.
> - [High] Dynamically allocated `nomap` reserved memory regions are permanently leaked if their initialization fails.
> --
> 
> --- Patch [3]: [PATCH v2 3/5] of: reserved_mem: reject statically placed regions overlapping existing reservations ---
> commit 1c8f1d8c03461ed83a10e5698047d85d76387f8b
> Author: Wandun Chen <chenwandun@lixiang.com>
> 
> of: reserved_mem: reject statically placed regions overlapping existing reservations
> 
> This patch rejects overlapping memblock reservations up front for
> statically placed regions, preventing them from corrupting memory if
> initialization fails. Dynamically allocated regions are unaffected.
> 
> Link: https://sashiko.dev/#/message/20260806100605.2C2C01F000E9%40smtp.kernel.org [1]
> 
>> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
>> index f25dc0cca2f06..71a879628c4aa 100644
>> --- a/drivers/of/of_reserved_mem.c
>> +++ b/drivers/of/of_reserved_mem.c
>> @@ -158,6 +158,11 @@ static int __init early_init_dt_reserve_memory(phys_addr_t base,
>>  
>>  		return memblock_mark_nomap(base, size);
>>  	}
>> +
>> +	if (memblock_is_region_reserved(base, size) ||
>> +	    memblock_overlaps_nomap(base, size))
>> +		return -EBUSY;
> 
> [Severity: High]
> Will this upfront overlap rejection break platforms where memory is
> legitimately reserved early by EFI or architecture code, and then described
> by a device tree reserved-memory node?
> 
> If architecture code like efi_init() on ARM64 cleanly reserves memory using
> memblock_reserve() before fdt_scan_reserved_mem() executes, the new
> memblock_is_region_reserved() check will evaluate to true and reject the
> node unconditionally with -EBUSY.

An EFI memory-map entry and a DT reserved-memory node may describe the
same physical range, but they serve different purposes and do not provide
an ownership relationship to the kernel. So the kernel cannot safely
treat an existing memblock reservation belonging to the DT node,
especially when the node fail to init and release that reservation.

> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does this overlap check miss
> /memreserve/ entries entirely due to execution ordering?
> 
> In early_init_fdt_scan_reserved_mem(), fdt_scan_reserved_mem() executes
> before the /memreserve/ blocks are iterated and reserved. Because of this,
> when memblock_is_region_reserved() runs here, the /memreserve/ regions are
> not yet known.
> 
> If a reserved-memory node overlaps a /memreserve/, it bypasses the check.
> If the node's driver later fails initialization, its error path executes
> memblock_phys_free(), which will still wrongly free the /memreserve/ memory.
> 
>> +
>>  	return memblock_reserve(base, size);
>>  }
> 
> [Severity: High]
> Can an array overflow in reserve_failed_nodes bypass the failed-node skip
> logic?
> 
> If the MAX_RESERVED_REGIONS limit is exceeded, record_reserve_failed_node()
> returns false. The fallback logic in fdt_scan_reserved_mem() then
> increments count instead of skipping the node:
> 
> drivers/of/of_reserved_mem.c:fdt_scan_reserved_mem() {
>     ...
>     else if (should_record_failed_node &&
>              !record_reserve_failed_node(child, uname))
>         count++;
>     ...
> }
> 
> Later, in fdt_scan_reserved_mem_late(), reserved_mem_node_reserve_failed()
> returns false for this untracked node, passing it to
> fdt_init_reserved_mem_node().
> 
> If initialization fails, the error path frees the memory, potentially
> corrupting an existing overlapping reservation.

If too many nodes init fail, it will fallback to the current behavior.

> 
> [Severity: High]
> This is a pre-existing issue, but do dynamically allocated nomap reserved
> memory regions permanently leak if their initialization fails?
> 
> In early_init_dt_alloc_reserved_memory_arch(), dynamic regions are added to
> memblock.reserved. If driver initialization fails in
> fdt_init_reserved_mem_node(), the error path only clears the nomap flag:
> 
> 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);
>     ...
> }
> 
> This completely omits the memblock_phys_free() call for nomap regions,
> creating a physical memory leak.
> 
patch 04 of this series fixed this issue.

> [Severity: Critical]
> This isn't a bug introduced by this patch, but does the error handling in
> __reserved_mem_alloc_size() ignore internal failures and lead to early boot
> use-after-free issues?
> 
> In early_init_dt_alloc_reserved_memory_arch(), the output parameter
> *res_base is populated before calling memblock_mark_nomap():
> 
> drivers/of/of_reserved_mem.c:early_init_dt_alloc_reserved_memory_arch() {
>     *res_base = base;
>     if (nomap) {
>         err = memblock_mark_nomap(base, size);
>         if (err)
>             memblock_phys_free(base, size);
>     }
>     ...
>     return err;
> }

The reason of memblock_mark_nomap fails only can be out of memory, and this would
trigger panic because memblock is not allowd resize before paging_init.
> 
> If memblock_mark_nomap() fails, it frees the memory but leaves the freed
> physical address in *res_base.
> 
> The caller, __reserved_mem_alloc_size(), ignores the return code and
> checks the base address instead:
> 
> drivers/of/of_reserved_mem.c:__reserved_mem_alloc_size() {
>     ...
>     ret = early_init_dt_alloc_reserved_memory_arch(size, align, 0, 0,
>                                                    nomap, &base);
>     ...
>     if (base == 0) {
>         pr_err("failed to allocate memory...");
>         return -ENOMEM;
>     }
>     ...
> }
> 
> Because base contains the freed address rather than 0, the check passes and
> the freed memory block is incorrectly passed to fdt_init_reserved_mem_node().
>