[PATCH] of: fdt: reject oversized unflattened trees

[PATCH] of: fdt: reject oversized unflattened trees

[Sung Byeongchan]

unflatten_dt_nodes() computes the expanded tree size by advancing a dry
pointer cursor, but returns the result as int. A valid flattened tree whose
expanded nodes and properties exceed the signed range can consequently
wrap to a small positive allocation before the full second write pass.

On a 64-bit UML build, a 157,133,160-byte FDT expanded to 4,294,971,394
bytes and narrowed to 4,098. The undersized write pass left a corrupted OF
tree whose property pointer later caused a kernel segfault in
__of_add_property_sysfs(), reproducibly in three fresh processes.

Reject the dry scan before its cursor can exceed the signed caller extent,
leaving room for the following alignment and four-byte end marker. This
prevents the write pass from starting with a narrowed allocation.

The fixed target rejected the trigger before allocation or write in three
of three fresh processes. Normal, near-boundary, malformed-header and
recovery controls passed in both lanes.

The demonstrated impact is host or VMM boot input corrupting and crashing
a guest kernel. Information disclosure, controlled write, code execution,
host escape, RCE and LPE were not demonstrated.

Assisted-by: OpenAI Codex

Signed-off-by: Sung Byeongchan <tjdqudcks0424@naver.com>
---
 drivers/of/fdt.c |    5 +++++
 1 file changed, 5 insertions(+)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index ecc9231026224..135502b882020 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -310,6 +310,11 @@ static int unflatten_dt_nodes(const void *blob,
 				   &nps[depth+1], dryrun);
 		if (ret < 0)
 			return ret;
+		/* Leave room for the alignment and end marker below. */
+		if (dryrun && (unsigned long)mem > INT_MAX - 7) {
+			pr_err("Unflattened device tree is too large\n");
+			return -EOVERFLOW;
+		}
 
 		if (!dryrun && nodepp && !*nodepp)
 			*nodepp = nps[depth+1];

base-commit: d48723711bae9b8cdc56719c448ba42fef28cbee
-- 
2.43.0

Re: [PATCH] of: fdt: reject oversized unflattened trees

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] of: fdt: 32-bit pointer wrap of mem in populate_node()
  bypasses size check
- [Low] of: fdt: missing Fixes trailer for commit 50800082f176
--

commit 473f33a66107c250c3f97cdffe2600a50ef45e63
Author: Sung Byeongchan <tjdqudcks0424@naver.com>

> of: fdt: reject oversized unflattened trees
>
> unflatten_dt_nodes() computes the expanded tree size by advancing a dry
> pointer cursor, but returns the result as int. A valid flattened tree whose
> expanded nodes and properties exceed the signed range can consequently
> wrap to a small positive allocation before the full second write pass.
>
> On a 64-bit UML build, a 157,133,160-byte FDT expanded to 4,294,971,394
> bytes and narrowed to 4,098. The undersized write pass left a corrupted OF
> tree whose property pointer later caused a kernel segfault in
> __of_add_property_sysfs(), reproducibly in three fresh processes.
>
> Reject the dry scan before its cursor can exceed the signed caller extent,
> leaving room for the following alignment and four-byte end marker. This
> prevents the write pass from starting with a narrowed allocation.
>
> The fixed target rejected the trigger before allocation or write in three
> of three fresh processes. Normal, near-boundary, malformed-header and
> recovery controls passed in both lanes.
>
> The demonstrated impact is host or VMM boot input corrupting and crashing
> a guest kernel. Information disclosure, controlled write, code execution,
> host escape, RCE and LPE were not demonstrated.
>
> Assisted-by: OpenAI Codex
>
> Signed-off-by: Sung Byeongchan <tjdqudcks0424@naver.com>

[Severity: Low]
Since this fixes the int narrowing of the dry-run size introduced when
unflatten_dt_node() was converted to return int, should this include a
Fixes: tag?

  Fixes: 50800082f176 ("drivers/of: Avoid recursively calling unflatten_dt_node()")

> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index ecc9231026224..135502b882020 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -310,6 +310,11 @@ static int unflatten_dt_nodes(const void *blob,
>  				   &nps[depth+1], dryrun);
>  		if (ret < 0)
>  			return ret;
> +		/* Leave room for the alignment and end marker below. */
> +		if (dryrun && (unsigned long)mem > INT_MAX - 7) {
> +			pr_err("Unflattened device tree is too large\n");
> +			return -EOVERFLOW;
> +		}
>  
>  		if (!dryrun && nodepp && !*nodepp)
>  			*nodepp = nps[depth+1];

[Severity: Medium]
On 32-bit architectures, can a single populate_node() call wrap the 32-bit
mem pointer past ULONG_MAX and back below INT_MAX - 7 before this check
runs?

Inside populate_node() -> populate_properties(), unflatten_dt_alloc()
advances *mem for every property without bounds checks, and a node
without an explicit name property also allocates its unit name length
twice (once in populate_node() and again in populate_properties()):

drivers/of/fdt.c:unflatten_dt_alloc() {
	*mem = PTR_ALIGN(*mem, align);
	res = *mem;
	*mem += size;
}

If a node with many small properties or a large unit name advances *mem
from just below INT_MAX - 7 past 0xffffffff into [0, INT_MAX - 7],
unflatten_dt_nodes() returns a small wrapped positive size and the second
pass writes past the end of the allocated buffer.  Checking for overflow
inside unflatten_dt_alloc() before advancing *mem would catch this on
every allocation step.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107093519.618896-1-tjdqudcks0424@naver.com?part=1