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@gmail.com Cc: David Gibson david@gibson.dropbear.id.au Signed-off-by: Teddy Reed teddy.reed@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)