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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 7604 bytes --]

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@chromium.org>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]