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

[Simon Glass]

Hi Teddy,

On Fri, 23 Nov 2018 at 19:18, Teddy Reed <teddy.reed at 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 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(-)

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