Re: [PATCH v3 01/15] libfdt: overlay: Fix out-of-bounds write on bad fixup offset

From: David Gibson

Date: Sun Nov 01 2026 - 23:29:26 EST

On Sun, Nov 01, 2026 at 02:01:08PM -0800, Douglas Anderson wrote:
> A malformed overlay with a negative or very large fixup offset in
> /__fixups__ (such as "/fragment@0:target:-4" or
> "/fragment@0:target:4294967292") can cause an out-of-bounds write during
> overlay application:
>
> 1. overlay_fixup_phandle() parses the offset with strtoul(), which
> accepts negative numbers and values larger than UINT32_MAX, and
> truncates the result into a 32-bit integer.
> 2. fdt_setprop_inplace_namelen_partial() checks bounds with
> `(unsigned)proplen < (len + idx)`, where `len + idx` wraps around to
> a small number when `idx` is near UINT32_MAX, bypassing the check and
> passing the huge offset to memcpy().
>
> Tighten the fixup offset parsing in overlay_fixup_phandle(), fix the
> bounds check in fdt_setprop_inplace_namelen_partial() to avoid unsigned
> wrap-around, and add test cases covering both negative and overflowing
> offsets.
>
> Assisted-by: Gemini:gemini-pro-prerelease
> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>

Reviewed-by: David Gibson <david@xxxxxxxxxxxxxxxxxxxxx>

Thanks for this - this bug has been reported by a couple of people,
and I hadn't gotten around to making my own fix.

> ---
>
> Changes in v3:
> - Split the fixup offset overflow fix and fdt_wip.c bounds check out of
> "libfdt: overlay: Prepare fixup and fragment path helpers for reuse"
> into a standalone bugfix patch with dedicated tests (Simon Glass)
> - Add a comment explaining why an initial ASCII digit check is needed
> before strtoul() (David Gibson)
>
> libfdt/fdt_overlay.c | 34 +++++++++++++++++-----
> libfdt/fdt_wip.c | 7 ++++-
> tests/overlay_bad_fixup_negative_index.dts | 9 ++++++
> tests/overlay_bad_fixup_overflow_index.dts | 9 ++++++
> tests/run_tests.sh | 2 ++
> tests/setprop_inplace.c | 8 +++++
> 6 files changed, 60 insertions(+), 9 deletions(-)
> create mode 100644 tests/overlay_bad_fixup_negative_index.dts
> create mode 100644 tests/overlay_bad_fixup_overflow_index.dts
>
> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> index 51a3859620a4..c6dc4774cff3 100644
> --- a/libfdt/fdt_overlay.c
> +++ b/libfdt/fdt_overlay.c
> @@ -330,10 +330,10 @@ static int overlay_update_local_references(void *fdto, uint32_t delta)
> static int overlay_fixup_one_phandle(void *fdto, int symbols_off,
> const char *path, uint32_t path_len,
> const char *name, uint32_t name_len,
> - int poffset, uint32_t phandle)
> + uint32_t poffset, uint32_t phandle)
> {
> fdt32_t phandle_prop;
> - int fixup_off;
> + int fixup_off, ret;
>
> if (symbols_off < 0)
> return symbols_off;
> @@ -345,10 +345,20 @@ static int overlay_fixup_one_phandle(void *fdto, int symbols_off,
> return fixup_off;
>
> phandle_prop = cpu_to_fdt32(phandle);
> - return fdt_setprop_inplace_namelen_partial(fdto, fixup_off,
> - name, name_len, poffset,
> - &phandle_prop,
> - sizeof(phandle_prop));
> + ret = fdt_setprop_inplace_namelen_partial(fdto, fixup_off,
> + name, name_len, poffset,
> + &phandle_prop,
> + sizeof(phandle_prop));
> + /*
> + * fdt_setprop_inplace_namelen_partial() only returns -FDT_ERR_NOSPACE
> + * when poffset + sizeof(phandle_prop) exceeds the existing property's
> + * length in the overlay, which means the overlay is malformed (not
> + * that the base DTB buffer ran out of space).
> + */
> + if (ret == -FDT_ERR_NOSPACE)
> + return -FDT_ERR_BADOVERLAY;
> +
> + return ret;
> }
>
> /**
> @@ -409,7 +419,8 @@ static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
> uint32_t fixup_len;
> const char *sep;
> char *endptr;
> - int poffset, ret;
> + unsigned long poffset;
> + int ret;
>
> fixup_end = memchr(value, '\0', len);
> if (!fixup_end)
> @@ -438,8 +449,15 @@ static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
> if (!name_len)
> return -FDT_ERR_BADOVERLAY;
>
> + /*
> + * Require an initial ASCII digit because strtoul() accepts
> + * leading whitespace and '+'/'-' signs (wrapping negative
> + * values to unsigned long).
> + */
> + 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 > UINT32_MAX))
> return -FDT_ERR_BADOVERLAY;
>
> ret = overlay_fixup_one_phandle(fdto, symbols_off,
> diff --git a/libfdt/fdt_wip.c b/libfdt/fdt_wip.c
> index c2d7566a67dc..dd163d91498c 100644
> --- a/libfdt/fdt_wip.c
> +++ b/libfdt/fdt_wip.c
> @@ -23,7 +23,12 @@ int fdt_setprop_inplace_namelen_partial(void *fdt, int nodeoffset,
> if (!propval)
> return proplen;
>
> - if ((unsigned)proplen < (len + idx))
> + /*
> + * Check that idx + len fits within proplen using subtraction rather
> + * than addition so that a large idx (or negative len) cannot wrap
> + * around 32-bit unsigned arithmetic.
> + */
> + if (idx > (unsigned)proplen || (unsigned)len > (unsigned)proplen - idx)
> return -FDT_ERR_NOSPACE;
>
> memcpy((char *)propval + idx, val, len);
> diff --git a/tests/overlay_bad_fixup_negative_index.dts b/tests/overlay_bad_fixup_negative_index.dts
> new file mode 100644
> index 000000000000..02ccee905e85
> --- /dev/null
> +++ b/tests/overlay_bad_fixup_negative_index.dts
> @@ -0,0 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/include/ "overlay_bad_fixup_base.dtsi"
> +
> +/ {
> + __fixups__ {
> + test = "/fragment@0:target:-4";
> + };
> +};
> diff --git a/tests/overlay_bad_fixup_overflow_index.dts b/tests/overlay_bad_fixup_overflow_index.dts
> new file mode 100644
> index 000000000000..d7d05267d7b3
> --- /dev/null
> +++ b/tests/overlay_bad_fixup_overflow_index.dts
> @@ -0,0 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/include/ "overlay_bad_fixup_base.dtsi"
> +
> +/ {
> + __fixups__ {
> + test = "/fragment@0:target:4294967292";
> + };
> +};
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 5bc7817dd12a..0d815e307909 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -274,6 +274,8 @@ BAD_FIXUP_TREES="bad_index \
> empty \
> empty_index \
> index_trailing \
> + negative_index \
> + overflow_index \
> path_empty_prop \
> path_only \
> path_only_sep \
> diff --git a/tests/setprop_inplace.c b/tests/setprop_inplace.c
> index 61a0da16fce8..48182169fbf6 100644
> --- a/tests/setprop_inplace.c
> +++ b/tests/setprop_inplace.c
> @@ -80,5 +80,13 @@ int main(int argc, char *argv[])
> check_getprop(fdt, 0, "compatible", strlen(TEST_STRING_4_RESULT) + 1,
> TEST_STRING_4_RESULT);
>
> + err = fdt_setprop_inplace_namelen_partial(fdt, 0, "compatible",
> + strlen("compatible"),
> + 0xfffffffcU,
> + TEST_STRING_4_PARTIAL, 4);
> + if (err != -FDT_ERR_NOSPACE)
> + FAIL("fdt_setprop_inplace_namelen_partial() with wrapping idx returned %d instead of -FDT_ERR_NOSPACE\n",
> + err);
> +
> PASS();
> }
> --
> 2.56.0.317.g454f8302e1-goog
>
>

--
David Gibson (he or they) | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you, not the other way
| around.
http://www.ozlabs.org/~dgibson

Attachment: signature.asc
Description: PGP signature