[U-Boot] [PATCH 03/12] libfdt: Safer access to strings section
Masahiro Yamada
yamada.masahiro at socionext.com
Tue Apr 17 03:03:19 UTC 2018
2018-04-14 23:42 GMT+09:00 Warner Losh <imp at bsdimp.com>:
> 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.
>
>> 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....
>
> Warner
>
> [[ sorry if you see this twice, the mailing list didn't like the other
> mailer I used to reply, hopefully this one is better ]]
> _______________________________________________
You can choose to not use DT on such constrained environments.
According to David's analysis, this is just a matter of 276 byte.
>> > 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%.
>
> 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?
If 276 byte increase is significant, it is a sign that
using DT is a bad idea.
Even if you compile out the safety code by ifdef's,
you will hit the size limit sooner or later by other factors.
U-Boot spins around the loop of [1] and [2].
[1] Support luxury infrastructure (driver mode, device tree, etc.)
[2] Invent workarounds to cut down less important code
to solve image size problem (fdtgrep, dtoc, TPL, etc.)
I am fine with [1] in U-Boot full,
and in SPL as long as the SoC has large enough SRAM.
However, Rockchip is not the case.
Rockchip boards should implement SPL in ad-hoc way
without device tree.
U-Boot is patching around to save Rockchip boards,
which I do not like.
--
Best Regards
Masahiro Yamada
More information about the U-Boot
mailing list