Hi Teddy,
On Fri, 23 Nov 2018 at 19:18, Teddy Reed <teddy.reed@xxxxxxxxx> 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@xxxxxxxxx>
> Cc: David Gibson <david@xxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Teddy Reed <teddy.reed@xxxxxxxxx>
> ---
> 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
