Re: [PATCH v2 4/5] of: reserved_mem: release dynamically allocated no-map region on init failure

On 8/18/26 17:38, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] `__reserved_mem_alloc_size()` ignores the error return value and checks `base == 0` for failure, leading to Use-After-Free of reserved memory regions.
> - [High] Statically allocated reserved memory is unconditionally freed back to the OS on driver initialization failure, violating the DT specification and causing memory corruption.
> --
> 
> --- Patch [4]: [PATCH v2 4/5] of: reserved_mem: release dynamically allocated no-map region on init failure ---
> commit cf35e3414292da4070c89965dbb1bce8416755e9
> Author: Wandun Chen <[email protected]>
> 
> of: reserved_mem: release dynamically allocated no-map region on init failure
> 
> Dynamically reserved-memory regions are added to memblock.reserved by
> memblock_phys_alloc_range() during __reserved_mem_alloc_size(). When a
> reserved-memory region's driver initialization fails,
> fdt_init_reserved_mem_node() cleans up the reservation. For no-map
> regions it only calls memblock_clear_nomap(), leaving the range in
> memblock.reserved and unavailable for normal memory use.
> 
> Fix it by freeing the region on init failure when it was dynamically
> allocated.
> 
> Sashiko found this issue in [1].
> 
> 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
>> --- a/drivers/of/of_reserved_mem.c
>> +++ b/drivers/of/of_reserved_mem.c
> 
> [ ... ]
> 
>> @@ -567,7 +568,7 @@ static int __init __reserved_mem_alloc_size(unsigned long node, const char *unam
>>  	}
>>  
>>  	fdt_fixup_reserved_mem_node(node, base, size);
>> -	fdt_init_reserved_mem_node(node, uname, base, size);
>> +	fdt_init_reserved_mem_node(node, uname, base, size, true);
>>  
>>  	return 0;
>>  }
> 
> [Severity: High]
> This is a pre-existing issue, but is it possible for
> __reserved_mem_alloc_size() to ignore the error return value from
> early_init_dt_alloc_reserved_memory_arch() and only check if base is 0 for
> failure?
> 
> If early_init_dt_alloc_reserved_memory_arch() fails after setting *res_base
> because memblock_mark_nomap() fails:
> 
> 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);
>         }
>         ...
> }
> 
> The base variable in __reserved_mem_alloc_size() will be non-zero, but the
> memory has already been freed. Since __reserved_mem_alloc_size() only checks
> if base is 0, it passes the error check and registers the freed memory.
> 
> Could this result in a use-after-free if the OS hands the memory out to
> other subsystems while it is still mapped or used by the reserved region?
> 

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

> [ ... ]
> 
>> @@ -707,7 +710,8 @@ static void __init fdt_init_reserved_mem_node(unsigned long node, const char *un
>>  
>>  		if (nomap)
>>  			memblock_clear_nomap(rmem->base, rmem->size);
>> -		else
>> +
>> +		if (dynamic || !nomap)
>>  			memblock_phys_free(rmem->base, rmem->size);
>>  		return;
>>  	} else {
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but could this logic
> unconditionally free statically allocated memory without the no-map
> property back to the general pool?
> 
> If a driver fails to initialize a statically defined reserved memory region,
> and the region does not have the no-map property, it is freed here.

This is pre-existing behavior for static non-no-map regions.
> 
> According to the Device Tree specification, if a driver fails to initialize,
> the OS must not use the memory for general purposes unless the reusable
> property is explicitly present.
> 
> Will this cause kernel memory corruption if the general buddy allocator
> hands this memory out to unrelated kernel subsystems, but the firmware or a
> coprocessor still expects the region to remain reserved and accesses it?
>