Re: [PATCH v9 02/10] of: reserved_mem: Introduce "ll-map" property

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Unreachable rollback logic in error paths attempting to clear the LLMAP flag when NOMAP fails.

Pre-existing issues:
- [High] Failed memory allocations for reserved memory regions are incorrectly treated as successful, leading to use-after-free of physical memory.
--

commit 08fe7ffd815bf0c630423a7e7417a3bec7a82b60
Author: Vincent Donnefort <vdonnefort@google.com>

of: reserved_mem: Introduce "ll-map" property

This commit introduces an "ll-map" property for reserved-memory regions,
allowing them to keep last-level mappings. This helps avoid splitting blocks
when mapping or unmapping pages from the kernel direct map.

> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 8c9d6395d6a39..9ff2d02cb1491 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -46,10 +69,21 @@ static int __init early_init_dt_alloc_reserved_memory_arch(phys_addr_t size,
>  		return -ENOMEM;
>  
>  	*res_base = base;
> -	if (nomap) {
> +	if (flags & OF_RMEM_LLMAP) {
> +		err = memblock_mark_llmap(base, size);
> +		if (err) {
> +			memblock_phys_free(base, size);
> +			return err;
> +		}
> +	}

[Severity: High]
This is a pre-existing issue, but does this error path lead to a use-after-free
of physical memory?

The output parameter *res_base is populated with the physical address before
the failure paths free the memory. If memblock_mark_llmap() (or
memblock_mark_nomap()) fails, memblock_phys_free() is called and an error
code is returned.

However, looking at the caller __reserved_mem_alloc_size():

drivers/of/of_reserved_mem.c:__reserved_mem_alloc_size() {
    ...
    ret = early_init_dt_alloc_reserved_memory_arch(size, align,
                                                   0, 0, flags, &base);
    ...
    if (base == 0) {
        pr_err("failed to allocate memory for node '%s': size %lu MiB\n",
               uname, (unsigned long)(size / SZ_1M));
        return -ENOMEM;
    }
    
    fdt_fixup_reserved_mem_node(node, base, size);
    fdt_init_reserved_mem_node(node, uname, base, size);
    ...
}

Because __reserved_mem_alloc_size() checks if (base == 0) instead of
checking the returned error code ret, the check passes (since base was
populated), and the freed memory is subsequently registered as a valid
reserved memory region.

> +
> +	if (flags & OF_RMEM_NOMAP) {
>  		err = memblock_mark_nomap(base, size);
> -		if (err)
> +		if (err) {
> +			if (flags & OF_RMEM_LLMAP)
> +				memblock_clear_llmap(base, size);

[Severity: Low]
Is this rollback logic reachable?

Since of_reserved_mem_flags() enforces that OF_RMEM_NOMAP and
OF_RMEM_LLMAP are strictly mutually exclusive (returning -EINVAL if both
are set), it doesn't seem possible for flags to have both bits set
simultaneously.

Therefore, the if (flags & OF_RMEM_LLMAP) block inside the
if (flags & OF_RMEM_NOMAP) condition appears to be dead code.

>  			memblock_phys_free(base, size);
> +		}
>  	}
>  
>  	if (!err)

[ ... ]

> @@ -119,17 +153,29 @@ static int fdt_fixup_reserved_mem_node(unsigned long node,
>  				       phys_addr_t base, phys_addr_t size);
>  
>  static int __init early_init_dt_reserve_memory(phys_addr_t base,
> -					       phys_addr_t size, bool nomap)
> +					       phys_addr_t size,
> +					       enum of_rmem_flags flags)
>  {
> -	if (nomap) {
> +	int err;
> +
> +	if (flags & OF_RMEM_LLMAP) {
> +		err = memblock_mark_llmap(base, size);
> +		if (err)
> +			return err;
> +	}
> +
> +	if (flags & OF_RMEM_NOMAP) {
>  		/*
>  		 * If the memory is already reserved (by another region), we
>  		 * should not allow it to be marked nomap, but don't worry
>  		 * 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)) {
> +			if (flags & OF_RMEM_LLMAP)
> +				memblock_clear_llmap(base, size);

[Severity: Low]
Similar to the above, is this rollback logic reachable?

Since OF_RMEM_NOMAP and OF_RMEM_LLMAP are mutually exclusive, this
cleanup code inside the OF_RMEM_NOMAP block should never execute.

>  			return -EBUSY;
> +		}
>  
>  		return memblock_mark_nomap(base, size);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902104712.2399797-1-vdonnefort@google.com?part=2