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

Mon Apr 16 00:37:59 UTC 2018

On Sat, Apr 14, 2018 at 08:42:23AM -0600, Warner Losh wrote:
> On Fri, Apr 13, 2018 at 9:43 PM, David Gibson <david at gibson.dropbear.id.au>
> wrote:
> 
> > 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 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.
> >
> > 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.
> 
> Usually a boot loader can only do binary things: either use it or not use
> it. If it is corrupt, there's usually not enough room for self-healing
> code. There may be room for falling back to a second, backup copy. However,
> any data corruption that would take out the first copy may also take out
> the second, so that's a crap shoot.

Yeah, I wouldn't anticipate being able to do any sort of correction in
a bootloader - or anywhere, hardly.  But a bootloader could print a
message indicating the dtb is faulty.  That could mean the difference
between immediately seeing the problem or spending hours figuring out
why the bootloader corrupted its memory and jumped off into hyperspace
somewhere.

> > 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.
> 
> 
> It doesn't matter how safe it is if the code doesn't fit. Sometimes you
> have to give up safety for functionality in constrained environments. Those
> constrained environments often have to assume the best. The code often
> isn't signed or has no stronger protections than a simple checksum. There
> isn't room for that stuff til the later layers. If your code doesn't fit, a
> libfdtlite will happen and that will likely be even worse because it
> doesn't have the testing coverage libfdt has. Better to have an ifdef to
> turn off the sanity checks than a whole new codebase.
> 
> I base this prediction on working on boot loaders that are constrained:
> code gets forked to deal with the smaller space, and bugs introduced into
> the forked code. It's much better to use a common code base with parts
> #ifdef'd out than to maintain two code bases....

That's basically the same conclusion I came to.  I welcome patches to
add the necessary ifdefs to cut the size down.  I just don't have time
to make and test them myself.

-- 
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/20180416/73af7bb5/attachment.sig>