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

[sashiko-bot@kernel.org]

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