Re: [PATCH v2 05/10] libfdt: overlay: Prepare fixup and fragment path helpers for reuse

[Simon Glass]

Hi Doug,

On 2026-10-28T21:46:49, Douglas Anderson <dianders@xxxxxxxxxxxx> wrote:
> libfdt: overlay: Prepare fixup and fragment path helpers for reuse
>
> Clean up `overlay_parse_fixup_entry()` and generalize
> `overlay_split_fragment_path()` in `libfdt/fdt_overlay.c` so both can be
> reused when updating `/__local_fixups__`:
>
> - Change `poffset` from `int` to `uint32_t` in
>   `overlay_fixup_one_phandle()`, `overlay_parse_fixup_entry()`, and
>   `overlay_fixup_phandle()`, and parse into `unsigned long` before
>   validating the range.
> - Require `*(sep + 1)` to be an ASCII digit (`'0'..'9'`) and bound
>   `poffset` to `INT32_MAX - sizeof(uint32_t)` so negative strings (such
>   as `"-4"`), leading whitespace, or large values cannot silently
>   truncate or wrap around in `fdt_setprop_inplace_namelen_partial()`.

This is more than a clean-up: it fixes an existing out-of-bounds write
in fdt_overlay_apply(). With a /__fixups__ entry like
'/fragment@0/__overlay__/node:prop:-4', strtoul() gives -4 as an int,
which reaches fdt_setprop_inplace_namelen_partial() as idx =
0xfffffffc. Then 'len + idx' wraps to 0, the 'proplen < len + idx'
check passes and memcpy() writes 4 bytes before the property value. On
64-bit, '4294967292' does the same. Overlays may well be untrusted, so
please can you split this into its own patch at the start of the
series, with a commit message that starts with the problem so it
doesn't get lost under 'Prepare ... for reuse'?

> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> @@ -398,15 +398,17 @@ static int overlay_parse_fixup_entry(const char **valuep, int *lenp,
> +	if ((*(sep + 1) < '0') || (*(sep + 1) > '9'))
> +		return -FDT_ERR_BADOVERLAY;
>  	poffset = strtoul(sep + 1, &endptr, 10);
> -	if ((*endptr != '\0') || (endptr <= (sep + 1)))
> +	if ((*endptr != '\0') || (poffset > (INT32_MAX - sizeof(uint32_t))))
>  		return -FDT_ERR_BADOVERLAY;

The real cause is the wrapping check in
fdt_setprop_inplace_namelen_partial(). That function is public, so any
caller passing a large idx hits the same overflow. I suggest fixing it
there with something like:

    if (idx > (unsigned)proplen || (unsigned)len > (unsigned)proplen - idx)
        return -FDT_ERR_NOSPACE;

and keeping the digit check here so that '+4' and ' 4' are still
rejected. The INT32_MAX bound then becomes an early rejection rather
than what keeps memory safe. What do you think?

A test in tests/ checking that fdt_overlay_apply() returns
-FDT_ERR_BADOVERLAY for an overlay with a negative or oversized fixup
offset would also be good.

> - Drop the redundant `*sep != ':'` checks after `memchr(..., ':', ...)`.
> - Generalize `overlay_split_fragment_path()` to accept a length-bounded
>   `path` slice that is not necessarily NUL-terminated (using `memchr()`
>   instead of `strchr()`, and keeping the NUL-terminated property check
>   in `overlay_symbol_update()`) so it can also split `:`-delimited paths
>   from `/__fixups__`.
> [...]
>
>  libfdt/fdt_overlay.c | 46 ++++++++++++++++++++++----------------------
>  1 file changed, 24 insertions(+), 22 deletions(-)

Regards,
Simon