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