[U-Boot] [PATCH] fdt: Fix string property comparison overflow

[Teddy Reed]

FDT property searching can overflow when comparing strings. This will
result in undefined behavior. This upstream patch adds checks to assure
property name lengths do not overrun the string region or the totalsize.

The implementation is from upstream dtc:

70166d62a27f libfdt: Safer access to strings section

Cc: Peter Robinson <pbrobinson at gmail.com>
Cc: David Gibson <david at gibson.dropbear.id.au>
Signed-off-by: Teddy Reed <teddy.reed at gmail.com>
---
Note this file is not synchronized from upstream dtc when using
the scripts/dtc/update-dtc-source.sh script.

The file size of the ELF increases with sandbox_spl_defconfig.

$ bloaty spl/u-boot-spl -- spl/u-boot-spl.bin.before
     VM SIZE                      FILE SIZE
 --------------                --------------
  [ = ]       0 .debug_loc        +948  +0.3%
  [ = ]       0 .debug_info       +284  +0.0%
  +0.5%    +176 .text             +176  +0.5%
  [ = ]       0 .debug_line       +150  +0.2%
  [ = ]       0 .debug_ranges      +48  +0.2%
  +0.4%     +40 .eh_frame          +40  +0.4%
  [ = ]       0 .debug_str         +20  +0.0%
  [ = ]       0 .debug_aranges     +16  +0.1%
   +59%     +16 [LOAD [RX]]        +16   +59%
  [ = ]       0 .strtab             +4  +0.0%
  [ = ]       0 [Unmapped]        -238 -18.1%
  +0.3%    +232 TOTAL          +1.43Ki  +0.1%

$ du -b spl/u-boot-spl.bin spl/u-boot-spl.bin.before
2174032	spl/u-boot-spl.bin
2174032	spl/u-boot-spl.bin.before

You could also apply the patch:

@@ -42,6 +42,11 @@ const char *fdt_string(const void *fdt, int stroffset)
 static int _fdt_string_eq(const void *fdt, int stroffset,
 			  const char *s, int len)
 {
+	int total_off = fdt_off_dt_strings(fdt) + stroffset;
+	if (total_off + len + 1 < total_off ||
+	    total_off + len + 1 > fdt_totalsize(fdt))
+		return 0;
+
 	const char *p = fdt_string(fdt, stroffset);
 
 	return (strnlen(p, len + 1) == len) && (memcmp(p, s, len) == 0);

To mitigate the read overflow, with minimum added bytes. I proposed this
along with another change [1]. This was not a good idea since the change
to scripts/dtc/libfdt/fdt.c is part of dtc synchronized from upsteam.

[1] https://lists.denx.de/pipermail/u-boot/2018-June/330488.html

 lib/libfdt/fdt_ro.c | 61 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 58 insertions(+), 3 deletions(-)

diff --git a/lib/libfdt/fdt_ro.c b/lib/libfdt/fdt_ro.c
index b6ca4e0b0c..f67dcb7fc9 100644
--- a/lib/libfdt/fdt_ro.c
+++ b/lib/libfdt/fdt_ro.c
@@ -34,17 +34,72 @@ static int _fdt_nodename_eq(const void *fdt, int offset,
 		return 0;
 }
 
+const char *fdt_get_string(const void *fdt, int stroffset, int *lenp)
+{
+	uint32_t absoffset = stroffset + fdt_off_dt_strings(fdt);
+	size_t len;
+	int err;
+	const char *s, *n;
+
+	err = fdt_check_header(fdt);
+	if (err != 0)
+		goto fail;
+
+	err = -FDT_ERR_BADOFFSET;
+	if (absoffset >= fdt_totalsize(fdt))
+		goto fail;
+	len = fdt_totalsize(fdt) - absoffset;
+
+	if (fdt_magic(fdt) == FDT_MAGIC) {
+		if (stroffset < 0)
+			goto fail;
+		if (fdt_version(fdt) >= 17) {
+			if (stroffset >= fdt_size_dt_strings(fdt))
+				goto fail;
+			if ((fdt_size_dt_strings(fdt) - stroffset) < len)
+				len = fdt_size_dt_strings(fdt) - stroffset;
+		}
+	} else if (fdt_magic(fdt) == FDT_SW_MAGIC) {
+		if ((stroffset >= 0)
+		    || (stroffset < -fdt_size_dt_strings(fdt)))
+			goto fail;
+		if ((-stroffset) < len)
+			len = -stroffset;
+	} else {
+		err = -FDT_ERR_INTERNAL;
+		goto fail;
+	}
+
+	s = (const char *)fdt + absoffset;
+	n = memchr(s, '\0', len);
+	if (!n) {
+		/* missing terminating NULL */
+		err = -FDT_ERR_TRUNCATED;
+		goto fail;
+	}
+
+	if (lenp)
+		*lenp = n - s;
+	return s;
+
+fail:
+	if (lenp)
+		*lenp = err;
+	return NULL;
+}
+
 const char *fdt_string(const void *fdt, int stroffset)
 {
-	return (const char *)fdt + fdt_off_dt_strings(fdt) + stroffset;
+	return fdt_get_string(fdt, stroffset, NULL);
 }
 
 static int _fdt_string_eq(const void *fdt, int stroffset,
 			  const char *s, int len)
 {
-	const char *p = fdt_string(fdt, stroffset);
+	int slen;
+	const char *p = fdt_get_string(fdt, stroffset, &slen);
 
-	return (strnlen(p, len + 1) == len) && (memcmp(p, s, len) == 0);
+	return p && (slen == len) && (memcmp(p, s, len) == 0);
 }
 
 uint32_t fdt_get_max_phandle(const void *fdt)
-- 
2.17.1