Re: [PATCH v2 2/5] of: reserved_mem: reject static regions overlapping no-map memory

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch provides an incomplete fix for overlapping reservations, allowing a static `map` region to silently overlap an existing `no-map` region.
- [High] Error handling in `__reserved_mem_alloc_size()` ignores the return code, leading to a Use-After-Free of physical memory if marking a dynamic `no-map` region fails.
--

--- Patch [2]: [PATCH v2 2/5] of: reserved_mem: reject static regions overlapping no-map memory ---
commit 5bad18fc3e691089fa21e129bf6cd3d6145676c4
Author: Wandun Chen <chenwandun@lixiang.com>

of: reserved_mem: reject static regions overlapping no-map memory

Static no-map reserved-memory regions are marked in memblock.memory rather
than memblock.reserved. So the reservation overlap check does not reject
a static region that overlaps existing no-map memory. Both regions can then
be initialized and hand the same physical memory to different drivers.
This commit rejects static regions that overlap existing no-map memory.

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

> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 491d64d93d707..f25dc0cca2f06 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -152,7 +152,8 @@ static int __init early_init_dt_reserve_memory(phys_addr_t base,
>  		 * if the region isn't memory as it won't be mapped.
>  		 */
>  		if (memblock_overlaps_region(&memblock.memory, base, size) &&
> -		    memblock_is_region_reserved(base, size))
> +		    (memblock_is_region_reserved(base, size) ||
> +		     memblock_overlaps_nomap(base, size)))
>  			return -EBUSY;
>  
>  		return memblock_mark_nomap(base, size);

[Severity: High]
This isn't a bug introduced by this patch, but since this patch is fixing
overlap checks, does the !nomap path still allow a static map region to
silently overlap an existing no-map region?

If a Device Tree contains a no-map region followed by a regular map region
pointing to the same physical address, the no-map region is processed first.

When the map region is processed, nomap is false, so it bypasses the
overlap check updated here and directly calls memblock_reserve():

    drivers/of/of_reserved_mem.c:early_init_dt_reserve_memory() {
        if (nomap) {
            ...
            return memblock_mark_nomap(base, size);
        }
        return memblock_reserve(base, size);
    }

Since memblock_reserve() adds the region to memblock.reserved without
checking for MEMBLOCK_NOMAP overlaps in memblock.memory, both regions
can be initialized. This would hand the same physical memory to different
reserved-memory drivers.

Should the !nomap path also check for overlaps against existing no-map
memory?


While reviewing the memory allocation paths, I also noticed a pre-existing
issue in __reserved_mem_alloc_size().

[Severity: High]
This is a pre-existing issue, but could this error handling cause a
use-after-free of physical memory if memblock_mark_nomap() fails?

Looking at early_init_dt_alloc_reserved_memory_arch():

    drivers/of/of_reserved_mem.c:early_init_dt_alloc_reserved_memory_arch() {
        ...
        base = memblock_phys_alloc_range(size, align, start, end);
        if (!base)
            return -ENOMEM;

        *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 allocated memory and returns
the error code, but *res_base (the base pointer in the caller) remains
set to the freed address.

In __reserved_mem_alloc_size(), the return code is ignored:

    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 (ret == 0)
            pr_debug(...);

        if (base == 0) {
            pr_err(...);
            return -ENOMEM;
        }

        fdt_init_reserved_mem_node(node, uname, base, size);
        ...
    }

Because __reserved_mem_alloc_size() ignores ret and only checks
if (base == 0), it will proceed with the freed memory and register it via
fdt_init_reserved_mem_node(). The page allocator will hand this memory to
regular allocations while a reserved-memory driver also uses it.

Should this check be if (ret != 0) or if (base == 0 || ret != 0) to
properly catch allocation failures?

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