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

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?

[ ... ]

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

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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4