[PATCH v2 0/3] of: reserved_mem: fixes and cleanups

[PATCH v2 0/3] of: reserved_mem: fixes and cleanups

[Wandun Chen]

This is v2 of the OF reserved memory fixes/cleanups series.

The original v1 [1] contains two parts of work:
 - Bug fixes and small cleanups about reserved memory. 
 - A vmcore size optimization that exclude reserved memory out of
   vmcore.

For the convenience of review, I have split it into two independent
patchsets. This patchset only contains bug fixes and cleanups
content, while the content optimized for vmcore size will be in
another patchset.

v1 --> v2:
1. Add new patch 1, to fix unnecessary memory allocation.
2. Support only one entry in reg property in patch 3, suggested by
   Rob Herring [2].
3. Per Rob's review on v1 2/11 [3], drop patch about "rejecting
   reserved memory outside physical memory range" due to potential
   abuse.
4. Drop patch about "of: reserved_mem: avoid unconditional save of
   reg entries in fdt_scan_reserved_mem_late()" due to a
   misunderstanding on my side.
5. Update commit message.

[1] https://lore.kernel.org/lkml/20260429065831.1510858-1-chenwandun@lixiang.com/
[2] https://lore.kernel.org/all/20260429065831.1510858-1-chenwandun@lixiang.com/T/#m29fa0f1c22c23e6343070e70f905c9482f930901
[3] https://lore.kernel.org/all/20260429065831.1510858-1-chenwandun@lixiang.com/T/#m5948f8679540fb361e7f432137babb4609ff154f


Wandun Chen (3):
  of: reserved_mem: avoid unnecessary memory allocation when
    __reserved_mem_check_root() fails
  of: reserved_mem: clean up redundant alloc_reserved_mem_array() call
  of: reserved_mem: only support one <base size> entry in reg property

 drivers/of/of_reserved_mem.c | 48 +++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 20 deletions(-)

-- 
2.43.0

[PATCH v2 1/3] of: reserved_mem: avoid unnecessary memory allocation when __reserved_mem_check_root() fails

[Wandun Chen]

total_reserved_mem_cnt will keep the init value (MAX_RESERVED_REGIONS)
in fdt_scan_reserved_mem() if __reserved_mem_check_root() fails.

fdt_scan_reserved_mem_late() calls alloc_reserved_mem_array() to
allocate memory according to total_reserved_mem_cnt before
__reserved_mem_check_root(), so if __reserved_mem_check_root() fails,
the allocated array has nowhere to be used, so no need to allocate
memory.

Move alloc_reserved_mem_array() after __reserved_mem_check_root() to
avoid potential memory waste.

Signed-off-by: Wandun Chen <chenwandun@lixiang.com>
Tested-by: Meijing Zhao <zhaomeijing@lixiang.com>
---
 drivers/of/of_reserved_mem.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 8d5777cb5d1b..7856dc857d65 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -265,14 +265,14 @@ void __init fdt_scan_reserved_mem_late(void)
 		return;
 	}
 
-	/* Attempt dynamic allocation of a new reserved_mem array */
-	alloc_reserved_mem_array();
-
 	if (__reserved_mem_check_root(node)) {
 		pr_err("Reserved memory: unsupported node format, ignoring\n");
 		return;
 	}
 
+	/* Attempt dynamic allocation of a new reserved_mem array */
+	alloc_reserved_mem_array();
+
 	fdt_for_each_subnode(child, fdt, node) {
 		const char *uname;
 		u64 b, s;
-- 
2.43.0

Re: [PATCH v2 1/3] of: reserved_mem: avoid unnecessary memory allocation when __reserved_mem_check_root() fails

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Error paths in alloc_reserved_mem_array() fail to reset total_reserved_mem_cnt, causing a deterministic buffer overflow.
--

commit b6927a2d1eca00b0e1f7c1ca6f640812391b20c8
Author: Wandun Chen <chenwandun1@gmail.com>

of: reserved_mem: avoid unnecessary memory allocation when __reserved_mem_check_root() fails

This commit delays the dynamic allocation of the reserved memory array
until after checking the root node format. It aims to prevent memory
allocation and potential waste if the reserved memory root format is
unsupported.

> 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
> @@ -265,14 +265,14 @@ void __init fdt_scan_reserved_mem_late(void)
[ ... ]
>  	if (__reserved_mem_check_root(node)) {
>  		pr_err("Reserved memory: unsupported node format, ignoring\n");
>  		return;
>  	}
>  
> +	/* Attempt dynamic allocation of a new reserved_mem array */
> +	alloc_reserved_mem_array();

This isn't a bug introduced by this commit, but while looking at the
relocation of alloc_reserved_mem_array(), I noticed a pre-existing flaw
in its error paths.

If total_reserved_mem_cnt exceeds MAX_RESERVED_REGIONS (64), we attempt
to allocate a new array. If memblock_alloc() fails or alloc_size == SIZE_MAX,
alloc_reserved_mem_array() logs an error and returns early without resetting
total_reserved_mem_cnt back to MAX_RESERVED_REGIONS.

Since alloc_reserved_mem_array() returns void and doesn't propagate the
error, fdt_scan_reserved_mem_late() continues iterating over the remaining
nodes and calling fdt_init_reserved_mem_node().

Does this lead to an out-of-bounds write?

In fdt_init_reserved_mem_node(), the bounds check compares against the active
total_reserved_mem_cnt:

drivers/of/of_reserved_mem.c:fdt_init_reserved_mem_node() {
    ...
    if (reserved_mem_count == total_reserved_mem_cnt) {
        pr_err("not enough space for all defined regions.\n");
        return -ENOSPC;
    }
    ...
}

If alloc_reserved_mem_array() failed, reserved_mem still points to the
static 64-element array, but total_reserved_mem_cnt is still set to the
larger requested size. This appears to allow reserved_mem_count to exceed
64, deterministically writing past the end of the static array.

Could the error paths in alloc_reserved_mem_array() be updated to always
reset total_reserved_mem_cnt to MAX_RESERVED_REGIONS?

> +
>  	fdt_for_each_subnode(child, fdt, node) {
>  		const char *uname;
>  		u64 b, s;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519082427.4181476-1-chenwandun@lixiang.com?part=1

[PATCH v2 2/3] of: reserved_mem: clean up redundant alloc_reserved_mem_array() call

[Wandun Chen]

If total_reserved_mem_cnt is 0 after fdt_scan_reserved_mem(), there
is no entry to save, alloc_reserved_mem_array() has nothing to do.
Just skip it.

Signed-off-by: Wandun Chen <chenwandun@lixiang.com>
---
 drivers/of/of_reserved_mem.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 7856dc857d65..462e7c3078a3 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -270,6 +270,9 @@ void __init fdt_scan_reserved_mem_late(void)
 		return;
 	}
 
+	if (!total_reserved_mem_cnt)
+		return;
+
 	/* Attempt dynamic allocation of a new reserved_mem array */
 	alloc_reserved_mem_array();
 
-- 
2.43.0

Re: [PATCH v2 2/3] of: reserved_mem: clean up redundant alloc_reserved_mem_array() call

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Missing bounds check on `dynamic_nodes_cnt` in `fdt_scan_reserved_mem()` allows out-of-bounds writes to the stack array `dynamic_nodes`.
- [High] Error handling in `alloc_reserved_mem_array()` fails to reset `total_reserved_mem_cnt` on allocation failure, leading to a buffer overflow on the static `reserved_mem_array`.
--

commit 7ce94f9c503bebcfe72a546d200327f9bde1957e
Author: Wandun Chen <chenwandun1@gmail.com>

of: reserved_mem: clean up redundant alloc_reserved_mem_array() call

This commit skips calling alloc_reserved_mem_array() if total_reserved_mem_cnt
is 0 after fdt_scan_reserved_mem(). This avoids executing dynamic allocation
logic when there are no reserved memory regions to save.

> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 7856dc857d650..462e7c3078a3a 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -270,6 +270,9 @@ void __init fdt_scan_reserved_mem_late(void)
>  		return;
>  	}
>  
> +	if (!total_reserved_mem_cnt)
> +		return;
> +
>  	/* Attempt dynamic allocation of a new reserved_mem array */
>  	alloc_reserved_mem_array();

This problem wasn't introduced by this patch, but if alloc_reserved_mem_array()
fails (e.g. memblock_alloc() fails or alloc_size == SIZE_MAX), it returns
without resetting total_reserved_mem_cnt to MAX_RESERVED_REGIONS.

Because total_reserved_mem_cnt can exceed MAX_RESERVED_REGIONS, and
reserved_mem would still point to the static reserved_mem_array, would the
boundary check in fdt_init_reserved_mem_node() fail to stop insertions at
MAX_RESERVED_REGIONS?

Can this result in an out-of-bounds write into the static array?

Also not introduced by this patch, there appears to be a missing bounds
check in fdt_scan_reserved_mem():

drivers/of/of_reserved_mem.c:fdt_scan_reserved_mem() {
	int dynamic_nodes_cnt = 0, count = 0;
	int dynamic_nodes[MAX_RESERVED_REGIONS];
	...
		if (err == -ENOENT && of_get_flat_dt_prop(child, "size", NULL)) {
			dynamic_nodes[dynamic_nodes_cnt] = child;
			dynamic_nodes_cnt++;
		}
	...
}

If a device tree contains more dynamically sized reserved memory nodes than
MAX_RESERVED_REGIONS, could this overwrite the bounds of the dynamic_nodes
stack array?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519082427.4181476-1-chenwandun@lixiang.com?part=2

[PATCH v2 3/3] of: reserved_mem: only support one <base size> entry in reg property

[Wandun Chen]

A /reserved-memory child node may have multiple <base size> tuples in
'reg' property, but multiple entries in 'reg' have never been fully
functional:
 - fdt_scan_reserved_mem() in the early pass loops over every
   tuple and reserves them all.

 - fdt_scan_reserved_mem_late() reads 'reg' by
   of_flat_dt_get_addr_size(), which returns false if entries != 1.
   So 'reg' property with multiple <base size> entries will be
   skipped, no reserved_mem entry is created in reserved_mem[].

Supporting multiple <base size> tuples is not a good idea:
  - It requires reserved_mem_ops->node_init support. Currently,
    CMA(rmem_cma_setup) and DMA(rmem_dma_setup) are not supported.

  - of_reserved_mem_lookup() is name-based, only the first entry in
    multiple <base size> tuples will be found.

So change to support one <base size> entry in 'reg' property.

Also update dt binding:
  https://github.com/devicetree-org/dt-schema/pull/197

Suggested-by: Rob Herring <robh@kernel.org>
Signed-off-by: Wandun Chen <chenwandun@lixiang.com>
Tested-by: Meijing Zhao <zhaomeijing@lixiang.com>
Link: https://lore.kernel.org/all/20260506014752.GA280279-robh@kernel.org/
---
 drivers/of/of_reserved_mem.c | 39 ++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 462e7c3078a3..0e91c4a71d89 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -134,37 +134,38 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
 					     const char *uname)
 {
 	phys_addr_t base, size;
-	int i, len, err;
+	int len, err;
 	const __be32 *prop;
 	bool nomap;
+	u64 b, s;
 
 	prop = of_flat_dt_get_addr_size_prop(node, "reg", &len);
 	if (!prop)
 		return -ENOENT;
 
+	if (len > 1)
+		pr_warn("Reserved memory: node '%s' has %d <base size> entries, only the first is used\n",
+			uname, len);
+
 	nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;
 
 	err = fdt_validate_reserved_mem_node(node, NULL);
 	if (err && err != -ENODEV)
 		return err;
 
-	for (i = 0; i < len; i++) {
-		u64 b, s;
-
-		of_flat_dt_read_addr_size(prop, i, &b, &s);
-
-		base = b;
-		size = s;
+	of_flat_dt_read_addr_size(prop, 0, &b, &s);
+	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 {
-			pr_err("Reserved memory: failed to reserve memory for node '%s': base %pa, size %lu MiB\n",
-			       uname, &base, (unsigned long)(size / SZ_1M));
-		}
+	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 {
+		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 0;
 }
 
@@ -277,20 +278,24 @@ void __init fdt_scan_reserved_mem_late(void)
 	alloc_reserved_mem_array();
 
 	fdt_for_each_subnode(child, fdt, node) {
+		const __be32 *prop;
 		const char *uname;
 		u64 b, s;
 		int ret;
+		int len;
 
 		if (!of_fdt_device_is_available(fdt, child))
 			continue;
 
-		if (!of_flat_dt_get_addr_size(child, "reg", &b, &s))
+		prop = of_flat_dt_get_addr_size_prop(child, "reg", &len);
+		if (!prop)
 			continue;
 
 		ret = fdt_validate_reserved_mem_node(child, NULL);
 		if (ret && ret != -ENODEV)
 			continue;
 
+		of_flat_dt_read_addr_size(prop, 0, &b, &s);
 		base = b;
 		size = s;
 
-- 
2.43.0

Re: [PATCH v2 3/3] of: reserved_mem: only support one <base size> entry in reg property

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unconditional read of the first `reg` property entry without verifying that `len > 0` leads to an out-of-bounds FDT read for empty `reg` properties.
--

commit def0743ccda5f4a97f9b1275e989c8ea559363b4
Author: Wandun Chen <chenwandun1@gmail.com>

of: reserved_mem: only support one <base size> entry in reg property

This commit simplifies reserved memory parsing by restricting support
to a single base/size entry in the reg property. It removes the loop
in the early pass and warns if multiple entries are present,
unconditionally reading only the first entry.

Link: https://lore.kernel.org/all/20260506014752.GA280279-robh@kernel.org/

> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 462e7c3078a3a..0e91c4a71d89d 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -134,37 +134,38 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
[ ... ]
>  	prop = of_flat_dt_get_addr_size_prop(node, "reg", &len);
>  	if (!prop)
>  		return -ENOENT;
>  
> +	if (len > 1)
> +		pr_warn("Reserved memory: node '%s' has %d <base size> entries, only the first is used\n",
> +			uname, len);
> +
>  	nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;
>  
>  	err = fdt_validate_reserved_mem_node(node, NULL);
>  	if (err && err != -ENODEV)
>  		return err;
>  
> -	for (i = 0; i < len; i++) {
> -		u64 b, s;
> -
> -		of_flat_dt_read_addr_size(prop, i, &b, &s);
> -
> -		base = b;
> -		size = s;
> +	of_flat_dt_read_addr_size(prop, 0, &b, &s);

If a device tree node contains an empty reg property,
of_flat_dt_get_addr_size_prop() returns a valid pointer to the empty
property data and sets len to 0.

Since the loop was removed, is there a risk of an out-of-bounds read here
because the code unconditionally calls of_flat_dt_read_addr_size()?

This could fetch garbage values for the physical base and size. If the
parsed garbage size happens to be non-zero, could the kernel attempt
to reserve unintended physical memory regions via
early_init_dt_reserve_memory()?

Would it be safer to verify len > 0 before reading the first entry?

> +	base = b;
> +	size = s;
[ ... ]
> @@ -277,20 +278,24 @@ void __init fdt_scan_reserved_mem_late(void)
>  	alloc_reserved_mem_array();
>  
>  	fdt_for_each_subnode(child, fdt, node) {
> +		const __be32 *prop;
[ ... ]
> -		if (!of_flat_dt_get_addr_size(child, "reg", &b, &s))
> +		prop = of_flat_dt_get_addr_size_prop(child, "reg", &len);
> +		if (!prop)
>  			continue;
>  
>  		ret = fdt_validate_reserved_mem_node(child, NULL);
>  		if (ret && ret != -ENODEV)
>  			continue;
>  
> +		of_flat_dt_read_addr_size(prop, 0, &b, &s);

Does this have the same out-of-bounds read potential when len == 0?

The previous of_flat_dt_get_addr_size() wrapper explicitly checked that
entries != 1, which inherently skipped empty reg properties.

>  		base = b;
>  		size = s;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519082427.4181476-1-chenwandun@lixiang.com?part=3