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

[Simon Glass]

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

Regards,
Simon