[PATCH 0/3] of: reserved_mem: fix a few reserved memory issues

[PATCH 0/3] of: reserved_mem: fix a few reserved memory issues

[Wandun Chen]

From: Wandun Chen <chenwandun@lixiang.com>

This series fixes a few issues in the reserved-memory handling:

1. A region whose early reservation failed was still counted and
   initialized by the late scan, handing unprotected memory to a
   device. (patch 1)

2. A statically placed region overlapping an existing reservation was
   accepted, so the overlap could be used by two regions concurrently
   and freed on init failure. (patch 2)

3. A dynamically allocated no-map region was never returned to the
   buddy allocator when its driver init failed: only
   memblock_clear_nomap() was called, leaving the entry in
   memblock.reserved. (patch 3)

Sashiko reported the original issue in [1].

This is sent for review now; given the v7.2 release is closing, there
is no expectation to merge it this cycle.

[1] https://lore.kernel.org/all/20260806100605.2C2C01F000E9@smtp.kernel.org/

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

 drivers/of/of_reserved_mem.c | 69 ++++++++++++++++++++++++++++--------
 1 file changed, 55 insertions(+), 14 deletions(-)

-- 
2.43.0

[PATCH 1/3] 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 | 49 ++++++++++++++++++++++++++++--------
 1 file changed, 39 insertions(+), 10 deletions(-)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 8c9d6395d6a3..f20747725de4 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -32,6 +32,19 @@ 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 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)
@@ -167,14 +180,19 @@ 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) {
 		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 +324,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 */
@@ -357,8 +379,15 @@ int __init fdt_scan_reserved_mem(void)
 		uname = fdt_get_name(fdt, child, NULL);
 
 		err = __reserved_mem_reserve_reg(child, uname);
-		if (!err)
+		if (!err) {
 			count++;
+		} else if (err != -ENOENT) {
+			if (reserve_failed_nodes_cnt < MAX_RESERVED_REGIONS)
+				reserve_failed_nodes[reserve_failed_nodes_cnt++] = child;
+			else
+				pr_err("too many failed regions, '%s' reservation failed\n",
+				       uname);
+		}
 
 		/*
 		 * Save the nodes for the dynamically-placed regions
-- 
2.43.0

[PATCH 2/3] 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. The
overlapping memory may then be used by two regions concurrently,
corrupting data; and if the region's driver init later 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://lore.kernel.org/all/20260806100605.2C2C01F000E9@smtp.kernel.org/ [1]
---
 drivers/of/of_reserved_mem.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index f20747725de4..67cb60616fc6 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -146,6 +146,15 @@ 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)) {
+		phys_addr_t end = base + size;
+
+		pr_err("Reserved memory: [%pa-%pa] overlaps an existing reservation, ignoring\n",
+		       &base, &end);
+		return -EBUSY;
+	}
+
 	return memblock_reserve(base, size);
 }
 
-- 
2.43.0

[PATCH 3/3] 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(), but leaves the entry in
memblock.reserved untouched, so the memory is never returned to the
buddy allocator.

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://lore.kernel.org/all/20260806100605.2C2C01F000E9@smtp.kernel.org/ [1]
---
 drivers/of/of_reserved_mem.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 67cb60616fc6..d09c29184acf 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -125,7 +125,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,
@@ -340,7 +341,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 */
@@ -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;
 }
@@ -671,7 +672,8 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem,
  * 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;
@@ -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 {
-- 
2.43.0