Re: [PATCH 3/3] 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] Initialization failure for statically defined `no-map` reserved memory regions unsafely returns memory to the buddy allocator without it being mapped in the direct map.
- [High] When dynamically allocating a no-map region, if memblock_mark_nomap() fails, early_init_dt_alloc_reserved_memory_arch() frees the memory but leaves *res_base non-zero. The caller __reserved_mem_alloc_size() incorrectly ignores the return code and checks `if (base == 0)`, causing it to proceed with freed memory.
--

--- Patch [3]: [PATCH 3/3] of: reserved_mem: release dynamically allocated no-map region on init failure ---
commit b0edc36fc6542fd03f33cbf2ed696a0cbaa2eebc
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, and this patch
ensures the memory is freed.

Link: https://lore.kernel.org/all/[email protected]/ [1]

> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 67cb60616fc69..d09c29184acf6 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
[ ... ]
> @@ -556,7 +557,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 does checking if base == 0 instead of the
return code lead to a use-after-free or double-free?

Looking at early_init_dt_alloc_reserved_memory_arch(), if memblock_mark_nomap()
fails, it frees the memory but leaves base set to a non-zero address:

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);
    ...
}

Since __reserved_mem_alloc_size() ignores the return value and checks base:

drivers/of/of_reserved_mem.c:__reserved_mem_alloc_size() {
    ...
    ret = early_init_dt_alloc_reserved_memory_arch(... &base);
    ...
    if (base == 0) {
        pr_err(...);
        return -ENOMEM;
    }
    ...
}

Could this cause fdt_init_reserved_mem_node() to be called on freed memory,
and with this patch, subsequently double-free the memory if initialization
fails?

[ ... ]
> @@ -697,7 +699,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 does clearing the nomap flag
for static regions on failure cause a fatal page fault later?

When early_init_dt_reserve_memory() processes static no-map regions, it marks
them MEMBLOCK_NOMAP without reserving them in memblock.reserved:

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

Because of the MEMBLOCK_NOMAP flag, paging_init() skips creating direct map
page tables for this memory.

If driver initialization fails, fdt_init_reserved_mem_node() clears the nomap
flag:

drivers/of/of_reserved_mem.c:fdt_init_reserved_mem_node() {
    ...
    if (nomap)
        memblock_clear_nomap(rmem->base, rmem->size);
    ...
}

Since the memory is now unreserved and no longer marked nomap, will
memblock_free_all() pass this firmware-protected memory to the buddy
allocator, resulting in an unmapped direct-map access crash when the kernel
allocates it?

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