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

[Douglas Anderson <dianders@chromium.org>]

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>
---

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