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

[sashiko-bot@kernel.org]

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.

[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.

[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.

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

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().

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