[U-Boot] [PATCH 03/12] libfdt: Safer access to strings section

Fri Apr 13 03:46:26 UTC 2018

On Thu, Apr 12, 2018 at 03:00:17PM -0400, Tom Rini wrote:
> On Thu, Apr 12, 2018 at 02:01:05PM +1000, David Gibson wrote:
> > On Tue, Apr 10, 2018 at 06:36:06PM -0400, Tom Rini wrote:
> > > On Tue, Apr 10, 2018 at 10:42:45AM -0400, Simon Glass wrote:
> > > > +U-Boot, Tom, Masahiro
> > > > 
> > > > Hi David,
> > > > 
> > > > On 10 April 2018 at 01:22, David Gibson <david at gibson.dropbear.id.au> wrote:
> > > > > On Wed, Apr 04, 2018 at 01:21:10AM +0800, Simon Glass wrote:
> > > > >> Hi David,
> > > > >>
> > > > >> On 3 April 2018 at 23:02, David Gibson <david at gibson.dropbear.id.au> wrote:
> > > > >> >
> > > > >> > On Fri, Mar 30, 2018 at 04:42:21PM +0800, Simon Glass wrote:
> > > > >> > > Hi David,
> > > > >> > >
> > > > >> > > On 26 March 2018 at 07:25, David Gibson <david at gibson.dropbear.id.au> wrote:
> > > > >> > > > fdt_string() is used to retrieve strings from a DT blob's strings section.
> > > > >> > > > It's rarely used directly, but is widely used internally.
> > > > >> > > >
> > > > >> > > > However, it doesn't do any bounds checking, which means in the case of a
> > > > >> > > > corrupted blob it could access bad memory, which libfdt is supposed to
> > > > >> > > > avoid.
> > > > >> > > >
> > > > >> > > > This write a safe alternative to fdt_string, fdt_get_string().  It checks
> > > > >> > > > both that the given offset is within the string section and that the string
> > > > >> > > > it points to is properly \0 terminated within the section.  It also returns
> > > > >> > > > the string's length as a convenience (since it needs to determine to do the
> > > > >> > > > checks anyway).
> > > > >> > > >
> > > > >> > > > fdt_string() is rewritten in terms of fdt_get_string() for compatibility.
> > > > >> > > >
> > > > >> > > > Most of the diff here is actually testing infrastructure.
> > > > >> > > >
> > > > >> > > > Signed-off-by: David Gibson <david at gibson.dropbear.id.au>
> > > > >> > > > ---
> > > > >> > > >  libfdt/fdt_ro.c          | 61 +++++++++++++++++++++++++++++++++++--
> > > > >> > > >  libfdt/libfdt.h          | 18 ++++++++++-
> > > > >> > > >  libfdt/version.lds       |  2 +-
> > > > >> > > >  tests/.gitignore         |  1 +
> > > > >> > > >  tests/Makefile.tests     |  2 +-
> > > > >> > > >  tests/run_tests.sh       |  1 +
> > > > >> > > >  tests/testdata.h         |  1 +
> > > > >> > > >  tests/testutils.c        | 11 +++++--
> > > > >> > > >  tests/trees.S            | 26 ++++++++++++++++
> > > > >> > > >  tests/truncated_string.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >> > > >  10 files changed, 193 insertions(+), 9 deletions(-)
> > > > >> > > >  create mode 100644 tests/truncated_string.c
> > > > >> > >
> > > > >> > > Similar code-size quesiton here. It looks like a lot of checking code.
> > > > >> > > Can we have an option to remove it?
> > > > >> >
> > > > >> > Again, I'm disinclined without a concrete example of a problem.  Fwiw
> > > > >> > the code size change is +276 bytes on my setup.
> > > > >>
> > > > >> That might not sound like a lot, but the overhead of DT in U-Boot is
> > > > >> about 3KB, so this adds nearly 10%.
> > > > >
> > > > > Hm.  And how much is it compared to the whole U-Boot blob?
> > > > >
> > > > >> The specific problem is that when U-Boot SPL gets too big boards don't
> > > > >> boot. Because we take the upstream libfdt this will affect U-Boot.
> > > > >>
> > > > >> Do you have any thoughts on how we could avoid this size increase?
> > > > >
> > > > > So, again, I'm very disinclined to prioritize size over memory safety
> > > > > without a *concrete* example.  i.e. "We hit this specific problem with
> > > > > size on this specific board that we were really using" rather than
> > > > > just "it might be a problem".
> > > 
> > > I'm either failing in my Google-fu or is there not an easy way to grab
> > > the patches from patchwork/similar?  But, if you shoot me the series
> > > off-list, I can tell you how much U-Boot targets grow here (we can use
> > > the same script as the kernel to re-sync sources back in, so I can give
> > > you a before/after).  But as Simon notes, we have a number of platforms
> > > that need to use (parts of) libfdt and stick to ~30KiB or less in total,
> > > sometimes including some memory for stack/etc and we've long been using
> > > -ffunction-sections/etc (the latest round of trying to use LTO has me
> > > thinking maybe we can see if that's a valid option finally, but that's
> > > an aside). Thanks!
> > 
> > We don't have a patchwork for these lists AFAIK, but you can get my
> > draft branch from:
> > 
> > https://github.com/dgibson/dtc/tree/safety
> 
> OK, thanks.  So, I used scripts/dtc/update-dtc-source.sh to re-sync us
> first with current master, and then with the safety branch, and
> boy-howdy, are there a lot of warning changes and additions :)  For the
> record, in U-Boot you can use the buildman tool
> (./tools/buildman/buildman) to build a series for a set of targets and
> get size change for each commit.  I did:
> ./tools/buildman/buildman -o /tmp/libfdt-test -b master -SCdvel \
> 'arc|arm|sandbox|x86|aarch64|powerpc|avr32|m68k|nios2|sh4|mips|xtensa|riscv'
> ./tools/buildman/buildman -o /tmp/libfdt-test -b master  -Ssdel \
> 'arc|arm|sandbox|x86|aarch64|powerpc|avr32|m68k|nios2|sh4|mips|xtensa|riscv'
> 
> And the full results are at:
> https://gist.githubusercontent.com/trini/0f5f4c368de6c3f58d88cd55359e1468/raw/5f3a96de5c9ec3c7724ec182f9c513f255084a89/libfdt-size-change.txt
> 
> Commit #1 that it built is the baseline results, commit #2 is current
> dtc master and commit #3 is with the safety branch.  The good news is
> that nothing here is fatal (for targets that avail themselves of the
> CONFIG knobs to fail if binary size exceeds a certain target).  Picking
> the BSC9132QDS_NOR_DDRCLK100_SECURE target and going for more detail on
> the growth we have:
> https://gist.github.com/trini/2c2ef7e839889279ac22fabf05360e4c#file-bsc9132qds_nor_ddrclk100_secure-txt-L7
> 
> That particular paste also shows that gcc-6.3 does a better job than
> gcc-5.4 (growth is 400 with 5.4 and 384 with gcc-6.3).
> 
> Building for km_kirkwood_pci shows similar (but larger growth) results
> and are at https://gist.github.com/182af8446274dbda76553cbf924e2358
> 
> In sum, this isn't a deal-breaker yet, but is a real concern moving
> forward (and I will follow up to your other reply too).

Ok.  So with that I'm definitely inclined to go ahead and merge these
changes, but sounds like adding some "unsafety" options to reduce size
sooner rather than later would be a good idea.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180413/28c47315/attachment.sig>