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

[Doug Anderson]

Hi Simon,

On Fri, Oct 30, 2026 at 5:22 AM Simon Glass <sjg@xxxxxxxxxxxx> wrote:
>
> On Wed, 28 Oct 2026 at 15:56, Douglas Anderson <dianders@xxxxxxxxxxxx> wrote:
> >
> > Tighten `overlay_parse_fixup_entry()` and `overlay_split_fragment_path()`
> > before reusing them to update the base tree's `/__local_fixups__`:
> >
> > - In `overlay_parse_fixup_entry()`, store the `strtoul()` result in an
> >   `unsigned long` before range-checking against `INT32_MAX -
> >   sizeof(uint32_t)`, and require the offset field to begin with an ASCII
> >   digit (`'0'`..`'9'`) since `strtoul()` accepts leading whitespace and
> >   `+`/`-` signs (wrapping negative values to `unsigned long`).
> > - In `overlay_split_fragment_path()`, reject paths with a trailing slash
> >   after `/__overlay__/` (`rel_path_len == 0`) with `-FDT_ERR_BADVALUE`.
> >
> > Assisted-by: Gemini:gemini-3-pro [edid_decode] [gerrit_and_build_artifacts] [gsheets_gdocs_cli]
> > Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> > ---
> >
> > (no changes since v1)
> >
> >  libfdt/fdt_overlay.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> > index b7dd8fd922eb..a19eb1bc4bd8 100644
> > --- a/libfdt/fdt_overlay.c
> > +++ b/libfdt/fdt_overlay.c
> > @@ -376,7 +376,8 @@ static int overlay_parse_fixup_entry(const char **valuep, int *lenp,
> >         uint32_t fixup_len;
> >         const char *sep;
> >         char *endptr;
> > -       uint32_t poffset;
> > +       unsigned long poffset;
> >
> >         fixup_end = memchr(*valuep, '\0', *lenp);
> >         if (!fixup_end)
> > @@ -404,9 +405,10 @@ static int overlay_parse_fixup_entry(const char **valuep, int *lenp,
> >         if (!name_len)
> >                 return -FDT_ERR_BADOVERLAY;
> >
> > +       if ((*(sep + 1) < '0') || (*(sep + 1) > '9'))
> > +               return -FDT_ERR_BADOVERLAY;
> >         poffset = strtoul(sep + 1, &endptr, 10);
> > -       if ((*endptr != '\0') || (poffset < 0))
> > +       if ((*endptr != '\0') || (poffset > (INT32_MAX - sizeof(uint32_t))))
> >                 return -FDT_ERR_BADOVERLAY;
>
> 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'?
>
> 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.

Great catch on `fdt_setprop_inplace_namelen_partial()`! In v3 I've:
1. Split this into its own patch at the very start of the series ("libfdt:
   overlay: Fix out-of-bounds write on bad fixup offset").
2. Fixed the overflow check in `fdt_setprop_inplace_namelen_partial()` in
   `libfdt/fdt_wip.c` as you suggested.
3. Kept the digit check and a `poffset > UINT32_MAX` bound in
   `libfdt/fdt_overlay.c` (mapping `-FDT_ERR_NOSPACE` from
   `fdt_setprop_inplace_namelen_partial()` to `-FDT_ERR_BADOVERLAY` in
   `overlay_fixup_one_phandle()`).
4. Added tests in `tests/setprop_inplace.c` and `tests/run_tests.sh`
   (`overlay_bad_fixup_negative_index.dts` and
   `overlay_bad_fixup_overflow_index.dts`).


-Doug