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

[David Gibson <david@gibson.dropbear.id.au>]

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

On Fri, Apr 13, 2018 at 12:53:19PM -0400, Tom Rini wrote:
> 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@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".
> > > >
> > > > 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.

Torn on what aspect, exactly?

> I guess the fundamental
> question is, what can the bootloader do if the DTB is somehow wrong?

Depends a lot on the details of the bootloader.

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

I absolutely agree, which is why I want safety in the face of a
corrupted tree to be the default behaviour.  But people are telling me
that size is vitally important, and there's not a whole lot that could
be cut other than the checking/safety code.

-- 
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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 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