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

On 8/18/26 17:40, [email protected] wrote:
> 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 <[email protected]>
> 
> 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?

patch 03 of this series fixed this issue.

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

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.

> 
> 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?
>