Re: [PATCH v3 11/15] libfdt: overlay: Update base /__local_fixups__ on apply

[Doug Anderson]

Hi Simon,

On Wed, Nov 4, 2026 at 6:08 AM Simon Glass <sjg@xxxxxxxxxxxx> wrote:
>
> > +               name = fdt_get_name(fdt, ancestor, &namelen);
> > +               if (!name)
> > +                       return namelen;
> > +
> > +               next_lf = fdt_subnode_offset_namelen(fdt, node, name, namelen);
> > +               if (next_lf == -FDT_ERR_NOTFOUND && create) {
> > +                       int old_struct_size = fdt_size_dt_struct(fdt);
> > +
> > +                       next_lf = fdt_add_subnode_namelen(fdt, node, name,
> > +                                                         namelen);
> > +                       if (next_lf < 0)
> > +                               return next_lf;
> > +                       if (next_lf <= ancestor) {
> > +                               int delta = fdt_size_dt_struct(fdt) -
> > +                                           old_struct_size;
> > +                               char *new_name;
> > +
> > +                               target += delta;
> > +                               ancestor += delta;
> > +                               name = fdt_get_name(fdt, ancestor, NULL);
> > +                               new_name = (char *)(uintptr_t)fdt_get_name(
> > +                                       fdt, next_lf, NULL);
> > +                               memcpy(new_name, name, namelen + 1);
> > +                       }
>
> Almost, but the pointer passed in still goes stale (my suggestion of
> re-reading the name missed this). When /__local_fixups__ comes before
> the target in the blob, 'name' points into data which the splice in
> fdt_add_subnode_namelen() moves, so its memcpy() reads from the wrong
> place and can overlap the destination. The final tree is correct
> because this code rewrites the name afterwards, but the overlap is
> undefined behaviour. With ASan and -fno-builtin-memcpy, I get
> memcpy-param-overlap at fdt_rw.c:405 for a base with /__local_fixups__
> immediately before the target node, where the name is longer than 4
> characters.
>
> Please can you avoid passing a pointer into the blob? A small internal
> helper which does the splice and then copies the name from its new
> location would also drop the cast, although it needs care not to add
> size to IGNORE builds. Using memmove() in fdt_add_subnode_namelen() is
> the smallest fix, but it still copies a stale name. What do you think?

Got it. Splitting out an internal helper `fdt_add_empty_subnode_()`
in `libfdt/fdt_rw.c` (and having `fdt_add_subnode_namelen()` call it)
works really nicely here. Since `fdt_add_subnode_namelen()` shares the
same implementation via `fdt_add_empty_subnode_()`, there is no code
duplication in `fdt_rw.c`, and it also avoids repeating
`fdt_subnode_offset_namelen()` and drops the cast:

In `libfdt/fdt_rw.c`:

+int fdt_add_empty_subnode_(void *fdt, int parentoffset, int namelen,
+                          char **namep)
+{
+       struct fdt_node_header *nh;
+       int offset, nextoffset;
+       int nodelen;
+       int err;
+       uint32_t tag;
+       fdt32_t *endtag;
+
+       /* Try to place the new node after the parent's properties */
+       tag = fdt_next_tag(fdt, parentoffset, &nextoffset);
+       /* the fdt_subnode_offset_namelen() should ensure this never hits */
+       if (!can_assume(LIBFDT_FLAWLESS) && (tag != FDT_BEGIN_NODE))
+               return -FDT_ERR_INTERNAL;
+       do {
+               offset = nextoffset;
+               tag = fdt_next_tag(fdt, offset, &nextoffset);
+       } while ((tag == FDT_PROP) || (tag == FDT_NOP));
+
+       nh = fdt_offset_ptr_w_(fdt, offset);
+       nodelen = sizeof(*nh) + FDT_TAGALIGN(namelen+1) + FDT_TAGSIZE;
+
+       err = fdt_splice_struct_(fdt, nh, 0, nodelen);
+       if (err)
+               return err;
+
+       nh->tag = cpu_to_fdt32(FDT_BEGIN_NODE);
+       memset(nh->name, 0, FDT_TAGALIGN(namelen+1));
+       endtag = (fdt32_t *)((char *)nh + nodelen - FDT_TAGSIZE);
+       *endtag = cpu_to_fdt32(FDT_END_NODE);
+       *namep = nh->name;
+
+       return offset;
+}

And in `overlay_fragment_local_fixups()` in `libfdt/fdt_overlay.c`:

                next_lf = fdt_subnode_offset_namelen(fdt, node, name, namelen);
                if (next_lf == -FDT_ERR_NOTFOUND && create) {
                        int old_struct_size = fdt_size_dt_struct(fdt);
                        char *new_name;

                        next_lf = fdt_add_empty_subnode_(fdt, node, namelen,
                                                         &new_name);
                        if (next_lf < 0)
                                return next_lf;
                        if (next_lf <= ancestor) {
                                int delta = fdt_size_dt_struct(fdt) -
                                            old_struct_size;

                                target += delta;
                                ancestor += delta;
                                name = fdt_get_name(fdt, ancestor, NULL);
                        }
                        memcpy(new_name, name, namelen);
                } else if (next_lf < 0) {
                        return next_lf;
                }

> Also, gcov shows that the tests never reach this creation code, in
> either direction. As far as I can tell, each target is either the root
> or already has a node under /__local_fixups__. Please can you add a
> case where the target has no such node, with /__local_fixups__ both
> before and after it in the blob? tests/overlay.c already creates it
> first in the root, so that ordering should be easy to cover there.

Done in my draft v4. Both `tests/overlay.c` and `tests/run_tests.sh` had
pre-created `/__local_fixups__/test-node` before applying the overlay,
so `overlay_fragment_local_fixups()` always found `test-node` already
present. I've added `fdt_overlay_local_fixups_create()` in
`tests/overlay.c` to test an empty `/__local_fixups__` (with no
`test-node` child) both before `/test-node` (`lf < tn`) and after
`/test-node` (`lf > tn`) in the blob.


> > +    # verify that /__local_fixups__ in a -L base blob is updated when applying an overlay
> > +    run_dtc_test -@ -L -I dts -O dtb -o overlay_base_phandle_lf.test.dtb "$SRCDIR/overlay_base_phandle.dts"
> > +    run_wrap_test $FDTOVERLAY -i overlay_base_phandle_lf.test.dtb -o overlay_base_phandle_lfO.test.dtb overlay_overlay_phandle.test.dtb
> > +    run_fdtget_test 0 overlay_base_phandle_lfO.test.dtb /__local_fixups__/b a
> > +    run_fdtget_test 0 overlay_base_phandle_lfO.test.dtb /__local_fixups__/b d
> > +    run_fdtget_test 0 overlay_base_phandle_lfO.test.dtb /__local_fixups__/c b
> > +    run_fdtget_test 0 overlay_base_phandle_lfO.test.dtb /__local_fixups__/d a
> > +    run_fdtget_test 0 overlay_base_phandle_lfO.test.dtb /__local_fixups__/d b
> > +    run_fdtget_test 0 overlay_base_phandle_lfO.test.dtb /__local_fixups__/d c
>
> These assume UPDATE, so 'meson test' fails for me with
> -Doverlay-local-fixups=0 (7 failures in the fdtoverlay suite).
> tests/overlay.c handles each mode with #if, so please can you do the
> same here, e.g. by passing the setting to run_tests.sh and skipping or
> adjusting these checks?

Done in my draft v4: `tests/meson.build` and `tests/Makefile.tests` now
pass `OVERLAY_LOCAL_FIXUPS` to `tests/run_tests.sh`, and both
`fdtoverlay_tests` and `fdtfactor_tests` adjust their `/__local_fixups__`
checks so `meson test` passes under `-Doverlay-local-fixups=0`, `1`,
and `2`.


-Doug