Re: [PATCH 03/12] libfdt: Safer access to strings section

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6228 bytes --]

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@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@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@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@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).

-- 
Tom

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot