| Message ID | 20181123211837.274e66d349a55958981207e7@gmail.com |
|---|---|
| State | Deferred |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | [U-Boot] fdt: Fix string property comparison overflow | expand |
Hi Teddy, On Fri, 23 Nov 2018 at 19:18, Teddy Reed <teddy.reed@gmail.com> wrote: > > 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(-) Assuming this is for U-Boot then please cc me, as I don't often look on the mailing list for patches. I am worried about the code-size impact of this patch. It will likely break quite a number of boards in SPL. David already added some checks in libfdt upstream which I have not synced to U-Boot for the same reason. We need some sort of build option to exclude them for when space is tight. I have been thinking of sending some patches upstream for this, but have not got to this yet. Regards, Simon
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)
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(-)