[PATCH v2 0/5] of: reserved_mem: several fixes about reserved memory

[PATCH v2 0/5] of: reserved_mem: several fixes about reserved memory

[Wandun Chen]

From: Wandun Chen <chenwandun@lixiang.com>

This series fixes several error-handling issues in the reserved-memory
initialization paths.

Static reserved-memory nodes are reserved during the early DT scan but
initialized later. The first patch prevents a node whose early
reservation failed from reaching the late initialization path.

The following two patches reject overlapping static regions. Without
these checks, overlapping nodes can be initialized over the same
physical memory, result in data corrupt.

The last two patches fix cleanup of no-map regions after driver.

Sashiko reported these issues in [1] [2] [3].

[1] https://sashiko.dev/#/message/20260814090305.4C8741F00A3D%40smtp.kernel.org
[2] https://sashiko.dev/#/message/20260814084718.29C341F000E9%40smtp.kernel.org
[3] https://sashiko.dev/#/message/20260806100605.2C2C01F000E9%40smtp.kernel.org

v1 --> v2:
1. Rework failed-node tracking in patch 1: do not track zero-sized nodes,
   and keep a reserved_mem slot when tracking overflows.
2. Reject static reserved regions overlapping existing no-map regions.
3. Keep MEMBLOCK_NOMAP flag for static no-map regions when init failure.

Wandun Chen (5):
  of: reserved_mem: skip init for regions whose early reservation failed
  of: reserved_mem: reject static regions overlapping no-map memory
  of: reserved_mem: reject statically placed regions overlapping
    existing reservations
  of: reserved_mem: release dynamically allocated no-map region on init
    failure
  of: reserved_mem: retain static no-map memory on init failure

 drivers/of/of_reserved_mem.c | 87 +++++++++++++++++++++++++++++-------
 include/linux/memblock.h     |  1 +
 mm/memblock.c                | 14 ++++++
 3 files changed, 85 insertions(+), 17 deletions(-)

-- 
2.43.0

[PATCH v2 1/5] of: reserved_mem: skip init for regions whose early reservation failed

[Wandun Chen]

From: Wandun Chen <chenwandun@lixiang.com>

__reserved_mem_reserve_reg() discards the error from
early_init_dt_reserve_memory() and returns 0 unconditionally, so the
caller counts the node in total_reserved_mem_cnt and the late scan
initializes it without checking whether the early reservation actually
succeeded. A region whose reservation failed is then handed to a
device assuming the memory is protected.

Propagate the error so failed reservations are no longer counted, and
record the failed nodes so fdt_scan_reserved_mem_late() can skip them.

Recording the failed nodes explicitly is necessary because
fdt_scan_reserved_mem_late() rescans the DT independently. It cannot
tell from memblock whether early reservation succeeded.

The failed-node array is bounded by MAX_RESERVED_REGIONS, the number
of static regions is not bounded by it, so on overflow the extra nodes
fall back to being initialized, which is the current behavior.

Fixes: 8a6e02d0c00e ("of: reserved_mem: Restructure how the reserved memory regions are processed")
Signed-off-by: Wandun Chen <chenwandun@lixiang.com>
---
 drivers/of/of_reserved_mem.c | 65 ++++++++++++++++++++++++++++++------
 1 file changed, 54 insertions(+), 11 deletions(-)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 8c9d6395d6a3..c6e73d710ee1 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -32,6 +32,30 @@ static struct reserved_mem *reserved_mem __refdata = reserved_mem_array;
 static int total_reserved_mem_cnt = MAX_RESERVED_REGIONS;
 static int reserved_mem_count;
 
+static int reserve_failed_nodes[MAX_RESERVED_REGIONS] __initdata;
+static int reserve_failed_nodes_cnt __initdata;
+
+static bool __init reserved_mem_node_reserve_failed(int node)
+{
+	int i;
+
+	for (i = 0; i < reserve_failed_nodes_cnt; i++)
+		if (reserve_failed_nodes[i] == node)
+			return true;
+	return false;
+}
+
+static bool __init record_reserve_failed_node(int node, const char *uname)
+{
+	if (reserve_failed_nodes_cnt == MAX_RESERVED_REGIONS) {
+		pr_err("too many failed regions, '%s' reservation failed\n", uname);
+		return false;
+	}
+
+	reserve_failed_nodes[reserve_failed_nodes_cnt++] = node;
+	return true;
+}
+
 static int __init early_init_dt_alloc_reserved_memory_arch(phys_addr_t size,
 	phys_addr_t align, phys_addr_t start, phys_addr_t end, bool nomap,
 	phys_addr_t *res_base)
@@ -141,7 +165,8 @@ static int __init early_init_dt_reserve_memory(phys_addr_t base,
  * first entry in 'reg' property
  */
 static int __init __reserved_mem_reserve_reg(unsigned long node,
-					     const char *uname)
+					     const char *uname,
+					     bool *should_record_failed_node)
 {
 	phys_addr_t base, size;
 	int len, err;
@@ -149,6 +174,8 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
 	bool nomap;
 	u64 b, s;
 
+	*should_record_failed_node = false;
+
 	prop = of_flat_dt_get_addr_size_prop(node, "reg", &len);
 	if (!prop || !len)
 		return -ENOENT;
@@ -167,14 +194,20 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
 	base = b;
 	size = s;
 
-	if (size && early_init_dt_reserve_memory(base, size, nomap) == 0) {
-		fdt_fixup_reserved_mem_node(node, base, size);
-		pr_debug("Reserved memory: reserved region for node '%s': base %pa, size %lu MiB\n",
-			 uname, &base, (unsigned long)(size / SZ_1M));
-	} else {
+	if (!size)
+		return -EINVAL;
+
+	err = early_init_dt_reserve_memory(base, size, nomap);
+	if (err) {
+		*should_record_failed_node = true;
 		pr_err("Reserved memory: failed to reserve memory for node '%s': base %pa, size %lu MiB\n",
 		       uname, &base, (unsigned long)(size / SZ_1M));
+		return err;
 	}
+
+	fdt_fixup_reserved_mem_node(node, base, size);
+	pr_debug("Reserved memory: reserved region for node '%s': base %pa, size %lu MiB\n",
+		 uname, &base, (unsigned long)(size / SZ_1M));
 	return 0;
 }
 
@@ -306,10 +339,14 @@ void __init fdt_scan_reserved_mem_late(void)
 		base = b;
 		size = s;
 
-		if (size) {
-			uname = fdt_get_name(fdt, child, NULL);
-			fdt_init_reserved_mem_node(child, uname, base, size);
-		}
+		if (!size)
+			continue;
+
+		if (reserved_mem_node_reserve_failed(child))
+			continue;
+
+		uname = fdt_get_name(fdt, child, NULL);
+		fdt_init_reserved_mem_node(child, uname, base, size);
 	}
 
 	/* check for overlapping reserved regions */
@@ -349,6 +386,7 @@ int __init fdt_scan_reserved_mem(void)
 
 	fdt_for_each_subnode(child, fdt, node) {
 		const char *uname;
+		bool should_record_failed_node;
 		int err;
 
 		if (!of_fdt_device_is_available(fdt, child))
@@ -356,9 +394,14 @@ int __init fdt_scan_reserved_mem(void)
 
 		uname = fdt_get_name(fdt, child, NULL);
 
-		err = __reserved_mem_reserve_reg(child, uname);
+		err = __reserved_mem_reserve_reg(child, uname,
+						 &should_record_failed_node);
 		if (!err)
 			count++;
+		else if (should_record_failed_node &&
+			 !record_reserve_failed_node(child, uname))
+			/* Keep a slot for the untracked node's late initialization. */
+			count++;
 
 		/*
 		 * Save the nodes for the dynamically-placed regions
-- 
2.43.0

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

[Wandun Chen]

From: Wandun Chen <chenwandun@lixiang.com>

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 reserved-memory drivers. So reject a static region that overlaps
existing no-map memory.

Sashiko found this issue in [1].

Fixes: 86588296acbf ("fdt: Properly handle "no-map" field in the memory region")
Signed-off-by: Wandun Chen <chenwandun@lixiang.com>
Link: https://sashiko.dev/#/message/20260814084718.29C341F000E9%40smtp.kernel.org [1]
---
 drivers/of/of_reserved_mem.c |  3 ++-
 include/linux/memblock.h     |  1 +
 mm/memblock.c                | 14 ++++++++++++++
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index c6e73d710ee1..9fb2e4c29443 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);
diff --git a/include/linux/memblock.h b/include/linux/memblock.h
index d62db9e776cf..27d68fbb3157 100644
--- a/include/linux/memblock.h
+++ b/include/linux/memblock.h
@@ -148,6 +148,7 @@ int memblock_physmem_add(phys_addr_t base, phys_addr_t size);
 void memblock_trim_memory(phys_addr_t align);
 unsigned long memblock_addrs_overlap(phys_addr_t base1, phys_addr_t size1,
 				     phys_addr_t base2, phys_addr_t size2);
+bool memblock_overlaps_nomap(phys_addr_t base, phys_addr_t size);
 bool memblock_overlaps_region(struct memblock_type *type,
 			      phys_addr_t base, phys_addr_t size);
 bool memblock_validate_numa_coverage(unsigned long threshold_bytes);
diff --git a/mm/memblock.c b/mm/memblock.c
index 9ce86349a29f..4cabfe365ac4 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -202,6 +202,20 @@ memblock_addrs_overlap(phys_addr_t base1, phys_addr_t size1, phys_addr_t base2,
 	return ((base1 < (base2 + size2)) && (base2 < (base1 + size1)));
 }
 
+bool __init memblock_overlaps_nomap(phys_addr_t base, phys_addr_t size)
+{
+	struct memblock_region *region;
+
+	memblock_cap_size(base, &size);
+	for_each_mem_region(region) {
+		if (memblock_is_nomap(region) &&
+		    memblock_addrs_overlap(base, size, region->base, region->size))
+			return true;
+	}
+
+	return false;
+}
+
 bool __init_memblock memblock_overlaps_region(struct memblock_type *type,
 					phys_addr_t base, phys_addr_t size)
 {
-- 
2.43.0

[PATCH v2 3/5] of: reserved_mem: reject statically placed regions overlapping existing reservations

[Wandun Chen]

From: Wandun Chen <chenwandun@lixiang.com>

memblock_reserve() permits overlapping reservations, so a statically
placed region whose 'reg' overlaps an existing one is accepted. When
initialization of a statically placed reserved-memory region
overlapping an existing reservation fails, memblock_phys_free()
returns the overlap to the buddy allocator, corrupting that memory.

Reject the overlap up front. Dynamically allocated regions are
unaffected, as they are allocated from free memory and so cannot
overlap an existing reservation.

Sashiko found this issue in [1].

Fixes: d0b8ed47e83a ("of: reserved_mem: fix reserve memory leak")
Signed-off-by: Wandun Chen <chenwandun@lixiang.com>
Link: https://sashiko.dev/#/message/20260806100605.2C2C01F000E9%40smtp.kernel.org [1]
---
 drivers/of/of_reserved_mem.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 9fb2e4c29443..767ef702392e 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -158,6 +158,11 @@ static int __init early_init_dt_reserve_memory(phys_addr_t base,
 
 		return memblock_mark_nomap(base, size);
 	}
+
+	if (memblock_is_region_reserved(base, size) ||
+	    memblock_overlaps_nomap(base, size))
+		return -EBUSY;
+
 	return memblock_reserve(base, size);
 }
 
-- 
2.43.0

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

[Wandun Chen]

From: Wandun Chen <chenwandun@lixiang.com>

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

Fixes: 7b25995f5319 ("of: of_reserved_mem: mark nomap memory instead of removing")
Signed-off-by: Wandun Chen <chenwandun@lixiang.com>
Link: https://sashiko.dev/#/message/20260806100605.2C2C01F000E9%40smtp.kernel.org [1]
---
 drivers/of/of_reserved_mem.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 767ef702392e..ac5db19dcc93 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -136,7 +136,8 @@ static int __init alloc_reserved_mem_array(void)
 }
 
 static void fdt_init_reserved_mem_node(unsigned long node, const char *uname,
-				       phys_addr_t base, phys_addr_t size);
+				       phys_addr_t base, phys_addr_t size,
+				       bool dynamic);
 static int fdt_validate_reserved_mem_node(unsigned long node,
 					  phys_addr_t *align);
 static int fdt_fixup_reserved_mem_node(unsigned long node,
@@ -352,7 +353,7 @@ void __init fdt_scan_reserved_mem_late(void)
 			continue;
 
 		uname = fdt_get_name(fdt, child, NULL);
-		fdt_init_reserved_mem_node(child, uname, base, size);
+		fdt_init_reserved_mem_node(child, uname, base, size, false);
 	}
 
 	/* check for overlapping reserved regions */
@@ -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;
 }
@@ -676,13 +677,15 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem,
  * @uname: name of the reserved memory node
  * @base: base address of the reserved memory region
  * @size: size of the reserved memory region
+ * @dynamic: whether the region was dynamically allocated
  *
  * This function calls the region-specific initialization function for a
  * reserved memory region and saves all region-specific data to the
  * reserved_mem array to allow of_reserved_mem_lookup() to find it.
  */
 static void __init fdt_init_reserved_mem_node(unsigned long node, const char *uname,
-					      phys_addr_t base, phys_addr_t size)
+					      phys_addr_t base, phys_addr_t size,
+					      bool dynamic)
 {
 	int err = 0;
 	bool nomap;
@@ -708,7 +711,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 {
-- 
2.43.0

[PATCH v2 5/5] of: reserved_mem: retain static no-map memory on init failure

[Wandun Chen]

From: Wandun Chen <chenwandun@lixiang.com>

Static no-map reserved-memory regions are initialized after paging_init().
If initialization fails, MEMBLOCK_NOMAP would be cleared, and the memory
is available to the buddy allocator even though it was excluded from the
kernel linear mapping.

Clear MEMBLOCK_NOMAP only for dynamically allocated regions, which are
initialized before paging_init() and can safely be made available to the
kernel linear mapping.

Sashiko found this issue in [1].

Fixes: 8a6e02d0c00e ("of: reserved_mem: Restructure how the reserved memory regions are processed")
Signed-off-by: Wandun Chen <chenwandun@lixiang.com>
Link: https://sashiko.dev/#/message/20260814090305.4C8741F00A3D%40smtp.kernel.org [1]
---
 drivers/of/of_reserved_mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index ac5db19dcc93..0090fa86dd10 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -709,7 +709,7 @@ static void __init fdt_init_reserved_mem_node(unsigned long node, const char *un
 		pr_info("node %s compatible matching fail\n", rmem->name);
 		rmem->name = NULL;
 
-		if (nomap)
+		if (dynamic && nomap)
 			memblock_clear_nomap(rmem->base, rmem->size);
 
 		if (dynamic || !nomap)
-- 
2.43.0

Re: [PATCH v2 5/5] of: reserved_mem: retain static no-map memory on init failure

[sashiko-bot]

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

Pre-existing issues:
- [High] In `__reserved_mem_alloc_size`, an allocation error when marking a region as `NOMAP` is ignored, leading to a use-after-free of memblock memory.
--

--- Patch [5]: [PATCH v2 5/5] of: reserved_mem: retain static no-map memory on init failure ---
commit 35ad8d6e4b86dc63d754a8415950fc7ab54dfce4
Author: Wandun Chen <chenwandun@lixiang.com>

of: reserved_mem: retain static no-map memory on init failure

Clear MEMBLOCK_NOMAP only for dynamically allocated regions on init failure.
Static regions are initialized after paging_init() and should retain their
memory states.

Link: https://sashiko.dev/#/message/20260814090305.4C8741F00A3D%40smtp.kernel.org [1]

> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index a0df075a9902e..0c5163ce601ea 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -708,7 +708,7 @@ static void __init fdt_init_reserved_mem_node(unsigned long node, const char *un
>  		pr_info("node %s compatible matching fail\n", rmem->name);
>  		rmem->name = NULL;
>  
> -		if (nomap)
> +		if (dynamic && nomap)
>  			memblock_clear_nomap(rmem->base, rmem->size);
>  
>  		if (dynamic || !nomap)

[Severity: High]
This is a pre-existing issue, but while looking at reserved memory error
handling in this file, I noticed a potential memory corruption when dynamic
allocation fails.

In early_init_dt_alloc_reserved_memory_arch(), the base address is assigned
before the nomap check:

base = memblock_phys_alloc_range(size, align, start, end);
...
*res_base = base;
if (nomap) {
        err = memblock_mark_nomap(base, size);
        if (err)
                memblock_phys_free(base, size);
}

If memblock_mark_nomap() fails, the memory is freed, but *res_base still
holds the freed address instead of being cleared.

Then in __reserved_mem_alloc_size(), the code checks the base address instead
of the return value ret:

ret = early_init_dt_alloc_reserved_memory_arch(size, align,
                                               0, 0, nomap, &base);
if (ret == 0)
        pr_debug("...");

if (base == 0) {
        pr_err("failed to allocate memory...");
        return -ENOMEM;
}

Since base contains the freed address instead of 0, does this allow the
freed memory to be registered and later handed out by the buddy allocator
while still being incorrectly used as reserved memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818092420.2859026-1-chenwandun1@gmail.com?part=5

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

[sashiko-bot]

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 <chenwandun@lixiang.com>

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/20260818092420.2859026-1-chenwandun1@gmail.com?part=4

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

[sashiko-bot]

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 <chenwandun@lixiang.com>

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?

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.

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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818092420.2859026-1-chenwandun1@gmail.com?part=2

Re: [PATCH v2 3/5] of: reserved_mem: reject statically placed regions overlapping existing reservations

[sashiko-bot]

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

New issues:
- [High] Rejecting overlapping memblock reservations up-front breaks platforms where memory is legitimately reserved early (e.g., by EFI or architecture code) and then described by a DT reserved-memory node.
- [High] An array overflow in `reserve_failed_nodes` bypasses the failed-node skip logic, leading to memory corruption.

Pre-existing issues:
- [Critical] Dynamic allocation of reserved memory regions ignores internal failures, leading to initialization of freed memory.
- [High] The patch fails to prevent overlaps with `/memreserve/` entries, leaving the memory corruption bug unfixed for those regions due to execution ordering.
- [High] Dynamically allocated `nomap` reserved memory regions are permanently leaked if their initialization fails.
--

--- Patch [3]: [PATCH v2 3/5] of: reserved_mem: reject statically placed regions overlapping existing reservations ---
commit 1c8f1d8c03461ed83a10e5698047d85d76387f8b
Author: Wandun Chen <chenwandun@lixiang.com>

of: reserved_mem: reject statically placed regions overlapping existing reservations

This patch rejects overlapping memblock reservations up front for
statically placed regions, preventing them from corrupting memory if
initialization fails. Dynamically allocated regions are unaffected.

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
> index f25dc0cca2f06..71a879628c4aa 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -158,6 +158,11 @@ static int __init early_init_dt_reserve_memory(phys_addr_t base,
>  
>  		return memblock_mark_nomap(base, size);
>  	}
> +
> +	if (memblock_is_region_reserved(base, size) ||
> +	    memblock_overlaps_nomap(base, size))
> +		return -EBUSY;

[Severity: High]
Will this upfront overlap rejection break platforms where memory is
legitimately reserved early by EFI or architecture code, and then described
by a device tree reserved-memory node?

If architecture code like efi_init() on ARM64 cleanly reserves memory using
memblock_reserve() before fdt_scan_reserved_mem() executes, the new
memblock_is_region_reserved() check will evaluate to true and reject the
node unconditionally with -EBUSY.

[Severity: High]
This isn't a bug introduced by this patch, but does this overlap check miss
/memreserve/ entries entirely due to execution ordering?

In early_init_fdt_scan_reserved_mem(), fdt_scan_reserved_mem() executes
before the /memreserve/ blocks are iterated and reserved. Because of this,
when memblock_is_region_reserved() runs here, the /memreserve/ regions are
not yet known.

If a reserved-memory node overlaps a /memreserve/, it bypasses the check.
If the node's driver later fails initialization, its error path executes
memblock_phys_free(), which will still wrongly free the /memreserve/ memory.

> +
>  	return memblock_reserve(base, size);
>  }

[Severity: High]
Can an array overflow in reserve_failed_nodes bypass the failed-node skip
logic?

If the MAX_RESERVED_REGIONS limit is exceeded, record_reserve_failed_node()
returns false. The fallback logic in fdt_scan_reserved_mem() then
increments count instead of skipping the node:

drivers/of/of_reserved_mem.c:fdt_scan_reserved_mem() {
    ...
    else if (should_record_failed_node &&
             !record_reserve_failed_node(child, uname))
        count++;
    ...
}

Later, in fdt_scan_reserved_mem_late(), reserved_mem_node_reserve_failed()
returns false for this untracked node, passing it to
fdt_init_reserved_mem_node().

If initialization fails, the error path frees the memory, potentially
corrupting an existing overlapping reservation.

[Severity: High]
This is a pre-existing issue, but do dynamically allocated nomap reserved
memory regions permanently leak if their initialization fails?

In early_init_dt_alloc_reserved_memory_arch(), dynamic regions are added to
memblock.reserved. If driver initialization fails in
fdt_init_reserved_mem_node(), the error path only clears the nomap flag:

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

This completely omits the memblock_phys_free() call for nomap regions,
creating a physical memory leak.

[Severity: Critical]
This isn't a bug introduced by this patch, but does the error handling in
__reserved_mem_alloc_size() ignore internal failures and lead to early boot
use-after-free issues?

In early_init_dt_alloc_reserved_memory_arch(), the output parameter
*res_base is populated before calling memblock_mark_nomap():

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

If memblock_mark_nomap() fails, it frees the memory but leaves the freed
physical address in *res_base.

The caller, __reserved_mem_alloc_size(), ignores the return code and
checks the base address instead:

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 (base == 0) {
        pr_err("failed to allocate memory...");
        return -ENOMEM;
    }
    ...
}

Because base contains the freed address rather than 0, the check passes and
the freed memory block is incorrectly passed to fdt_init_reserved_mem_node().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818092420.2859026-1-chenwandun1@gmail.com?part=3