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

Fri Apr 13 16:53:19 UTC 2018

On Thu, Apr 12, 2018 at 02:39:19PM +1000, David Gibson 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".
> > >
> > > IMO, thinking of it in terms of the "increase" is the wrong way
> > > arond.  If size is really a problem for you, you want to consider how
> > > you can reduce it in any way, not just rolling back the most recent
> > > changes.  The most obvious one to me would be to try
> > > -ffunction-sections to exclude any functions that aren't actually used
> > > by u-boot (if this is helpful and the compiler's an issue, I'd be
> > > willing to consider splitting up libfdt into a bunch more C files).
> > 
> > Actually U-Boot does use that option. Believe me, a lot of work has
> > gone into making this small. There is constant pressure to
> > reduce/retain the size in SPL so that we can stay below limits. E.g.
> > firefly-rk3288 has a 30KB limit for SPL. Current problems are the
> > 64-bit Allwinner parts which are right up against the limit at
> > present.
> > 
> > Also, Masahiro recently did some work to make U-Boot's version of
> > libfdt the same as is used by Linux, so any changes will impact us
> > quite quickly.
> 
> Hm, ok, point taken.
> 
> I did some quick hacks and I think it wouldn't be too hard to add a
> "-DUNSAFE" or similar option that would turn off most of the checking
> and save a substantial amount of code.
> 
> I don't really have time to polish this up myself, but I'd be happy to
> merge patches that add something like this.  I am disinclined to hold
> up this safety work for it, though.
> 
> If someone tackles this, I'd suggest 4 levels of "unsafety":
> 
> 1) Safe.  The default, as now, full checking and safety wherever possible
> 
> 2) Remove "assert"s.  Remove all checks that result in
>    -FDT_ERR_INTERNAL.  These are basically supposed to be assert()s,
>    but I don't want to rely on assert() as an external dependency.
>    Testsuite should still pass in full with this change
> 
> 3) Remove safety against a corrupted fdt.  This would remove basically
>    all the extra checking in this series, plus what was already
>    there.  fdt_offset_ptr() would become a no-op.  A handful of tests
>    that explicitly check against broken trees would need to be skipped
>    in this mode.
> 
> 4) Remove safety against bad parameters.  All of the above and also
>    remove sanity checks of arguments.  A rather larger number of tests
>    would need to be skipped here.

I'm honestly a little bit torn on this.  I guess the fundamental
question is, what can the bootloader do if the DTB is somehow wrong?  I
kind of feel like it's most important to be able to detect problems
within the tree and have a catchable error rather than assume the input
is good, be incorrect about that and go off in the weeds and possibly
hang.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180413/7730ab3f/attachment.sig>