[PATCH] of: reserved_mem: Add heuristic to validate reserved memory regions

[PATCH] of: reserved_mem: Add heuristic to validate reserved memory regions

[Andrea della Porta]

When parsing static reserved-memory DT nodes, any node with a reg property
length that is not perfectly conformant is discarded.
Specifically, any reg property whose length is not a multiple of the parent's
(#address-cells + #size-cells) is dropped.

Relax this condition (while still treating perfect multiples as having higher
precedence) by allowing regions that are subsets of the parent's addressable
space to be considered for inclusion.
For example, in the following scenario:

/ {
        #address-cells = <0x02>;
        #size-cells = <0x02>;
	...

        reserved-memory {
                #address-cells = <0x02>;
                #size-cells = <0x02>;
		...

                nvram {
                        reg = <0x00 0x3fd16d00 0x37>;
			...
		};
	};
};

Even though the reg property of the nvram node is not well-formed from a DT
syntax perspective, it still references a perfectly valid memory region of
0x37 bytes that should be reserved.

This has at least one real-world equivalent on the Raspberry Pi 5, for example,
on which the firmware incorrectly overwrites the nvram node's reg property
without taking into account the actual value of the parent's #size-cells.

Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
---

The aforementioned heuristic has been tested with several combo of #address,
#size and reg length and the results are shown in the following table:

Testing #address-cells 1, #size-cells 1

len     |t_len  |addr   |size   |#ignore
0       |8      | INVALID
4       |8      | INVALID
8       |8      |1      |1      |0
12      |8      | INVALID
16      |8      |1      |1      |1
20      |8      | INVALID
24      |8      |1      |1      |2
28      |8      | INVALID
32      |8      |1      |1      |3
36      |8      | INVALID
40      |8      |1      |1      |4
44      |8      | INVALID
48      |8      |1      |1      |5
52      |8      | INVALID
56      |8      |1      |1      |6
60      |8      | INVALID
64      |8      |1      |1      |7
68      |8      | INVALID
72      |8      |1      |1      |8
76      |8      | INVALID
80      |8      |1      |1      |9
84      |8      | INVALID
88      |8      |1      |1      |10
92      |8      | INVALID
96      |8      |1      |1      |11


Testing #address-cells 2, #size-cells 1

len     |t_len  |addr   |size   |#ignore
0       |12     | INVALID
4       |12     | INVALID
8       |12     | INVALID
12      |12     |2      |1      |0
16      |12     | INVALID
20      |12     | INVALID
24      |12     |2      |1      |1
28      |12     | INVALID
32      |12     | INVALID
36      |12     |2      |1      |2
40      |12     | INVALID
44      |12     | INVALID
48      |12     |2      |1      |3
52      |12     | INVALID
56      |12     | INVALID
60      |12     |2      |1      |4
64      |12     | INVALID
68      |12     | INVALID
72      |12     |2      |1      |5
76      |12     | INVALID
80      |12     | INVALID
84      |12     |2      |1      |6
88      |12     | INVALID
92      |12     | INVALID
96      |12     |2      |1      |7


Testing #address-cells 2, #size-cells 2

len     |t_len  |addr   |size   |#ignore
0       |16     | INVALID
4       |16     | INVALID
8       |16     | INVALID
12      |16     |2      |1      |0
16      |16     |2      |2      |0
20      |16     | INVALID
24      |16     |2      |1      |1
28      |16     | INVALID
32      |16     |2      |2      |1
36      |16     |2      |1      |2
40      |16     | INVALID
44      |16     | INVALID
48      |16     |2      |2      |2
52      |16     | INVALID
56      |16     | INVALID
60      |16     |2      |1      |4
64      |16     |2      |2      |3
68      |16     | INVALID
72      |16     |2      |1      |5
76      |16     | INVALID
80      |16     |2      |2      |4
84      |16     |2      |1      |6
88      |16     | INVALID
92      |16     | INVALID
96      |16     |2      |2      |5

 drivers/of/of_reserved_mem.c | 88 ++++++++++++++++++++++++++++++++----
 1 file changed, 78 insertions(+), 10 deletions(-)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 2e9ea751ed2d..f94069ef988e 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -130,6 +130,68 @@ static void __init fdt_reserved_mem_save_node(unsigned long node, const char *un
 	return;
 }
 
+/**
+ * reg_len_valid() - scan for a suitable #size-cells value and validate
+ *		     the reg property length
+ * @len:	Length of the reg property to be validated (bytes). Can be composed
+ *		by more than one reg addr+size pairs
+ * @calc_addr:	the number of address cells expected by the parent of the node
+ *		containing the reg property. Currently it's just used as in param
+ * @calc_size:	the number of size cells expected by the parent of the node
+ *		containing the reg property (out param)
+ *
+ * This function tries to find the correct #size-cells number for the size portion
+ * of a reg property, assuming the #address-cells passed in (calc_addr param)
+ * is valid to avoid ambiguity.
+ * Parent_len is the length in bytes of a single reg property * as expected by the
+ * parent.
+ * Either len is a multiple of parent_len, in which case there's no adjustment to
+ * be made to calc_size and the region is automatically valid (this choice has the
+ * priority), or it is not a multiple.
+ * In the latter case, it finds the smallest calc_addr+calc_size for which len
+ * is still a multiple and adjust calc_size accordingly.
+ * The rationale is to avoid nonsensical combo e.g. #adress-cells 1 and #size-cells
+ * 2 since it's not fully addressable, and to promote any other combo that is a
+ * subset of the original space, e.g. with calc_addr=2 and calc_size=2, returning
+ * calc_size=1 still makes sense since the region is included in the parent space.
+ * The reason for that is to avoid dropping perfectly valid memory regions that
+ * could just have been passed with the wrong format in the reg property (some fw
+ * are reportedly doing that when updating the DT at boot).
+ *
+ * Returns:	true if the region is valid and can be further processed,
+ *		false otherwise. If valid, calc_size is filled with the actual
+ *		length (in cells) of the size part.
+ *
+ */
+static bool reg_len_valid(int len, const int *calc_addr, int *calc_size)
+{
+	int parent_len = (*calc_addr + *calc_size) * sizeof(__be32);
+	bool parent_multiple = (len % parent_len) / sizeof(__be32);
+	int row_n, calc_row_len = parent_len / sizeof(__be32);
+	int len_b = len / sizeof(__be32);
+
+	if (!len || !parent_len)
+		return false;
+
+	for (row_n = len_b / 2; row_n > 0; row_n--) {
+		int tmp_row_len = len_b / row_n;
+
+		if (calc_row_len > tmp_row_len &&
+		    tmp_row_len > *calc_addr &&
+		    (len_b % tmp_row_len == 0))
+			calc_row_len = tmp_row_len;
+	}
+
+	if (parent_multiple && calc_row_len != parent_len / sizeof(__be32)) {
+		*calc_size = calc_row_len - *calc_addr;
+		return true;
+	} else if (!parent_multiple) {
+		return true;
+	}
+
+	return false;
+}
+
 static int __init early_init_dt_reserve_memory(phys_addr_t base,
 					       phys_addr_t size, bool nomap)
 {
@@ -154,9 +216,9 @@ static int __init early_init_dt_reserve_memory(phys_addr_t base,
 static int __init __reserved_mem_reserve_reg(unsigned long node,
 					     const char *uname)
 {
-	int t_len = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
+	int calc_addr, calc_size;
 	phys_addr_t base, size;
-	int len;
+	int len, t_len;
 	const __be32 *prop;
 	bool nomap;
 
@@ -164,17 +226,20 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
 	if (!prop)
 		return -ENOENT;
 
-	if (len && len % t_len != 0) {
+	calc_addr = dt_root_addr_cells;
+	calc_size = dt_root_size_cells;
+	if (!reg_len_valid(len, &calc_addr, &calc_size)) {
 		pr_err("Reserved memory: invalid reg property in '%s', skipping node.\n",
 		       uname);
 		return -EINVAL;
 	}
 
 	nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;
+	t_len = (calc_addr + calc_size) * (int)sizeof(__be32);
 
 	while (len >= t_len) {
-		base = dt_mem_next_cell(dt_root_addr_cells, &prop);
-		size = dt_mem_next_cell(dt_root_size_cells, &prop);
+		base = dt_mem_next_cell(calc_addr, &prop);
+		size = dt_mem_next_cell(calc_size, &prop);
 
 		if (size && early_init_dt_reserve_memory(base, size, nomap) == 0) {
 			/* Architecture specific contiguous memory fixup. */
@@ -255,6 +320,7 @@ void __init fdt_scan_reserved_mem_reg_nodes(void)
 	}
 
 	fdt_for_each_subnode(child, fdt, node) {
+		int calc_addr, calc_size;
 		const char *uname;
 
 		prop = of_get_flat_dt_prop(child, "reg", &len);
@@ -263,19 +329,21 @@ void __init fdt_scan_reserved_mem_reg_nodes(void)
 		if (!of_fdt_device_is_available(fdt, child))
 			continue;
 
+		calc_addr = dt_root_addr_cells;
+		calc_size = dt_root_size_cells;
 		uname = fdt_get_name(fdt, child, NULL);
-		if (len && len % t_len != 0) {
+		if (!reg_len_valid(len, &calc_addr, &calc_size)) {
 			pr_err("Reserved memory: invalid reg property in '%s', skipping node.\n",
 			       uname);
 			continue;
 		}
 
 		if (len > t_len)
-			pr_warn("%s() ignores %d regions in node '%s'\n",
-				__func__, len / t_len - 1, uname);
+			pr_warn("%s() ignores %d regions in node '%s'\n", __func__,
+				len / ((calc_addr + calc_size) * (int)sizeof(__be32)) - 1, uname);
 
-		base = dt_mem_next_cell(dt_root_addr_cells, &prop);
-		size = dt_mem_next_cell(dt_root_size_cells, &prop);
+		base = dt_mem_next_cell(calc_addr, &prop);
+		size = dt_mem_next_cell(calc_size, &prop);
 
 		if (size)
 			fdt_reserved_mem_save_node(child, uname, base, size);
-- 
2.35.3

Re: [PATCH] of: reserved_mem: Add heuristic to validate reserved memory regions

[Andrea della Porta]

Hi, 

On 09:34 Tue 14 Oct     , Andrea della Porta wrote:
> When parsing static reserved-memory DT nodes, any node with a reg property
> length that is not perfectly conformant is discarded.
> Specifically, any reg property whose length is not a multiple of the parent's
> (#address-cells + #size-cells) is dropped.
> 
> Relax this condition (while still treating perfect multiples as having higher
> precedence) by allowing regions that are subsets of the parent's addressable
> space to be considered for inclusion.
> For example, in the following scenario:
> 
> / {
>         #address-cells = <0x02>;
>         #size-cells = <0x02>;
> 	...
> 
>         reserved-memory {
>                 #address-cells = <0x02>;
>                 #size-cells = <0x02>;
> 		...
> 
>                 nvram {
>                         reg = <0x00 0x3fd16d00 0x37>;
> 			...
> 		};
> 	};
> };
> 
> Even though the reg property of the nvram node is not well-formed from a DT
> syntax perspective, it still references a perfectly valid memory region of
> 0x37 bytes that should be reserved.
> 
> This has at least one real-world equivalent on the Raspberry Pi 5, for example,
> on which the firmware incorrectly overwrites the nvram node's reg property
> without taking into account the actual value of the parent's #size-cells.
> 
> Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
> ---
> 
> The aforementioned heuristic has been tested with several combo of #address,
> #size and reg length and the results are shown in the following table:
> 
> Testing #address-cells 1, #size-cells 1
> 
> len     |t_len  |addr   |size   |#ignore
> 0       |8      | INVALID
> 4       |8      | INVALID
> 8       |8      |1      |1      |0
> 12      |8      | INVALID
> 16      |8      |1      |1      |1
> 20      |8      | INVALID
> 24      |8      |1      |1      |2
> 28      |8      | INVALID
> 32      |8      |1      |1      |3
> 36      |8      | INVALID
> 40      |8      |1      |1      |4
> 44      |8      | INVALID
> 48      |8      |1      |1      |5
> 52      |8      | INVALID
> 56      |8      |1      |1      |6
> 60      |8      | INVALID
> 64      |8      |1      |1      |7
> 68      |8      | INVALID
> 72      |8      |1      |1      |8
> 76      |8      | INVALID
> 80      |8      |1      |1      |9
> 84      |8      | INVALID
> 88      |8      |1      |1      |10
> 92      |8      | INVALID
> 96      |8      |1      |1      |11
> 
> 
> Testing #address-cells 2, #size-cells 1
> 
> len     |t_len  |addr   |size   |#ignore
> 0       |12     | INVALID
> 4       |12     | INVALID
> 8       |12     | INVALID
> 12      |12     |2      |1      |0
> 16      |12     | INVALID
> 20      |12     | INVALID
> 24      |12     |2      |1      |1
> 28      |12     | INVALID
> 32      |12     | INVALID
> 36      |12     |2      |1      |2
> 40      |12     | INVALID
> 44      |12     | INVALID
> 48      |12     |2      |1      |3
> 52      |12     | INVALID
> 56      |12     | INVALID
> 60      |12     |2      |1      |4
> 64      |12     | INVALID
> 68      |12     | INVALID
> 72      |12     |2      |1      |5
> 76      |12     | INVALID
> 80      |12     | INVALID
> 84      |12     |2      |1      |6
> 88      |12     | INVALID
> 92      |12     | INVALID
> 96      |12     |2      |1      |7
> 
> 
> Testing #address-cells 2, #size-cells 2
> 
> len     |t_len  |addr   |size   |#ignore
> 0       |16     | INVALID
> 4       |16     | INVALID
> 8       |16     | INVALID
> 12      |16     |2      |1      |0
> 16      |16     |2      |2      |0
> 20      |16     | INVALID
> 24      |16     |2      |1      |1
> 28      |16     | INVALID
> 32      |16     |2      |2      |1
> 36      |16     |2      |1      |2
> 40      |16     | INVALID
> 44      |16     | INVALID
> 48      |16     |2      |2      |2
> 52      |16     | INVALID
> 56      |16     | INVALID
> 60      |16     |2      |1      |4
> 64      |16     |2      |2      |3
> 68      |16     | INVALID
> 72      |16     |2      |1      |5
> 76      |16     | INVALID
> 80      |16     |2      |2      |4
> 84      |16     |2      |1      |6
> 88      |16     | INVALID
> 92      |16     | INVALID
> 96      |16     |2      |2      |5
> 

I've just realized that maybe describing the testing matrix above in
some more details could be beneficial, se here we are:

len: INPUT (bytes) - the total length of the reg property array in the
node (child of reserved-memory node) describing the reserved region.
This can be a single element array in case of only one reg entry, or
multiples ones.

t_len: INPUT (bytes) - the length of one entry row in the reg array as
expected by the root node, which is equal to (#address-cells + #size-cells)*4.

addr: IN/OUT (# of cells): a.k.a calc_addr in the code, the input is equal to
the root node's #address-cells. The output is equal to the number of address
cells to be parsed from the reg property array of length 'len'.

size: IN/OUT (# of cells): a.k.a. calc_size in the code, the input is equal to
the root node's #size-cellsi. The output is equal to the number of size cells to
be parsed from the reg property array of length 'len'.

#ignore: the number of reg property array entries that will be discarded, this
refers to the actual code in fdt_scan_reserved_mem_reg_nodes() which discard
any entry among the first, and it has been described in this matrix just to be
sure there is no regression in calculating that too.

Hope this helps to make that table a little clearer.

Many thanks,
Andrea

>  drivers/of/of_reserved_mem.c | 88 ++++++++++++++++++++++++++++++++----
>  1 file changed, 78 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 2e9ea751ed2d..f94069ef988e 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -130,6 +130,68 @@ static void __init fdt_reserved_mem_save_node(unsigned long node, const char *un
>  	return;
>  }
>  
> +/**
> + * reg_len_valid() - scan for a suitable #size-cells value and validate
> + *		     the reg property length
> + * @len:	Length of the reg property to be validated (bytes). Can be composed
> + *		by more than one reg addr+size pairs
> + * @calc_addr:	the number of address cells expected by the parent of the node
> + *		containing the reg property. Currently it's just used as in param
> + * @calc_size:	the number of size cells expected by the parent of the node
> + *		containing the reg property (out param)
> + *
> + * This function tries to find the correct #size-cells number for the size portion
> + * of a reg property, assuming the #address-cells passed in (calc_addr param)
> + * is valid to avoid ambiguity.
> + * Parent_len is the length in bytes of a single reg property * as expected by the
> + * parent.
> + * Either len is a multiple of parent_len, in which case there's no adjustment to
> + * be made to calc_size and the region is automatically valid (this choice has the
> + * priority), or it is not a multiple.
> + * In the latter case, it finds the smallest calc_addr+calc_size for which len
> + * is still a multiple and adjust calc_size accordingly.
> + * The rationale is to avoid nonsensical combo e.g. #adress-cells 1 and #size-cells
> + * 2 since it's not fully addressable, and to promote any other combo that is a
> + * subset of the original space, e.g. with calc_addr=2 and calc_size=2, returning
> + * calc_size=1 still makes sense since the region is included in the parent space.
> + * The reason for that is to avoid dropping perfectly valid memory regions that
> + * could just have been passed with the wrong format in the reg property (some fw
> + * are reportedly doing that when updating the DT at boot).
> + *
> + * Returns:	true if the region is valid and can be further processed,
> + *		false otherwise. If valid, calc_size is filled with the actual
> + *		length (in cells) of the size part.
> + *
> + */
> +static bool reg_len_valid(int len, const int *calc_addr, int *calc_size)
> +{
> +	int parent_len = (*calc_addr + *calc_size) * sizeof(__be32);
> +	bool parent_multiple = (len % parent_len) / sizeof(__be32);
> +	int row_n, calc_row_len = parent_len / sizeof(__be32);
> +	int len_b = len / sizeof(__be32);
> +
> +	if (!len || !parent_len)
> +		return false;
> +
> +	for (row_n = len_b / 2; row_n > 0; row_n--) {
> +		int tmp_row_len = len_b / row_n;
> +
> +		if (calc_row_len > tmp_row_len &&
> +		    tmp_row_len > *calc_addr &&
> +		    (len_b % tmp_row_len == 0))
> +			calc_row_len = tmp_row_len;
> +	}
> +
> +	if (parent_multiple && calc_row_len != parent_len / sizeof(__be32)) {
> +		*calc_size = calc_row_len - *calc_addr;
> +		return true;
> +	} else if (!parent_multiple) {
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
>  static int __init early_init_dt_reserve_memory(phys_addr_t base,
>  					       phys_addr_t size, bool nomap)
>  {
> @@ -154,9 +216,9 @@ static int __init early_init_dt_reserve_memory(phys_addr_t base,
>  static int __init __reserved_mem_reserve_reg(unsigned long node,
>  					     const char *uname)
>  {
> -	int t_len = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
> +	int calc_addr, calc_size;
>  	phys_addr_t base, size;
> -	int len;
> +	int len, t_len;
>  	const __be32 *prop;
>  	bool nomap;
>  
> @@ -164,17 +226,20 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
>  	if (!prop)
>  		return -ENOENT;
>  
> -	if (len && len % t_len != 0) {
> +	calc_addr = dt_root_addr_cells;
> +	calc_size = dt_root_size_cells;
> +	if (!reg_len_valid(len, &calc_addr, &calc_size)) {
>  		pr_err("Reserved memory: invalid reg property in '%s', skipping node.\n",
>  		       uname);
>  		return -EINVAL;
>  	}
>  
>  	nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;
> +	t_len = (calc_addr + calc_size) * (int)sizeof(__be32);
>  
>  	while (len >= t_len) {
> -		base = dt_mem_next_cell(dt_root_addr_cells, &prop);
> -		size = dt_mem_next_cell(dt_root_size_cells, &prop);
> +		base = dt_mem_next_cell(calc_addr, &prop);
> +		size = dt_mem_next_cell(calc_size, &prop);
>  
>  		if (size && early_init_dt_reserve_memory(base, size, nomap) == 0) {
>  			/* Architecture specific contiguous memory fixup. */
> @@ -255,6 +320,7 @@ void __init fdt_scan_reserved_mem_reg_nodes(void)
>  	}
>  
>  	fdt_for_each_subnode(child, fdt, node) {
> +		int calc_addr, calc_size;
>  		const char *uname;
>  
>  		prop = of_get_flat_dt_prop(child, "reg", &len);
> @@ -263,19 +329,21 @@ void __init fdt_scan_reserved_mem_reg_nodes(void)
>  		if (!of_fdt_device_is_available(fdt, child))
>  			continue;
>  
> +		calc_addr = dt_root_addr_cells;
> +		calc_size = dt_root_size_cells;
>  		uname = fdt_get_name(fdt, child, NULL);
> -		if (len && len % t_len != 0) {
> +		if (!reg_len_valid(len, &calc_addr, &calc_size)) {
>  			pr_err("Reserved memory: invalid reg property in '%s', skipping node.\n",
>  			       uname);
>  			continue;
>  		}
>  
>  		if (len > t_len)
> -			pr_warn("%s() ignores %d regions in node '%s'\n",
> -				__func__, len / t_len - 1, uname);
> +			pr_warn("%s() ignores %d regions in node '%s'\n", __func__,
> +				len / ((calc_addr + calc_size) * (int)sizeof(__be32)) - 1, uname);
>  
> -		base = dt_mem_next_cell(dt_root_addr_cells, &prop);
> -		size = dt_mem_next_cell(dt_root_size_cells, &prop);
> +		base = dt_mem_next_cell(calc_addr, &prop);
> +		size = dt_mem_next_cell(calc_size, &prop);
>  
>  		if (size)
>  			fdt_reserved_mem_save_node(child, uname, base, size);
> -- 
> 2.35.3
> 

Re: [PATCH] of: reserved_mem: Add heuristic to validate reserved memory regions

[Rob Herring]

On Tue, Oct 14, 2025 at 2:32 AM Andrea della Porta
<andrea.porta@suse.com> wrote:
>
> When parsing static reserved-memory DT nodes, any node with a reg property
> length that is not perfectly conformant is discarded.
> Specifically, any reg property whose length is not a multiple of the parent's
> (#address-cells + #size-cells) is dropped.
>
> Relax this condition (while still treating perfect multiples as having higher
> precedence) by allowing regions that are subsets of the parent's addressable
> space to be considered for inclusion.
> For example, in the following scenario:
>
> / {
>         #address-cells = <0x02>;
>         #size-cells = <0x02>;
>         ...
>
>         reserved-memory {
>                 #address-cells = <0x02>;
>                 #size-cells = <0x02>;
>                 ...
>
>                 nvram {
>                         reg = <0x00 0x3fd16d00 0x37>;
>                         ...
>                 };
>         };
> };
>
> Even though the reg property of the nvram node is not well-formed from a DT
> syntax perspective, it still references a perfectly valid memory region of
> 0x37 bytes that should be reserved.

No it isn't. I could just as easily argue that the reserved size
should be 0x37_00000000 because it's BE data. I have little interest
in supporting incorrect DTs especially generically where we have no
clue what platform needs it and whether we still have to carry the
code. There's enough of that crap with ancient PPC and Sparc systems.

Furthermore, this looks like an abuse of /reserved-memory which should
*only* be holes in what /memory node(s) define. I don't think we
enforce that and I imagine there is lots of abuse.

> This has at least one real-world equivalent on the Raspberry Pi 5, for example,
> on which the firmware incorrectly overwrites the nvram node's reg property
> without taking into account the actual value of the parent's #size-cells.

If we have to support this broken firmware, the kernel should fixup
the entry to be correct.

Rob

Re: [PATCH] of: reserved_mem: Add heuristic to validate reserved memory regions

[Andrea della Porta]

Hi Rob,

On 08:12 Tue 14 Oct     , Rob Herring wrote:
> On Tue, Oct 14, 2025 at 2:32 AM Andrea della Porta
> <andrea.porta@suse.com> wrote:
> >
> > When parsing static reserved-memory DT nodes, any node with a reg property
> > length that is not perfectly conformant is discarded.
> > Specifically, any reg property whose length is not a multiple of the parent's
> > (#address-cells + #size-cells) is dropped.
> >
> > Relax this condition (while still treating perfect multiples as having higher
> > precedence) by allowing regions that are subsets of the parent's addressable
> > space to be considered for inclusion.
> > For example, in the following scenario:
> >
> > / {
> >         #address-cells = <0x02>;
> >         #size-cells = <0x02>;
> >         ...
> >
> >         reserved-memory {
> >                 #address-cells = <0x02>;
> >                 #size-cells = <0x02>;
> >                 ...
> >
> >                 nvram {
> >                         reg = <0x00 0x3fd16d00 0x37>;
> >                         ...
> >                 };
> >         };
> > };
> >
> > Even though the reg property of the nvram node is not well-formed from a DT
> > syntax perspective, it still references a perfectly valid memory region of
> > 0x37 bytes that should be reserved.
> 
> No it isn't. I could just as easily argue that the reserved size
> should be 0x37_00000000 because it's BE data. I have little interest
> in supporting incorrect DTs especially generically where we have no
> clue what platform needs it and whether we still have to carry the
> code. There's enough of that crap with ancient PPC and Sparc systems.

I understand the pain, but IIUC the example you mentioned (0x37 0x00) deals
with an incorrect size value (due to endianness) over a correct size length
(#size-cells = 2), while the case this patch tries to address is the opposite,
i.e. correct size values (corrected by the fw) over an incorrect size length. 
For the former issue, the actual kernel code does not have an answer yet. For
the latter I propose this patch.
The point is that the potential erroneous regions we could introduce with this
patch are just a subset of the regions that can be erroneously introduced in
the actual kernel, so no additional harm could be done.

> 
> Furthermore, this looks like an abuse of /reserved-memory which should
> *only* be holes in what /memory node(s) define. I don't think we
> enforce that and I imagine there is lots of abuse.

AFAIK the only enforcement in the kernel is being an integer multiple of the
root address + size cells. As you already pointed out, this means easy abuse
but this is still a fact with the current kernel, not something that would
be exploitable more easily with this patch.

> 
> > This has at least one real-world equivalent on the Raspberry Pi 5, for example,
> > on which the firmware incorrectly overwrites the nvram node's reg property
> > without taking into account the actual value of the parent's #size-cells.
> 
> If we have to support this broken firmware, the kernel should fixup
> the entry to be correct.

This is what I first thought of, but it has several issues that complicates
its implementation:

- I guess there's no current infrastructure to execute fw specific code in
  the reserved-memory node (something that resembles PCI quirks?)

- Finding out whether a fix is required depends on identifying the fw, which is
  possible only reading its fingerprint through the reserved-memory region
  itself. This is kinda of a recursive problem...

- The reserved memory parsing function is invoked very early in the boot process,
  so we cannot rely on a driver module to amend that

I will try to cook up something on this line, but I guess it will not be easy.

Many thanks,
Andrea

> 
> Rob

Re: [PATCH] of: reserved_mem: Add heuristic to validate reserved memory regions

[Rob Herring]

On Tue, Oct 14, 2025 at 9:24 AM Andrea della Porta
<andrea.porta@suse.com> wrote:
>
> Hi Rob,
>
> On 08:12 Tue 14 Oct     , Rob Herring wrote:
> > On Tue, Oct 14, 2025 at 2:32 AM Andrea della Porta
> > <andrea.porta@suse.com> wrote:
> > >
> > > When parsing static reserved-memory DT nodes, any node with a reg property
> > > length that is not perfectly conformant is discarded.
> > > Specifically, any reg property whose length is not a multiple of the parent's
> > > (#address-cells + #size-cells) is dropped.
> > >
> > > Relax this condition (while still treating perfect multiples as having higher
> > > precedence) by allowing regions that are subsets of the parent's addressable
> > > space to be considered for inclusion.
> > > For example, in the following scenario:
> > >
> > > / {
> > >         #address-cells = <0x02>;
> > >         #size-cells = <0x02>;
> > >         ...
> > >
> > >         reserved-memory {
> > >                 #address-cells = <0x02>;
> > >                 #size-cells = <0x02>;
> > >                 ...
> > >
> > >                 nvram {
> > >                         reg = <0x00 0x3fd16d00 0x37>;
> > >                         ...
> > >                 };
> > >         };
> > > };
> > >
> > > Even though the reg property of the nvram node is not well-formed from a DT
> > > syntax perspective, it still references a perfectly valid memory region of
> > > 0x37 bytes that should be reserved.
> >
> > No it isn't. I could just as easily argue that the reserved size
> > should be 0x37_00000000 because it's BE data. I have little interest
> > in supporting incorrect DTs especially generically where we have no
> > clue what platform needs it and whether we still have to carry the
> > code. There's enough of that crap with ancient PPC and Sparc systems.
>
> I understand the pain, but IIUC the example you mentioned (0x37 0x00) deals
> with an incorrect size value (due to endianness) over a correct size length
> (#size-cells = 2), while the case this patch tries to address is the opposite,
> i.e. correct size values (corrected by the fw) over an incorrect size length.
> For the former issue, the actual kernel code does not have an answer yet. For
> the latter I propose this patch.

No, my point was who is to say the error is not 'reg' was treated as
if #size-cells was 1, but rather 'reg' was truncated by 1 cell by
mistake. You don't know (in general) which one it is.

> The point is that the potential erroneous regions we could introduce with this
> patch are just a subset of the regions that can be erroneously introduced in
> the actual kernel, so no additional harm could be done.

There's little reason for us to handle such an error as there is
little excuse for getting it wrong. We have multiple tools that check
this including the kernel evidently.

> > Furthermore, this looks like an abuse of /reserved-memory which should
> > *only* be holes in what /memory node(s) define. I don't think we
> > enforce that and I imagine there is lots of abuse.
>
> AFAIK the only enforcement in the kernel is being an integer multiple of the
> root address + size cells. As you already pointed out, this means easy abuse
> but this is still a fact with the current kernel, not something that would
> be exploitable more easily with this patch.
>
> >
> > > This has at least one real-world equivalent on the Raspberry Pi 5, for example,
> > > on which the firmware incorrectly overwrites the nvram node's reg property
> > > without taking into account the actual value of the parent's #size-cells.
> >
> > If we have to support this broken firmware, the kernel should fixup
> > the entry to be correct.
>
> This is what I first thought of, but it has several issues that complicates
> its implementation:
>
> - I guess there's no current infrastructure to execute fw specific code in
>   the reserved-memory node (something that resembles PCI quirks?)

Not there specifically, but PPC does do a number of fixups.

> - Finding out whether a fix is required depends on identifying the fw, which is
>   possible only reading its fingerprint through the reserved-memory region
>   itself. This is kinda of a recursive problem...

If RPi5, then check and fix 'reg' length in /reserved-memory nodes.
That doesn't seem hard.

> - The reserved memory parsing function is invoked very early in the boot process,
>   so we cannot rely on a driver module to amend that

Does it need to be? If the region truly isn't in DRAM, then we don't
really need to fix it early.

> I will try to cook up something on this line, but I guess it will not be easy.

I pushed a branch, dt/fixup-infrastruct, to my kernel.org tree. It's a
prototype that we ended up not using. It won't work for you if we need
to fixup the fdt rather than the unflattened tree.

Rob

Re: [PATCH] of: reserved_mem: Add heuristic to validate reserved memory regions

[Andrea della Porta]

Hi Rob,

On 11:25 Tue 14 Oct     , Rob Herring wrote:
> On Tue, Oct 14, 2025 at 9:24 AM Andrea della Porta
> <andrea.porta@suse.com> wrote:
> >
> > Hi Rob,
> >
> > On 08:12 Tue 14 Oct     , Rob Herring wrote:
> > > On Tue, Oct 14, 2025 at 2:32 AM Andrea della Porta
> > > <andrea.porta@suse.com> wrote:
> > > >
> > > > When parsing static reserved-memory DT nodes, any node with a reg property
> > > > length that is not perfectly conformant is discarded.
> > > > Specifically, any reg property whose length is not a multiple of the parent's
> > > > (#address-cells + #size-cells) is dropped.
> > > >
> > > > Relax this condition (while still treating perfect multiples as having higher
> > > > precedence) by allowing regions that are subsets of the parent's addressable
> > > > space to be considered for inclusion.
> > > > For example, in the following scenario:
> > > >
> > > > / {
> > > >         #address-cells = <0x02>;
> > > >         #size-cells = <0x02>;
> > > >         ...
> > > >
> > > >         reserved-memory {
> > > >                 #address-cells = <0x02>;
> > > >                 #size-cells = <0x02>;
> > > >                 ...
> > > >
> > > >                 nvram {
> > > >                         reg = <0x00 0x3fd16d00 0x37>;
> > > >                         ...
> > > >                 };
> > > >         };
> > > > };
> > > >
> > > > Even though the reg property of the nvram node is not well-formed from a DT
> > > > syntax perspective, it still references a perfectly valid memory region of
> > > > 0x37 bytes that should be reserved.
> > >
> > > No it isn't. I could just as easily argue that the reserved size
> > > should be 0x37_00000000 because it's BE data. I have little interest
> > > in supporting incorrect DTs especially generically where we have no
> > > clue what platform needs it and whether we still have to carry the
> > > code. There's enough of that crap with ancient PPC and Sparc systems.
> >
> > I understand the pain, but IIUC the example you mentioned (0x37 0x00) deals
> > with an incorrect size value (due to endianness) over a correct size length
> > (#size-cells = 2), while the case this patch tries to address is the opposite,
> > i.e. correct size values (corrected by the fw) over an incorrect size length.
> > For the former issue, the actual kernel code does not have an answer yet. For
> > the latter I propose this patch.
> 
> No, my point was who is to say the error is not 'reg' was treated as
> if #size-cells was 1, but rather 'reg' was truncated by 1 cell by
> mistake. You don't know (in general) which one it is.

Ok, general case can have ambiguity, got it. Since this seems to be a dead end,
I will abandon the heuristic path in favor of fixing the specific (Rpi5) case.

> 
> > The point is that the potential erroneous regions we could introduce with this
> > patch are just a subset of the regions that can be erroneously introduced in
> > the actual kernel, so no additional harm could be done.
> 
> There's little reason for us to handle such an error as there is
> little excuse for getting it wrong. We have multiple tools that check
> this including the kernel evidently.
> 
> > > Furthermore, this looks like an abuse of /reserved-memory which should
> > > *only* be holes in what /memory node(s) define. I don't think we
> > > enforce that and I imagine there is lots of abuse.
> >
> > AFAIK the only enforcement in the kernel is being an integer multiple of the
> > root address + size cells. As you already pointed out, this means easy abuse
> > but this is still a fact with the current kernel, not something that would
> > be exploitable more easily with this patch.
> >
> > >
> > > > This has at least one real-world equivalent on the Raspberry Pi 5, for example,
> > > > on which the firmware incorrectly overwrites the nvram node's reg property
> > > > without taking into account the actual value of the parent's #size-cells.
> > >
> > > If we have to support this broken firmware, the kernel should fixup
> > > the entry to be correct.
> >
> > This is what I first thought of, but it has several issues that complicates
> > its implementation:
> >
> > - I guess there's no current infrastructure to execute fw specific code in
> >   the reserved-memory node (something that resembles PCI quirks?)
> 
> Not there specifically, but PPC does do a number of fixups.
> 
> > - Finding out whether a fix is required depends on identifying the fw, which is
> >   possible only reading its fingerprint through the reserved-memory region
> >   itself. This is kinda of a recursive problem...
> 
> If RPi5, then check and fix 'reg' length in /reserved-memory nodes.
> That doesn't seem hard.

Maybe we can workaround this by just checking if the reserved node has 
"raspberrypi,bootloader-config" in the compatible list *and* the reg property
is not aligned with the #size-cells advertised by the parent. In this case we
are surely dealing with the misbehaving fw and we can act accordingly.
Thanks for the heads-up.

> 
> > - The reserved memory parsing function is invoked very early in the boot process,
> >   so we cannot rely on a driver module to amend that
> 
> Does it need to be? If the region truly isn't in DRAM, then we don't
> really need to fix it early.

I think we should. The reseved memory regions are outlined during the first-pass
parsing of the fdt and the nvmem-rmem driver will not work if it wouldn't find
a matching region. I think there's no point in rewriting this logic, let's just
fix the entry sooner rather than later. As an added benefit, whoever will need
to dump the fdt at later time can do so and have the reg property matching its
live tree counterpart. Need to do some testing to verify whether its already doable
or if it needs some new helper functions to deal with the fdt.

> 
> > I will try to cook up something on this line, but I guess it will not be easy.
> 
> I pushed a branch, dt/fixup-infrastruct, to my kernel.org tree. It's a
> prototype that we ended up not using. It won't work for you if we need
> to fixup the fdt rather than the unflattened tree.

Thanks for that. As said above though, I'd rather fix it early in the fdt rather
than in the live tree, although the actions in your proposed branch have direct
equivalent in the fdt world (or at least they should, I need to verify), so the
trace is still good to follow.
I'll bake something and come back with a renovate V2.

Many thanks,
Andrea

> 
> Rob