[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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Exceeding `MAX_RESERVED_REGIONS` failed memory reservations causes unrecorded failures to steal dynamic array slots from valid nodes, leading to skipped valid regions and potential arbitrary physical memory frees.
- [High] Redundant recording of natively skipped zero-sized nodes wastes capacity in the bounded `reserve_failed_nodes` array, artificially accelerating array slot stealing overflows.
- [High] Unconditionally skipping initialization for regions whose early memory reservation fails breaks valid legacy configurations.
--

--- Patch [1]: [PATCH 1/3] of: reserved_mem: skip init for regions whose early reservation failed ---
commit 89c6cc50df5c4bf222e4058d5f0ce76a20439009
Author: Wandun Chen <chenwandun@lixiang.com>

of: reserved_mem: skip init for regions whose early reservation failed

The patch modifies reserved memory initialization to propagate errors from
early memory reservation. It tracks nodes that failed to reserve and skips
them during the late scan, preventing devices from using memory that was
not actually protected.

> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 8c9d6395d6a39..f20747725de42 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c

[ ... ]

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

[Severity: High]
Does returning -EINVAL here needlessly waste slots in the reserve_failed_nodes
array?

Since fdt_scan_reserved_mem_late() in drivers/of/of_reserved_mem.c naturally
skips zero-sized nodes with its own if (!size) continue check before
evaluating the failure array, returning an error here will consume one of the
limited slots in reserve_failed_nodes for a node that doesn't need tracking.

> +
> +	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;

[Severity: High]
Does propagating the -EBUSY error from early_init_dt_reserve_memory() break
valid legacy configurations?

If a region is validly pre-reserved by EFI runtime services, a standard
/memreserve/ entry, or a bootloader, early_init_dt_reserve_memory() correctly
returns -EBUSY. 

Prior to this patch, the error was ignored, allowing the node to be
initialized so devices could still parse its properties like no-map or
reusable. By returning the error here and skipping the node during
fdt_scan_reserved_mem_late(), these valid regions are completely removed
from the subsystem, which could cause legitimate device drivers to fail to
probe.

>  	}
> +
> +	fdt_fixup_reserved_mem_node(node, base, size);

[ ... ]

> @@ -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);
> +		}

[Severity: Critical]
Can overflowing the reserve_failed_nodes array cause arbitrary physical memory
frees and valid device reservations being skipped?

If more than the maximum limit of statically placed reserved memory regions fail
their early reservation, the reserve_failed_nodes array overflows. These
overflowed nodes are not recorded, and their count is not added to
total_reserved_mem_cnt.

During fdt_scan_reserved_mem_late() in drivers/of/of_reserved_mem.c, the array
is dynamically allocated based on total_reserved_mem_cnt. However, the
unrecorded failed nodes bypass the skip check:

drivers/of/of_reserved_mem.c:fdt_scan_reserved_mem_late() {
    ...
		if (reserved_mem_node_reserve_failed(child))
			continue;
    ...
}

These nodes then steal array slots from valid regions that appear later. If
the unrecorded region's driver node_init callback subsequently fails, the
error path in fdt_init_reserved_mem_node() will call memblock_phys_free()
on the region:

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

Since the region originally failed reservation, this memory is owned by
another subsystem, resulting in an arbitrary free of in-use memory.

>  
>  		/*
>  		 * Save the nodes for the dynamically-placed regions

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

[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

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

[sashiko-bot]

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

New issues:
- [High] Rejecting partially overlapping reserved memory regions without calling memblock_reserve leaves non-overlapping parts exposed to the buddy allocator.
- [Low] The overlap error message prints an exclusive end address using a format conventionally used for inclusive ranges, and can wrap around to 0 on integer overflow.

Pre-existing issues:
- [High] The patch fails to reject overlaps for 'no-map' regions, leaving the memory corruption vulnerability it intended to fix fully exploitable in that case.
--

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

This patch rejects statically placed reserved memory regions that overlap
with existing reservations. It prevents the overlapping memory from being
used concurrently or returned to the buddy allocator if a region's driver
fails to initialize.

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

> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index f20747725de42..67cb60616fc69 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);
>  	}

[Severity: High]
This is a pre-existing issue, but in early_init_dt_reserve_memory() does the
overlap check for nomap regions fail to detect overlaps between two nomap
regions?

Because memblock_mark_nomap() does not add the region to memblock.reserved,
a second overlapping nomap region bypasses the check. If the second driver
later fails and clears the flag, could memblock_free_all() release the memory
to the buddy allocator while the first driver is still using it?

> +
> +	if (memblock_is_region_reserved(base, size)) {
> +		phys_addr_t end = base + size;

[Severity: Low]
Does this calculate an exclusive end address instead of an inclusive one?

If base + size reaches the physical address limit, could this overflow to 0
and print a misleading log message?

> +
> +		pr_err("Reserved memory: [%pa-%pa] overlaps an existing reservation, ignoring\n",
> +		       &base, &end);
> +		return -EBUSY;

[Severity: High]
If a statically placed region only partially overlaps an existing
reservation, does returning -EBUSY here without calling memblock_reserve()
leave the non-overlapping parts exposed to the buddy allocator?

To safely protect the non-overlapping physical pages from being used for
general kernel use, should the code still call memblock_reserve() before
returning -EBUSY?

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

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

[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

Re: [PATCH 3/3] 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] 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 <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, and this patch
ensures the memory is freed.

Link: https://lore.kernel.org/all/20260806100605.2C2C01F000E9@smtp.kernel.org/ [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/20260814081443.1061046-1-chenwandun1@gmail.com?part=3