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

[Simon Glass]

Hi Douglas,

On 2026-11-01T22:01:07, Douglas Anderson <dianders@xxxxxxxxxxxx> wrote:
> tree matches what `dtc -L` produces).
>
> `fdt_overlay_apply()` updates the base tree's `/__symbols__` via
> `overlay_symbol_update()`, but leaves the base tree's
> `/__local_fixups__` untouched. As a result, applying an overlay to a
> base tree with `/__local_fixups__` leaves `/__local_fixups__` missing
> the phandle references introduced by the overlay (both from the
> overlay's `/__local_fixups__` and from its resolved `/__fixups__`), and
> leaves stale offsets if the overlay overwrites a base property that
> previously had phandle references.
>
> Add `overlay_local_fixups_update()`, controlled by a 3-state
> [...]
>
>  Makefile                 |   6 +-
>  libfdt/fdt_overlay.c     | 527 ++++++++++++++++++++++++++++++++++++++++++++++-
>  libfdt/libfdt.h          |   7 +
>  libfdt/libfdt_internal.h |  21 ++
>  meson.build              |   5 +
>  meson_options.txt        |   2 +
>  tests/overlay.c          |  80 +++++++
>  tests/run_tests.sh       |  19 ++
>  8 files changed, 663 insertions(+), 4 deletions(-)

> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> @@ -1135,6 +1134,524 @@ static int overlay_symbol_update(void *fdt, void *fdto)
> +			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?

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.

> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> @@ -1141,6 +1141,25 @@ fdtoverlay_tests() {
> +    # 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

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?

Regards,
Simon