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

Re: [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@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.

Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Tom Rini]

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

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

I'm either failing in my Google-fu or is there not an easy way to grab
the patches from patchwork/similar?  But, if you shoot me the series
off-list, I can tell you how much U-Boot targets grow here (we can use
the same script as the kernel to re-sync sources back in, so I can give
you a before/after).  But as Simon notes, we have a number of platforms
that need to use (parts of) libfdt and stick to ~30KiB or less in total,
sometimes including some memory for stack/etc and we've long been using
-ffunction-sections/etc (the latest round of trying to use LTO has me
thinking maybe we can see if that's a valid option finally, but that's
an aside). Thanks!

-- 
Tom

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

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

[David Gibson]

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

On Tue, Apr 10, 2018 at 06:36:06PM -0400, Tom Rini 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".
> 
> I'm either failing in my Google-fu or is there not an easy way to grab
> the patches from patchwork/similar?  But, if you shoot me the series
> off-list, I can tell you how much U-Boot targets grow here (we can use
> the same script as the kernel to re-sync sources back in, so I can give
> you a before/after).  But as Simon notes, we have a number of platforms
> that need to use (parts of) libfdt and stick to ~30KiB or less in total,
> sometimes including some memory for stack/etc and we've long been using
> -ffunction-sections/etc (the latest round of trying to use LTO has me
> thinking maybe we can see if that's a valid option finally, but that's
> an aside). Thanks!

We don't have a patchwork for these lists AFAIK, but you can get my
draft branch from:

https://github.com/dgibson/dtc/tree/safety

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

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

[Tom Rini]

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

On Thu, Apr 12, 2018 at 02:01:05PM +1000, David Gibson wrote:
> On Tue, Apr 10, 2018 at 06:36:06PM -0400, Tom Rini 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".
> > 
> > I'm either failing in my Google-fu or is there not an easy way to grab
> > the patches from patchwork/similar?  But, if you shoot me the series
> > off-list, I can tell you how much U-Boot targets grow here (we can use
> > the same script as the kernel to re-sync sources back in, so I can give
> > you a before/after).  But as Simon notes, we have a number of platforms
> > that need to use (parts of) libfdt and stick to ~30KiB or less in total,
> > sometimes including some memory for stack/etc and we've long been using
> > -ffunction-sections/etc (the latest round of trying to use LTO has me
> > thinking maybe we can see if that's a valid option finally, but that's
> > an aside). Thanks!
> 
> We don't have a patchwork for these lists AFAIK, but you can get my
> draft branch from:
> 
> https://github.com/dgibson/dtc/tree/safety

OK, thanks.  So, I used scripts/dtc/update-dtc-source.sh to re-sync us
first with current master, and then with the safety branch, and
boy-howdy, are there a lot of warning changes and additions :)  For the
record, in U-Boot you can use the buildman tool
(./tools/buildman/buildman) to build a series for a set of targets and
get size change for each commit.  I did:
./tools/buildman/buildman -o /tmp/libfdt-test -b master -SCdvel \
'arc|arm|sandbox|x86|aarch64|powerpc|avr32|m68k|nios2|sh4|mips|xtensa|riscv'
./tools/buildman/buildman -o /tmp/libfdt-test -b master  -Ssdel \
'arc|arm|sandbox|x86|aarch64|powerpc|avr32|m68k|nios2|sh4|mips|xtensa|riscv'

And the full results are at:
https://gist.githubusercontent.com/trini/0f5f4c368de6c3f58d88cd55359e1468/raw/5f3a96de5c9ec3c7724ec182f9c513f255084a89/libfdt-size-change.txt

Commit #1 that it built is the baseline results, commit #2 is current
dtc master and commit #3 is with the safety branch.  The good news is
that nothing here is fatal (for targets that avail themselves of the
CONFIG knobs to fail if binary size exceeds a certain target).  Picking
the BSC9132QDS_NOR_DDRCLK100_SECURE target and going for more detail on
the growth we have:
https://gist.github.com/trini/2c2ef7e839889279ac22fabf05360e4c#file-bsc9132qds_nor_ddrclk100_secure-txt-L7

That particular paste also shows that gcc-6.3 does a better job than
gcc-5.4 (growth is 400 with 5.4 and 384 with gcc-6.3).

Building for km_kirkwood_pci shows similar (but larger growth) results
and are at https://gist.github.com/182af8446274dbda76553cbf924e2358

In sum, this isn't a deal-breaker yet, but is a real concern moving
forward (and I will follow up to your other reply too).

-- 
Tom

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

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

[David Gibson]

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

On Thu, Apr 12, 2018 at 03:00:17PM -0400, Tom Rini wrote:
> On Thu, Apr 12, 2018 at 02:01:05PM +1000, David Gibson wrote:
> > On Tue, Apr 10, 2018 at 06:36:06PM -0400, Tom Rini 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".
> > > 
> > > I'm either failing in my Google-fu or is there not an easy way to grab
> > > the patches from patchwork/similar?  But, if you shoot me the series
> > > off-list, I can tell you how much U-Boot targets grow here (we can use
> > > the same script as the kernel to re-sync sources back in, so I can give
> > > you a before/after).  But as Simon notes, we have a number of platforms
> > > that need to use (parts of) libfdt and stick to ~30KiB or less in total,
> > > sometimes including some memory for stack/etc and we've long been using
> > > -ffunction-sections/etc (the latest round of trying to use LTO has me
> > > thinking maybe we can see if that's a valid option finally, but that's
> > > an aside). Thanks!
> > 
> > We don't have a patchwork for these lists AFAIK, but you can get my
> > draft branch from:
> > 
> > https://github.com/dgibson/dtc/tree/safety
> 
> OK, thanks.  So, I used scripts/dtc/update-dtc-source.sh to re-sync us
> first with current master, and then with the safety branch, and
> boy-howdy, are there a lot of warning changes and additions :)  For the
> record, in U-Boot you can use the buildman tool
> (./tools/buildman/buildman) to build a series for a set of targets and
> get size change for each commit.  I did:
> ./tools/buildman/buildman -o /tmp/libfdt-test -b master -SCdvel \
> 'arc|arm|sandbox|x86|aarch64|powerpc|avr32|m68k|nios2|sh4|mips|xtensa|riscv'
> ./tools/buildman/buildman -o /tmp/libfdt-test -b master  -Ssdel \
> 'arc|arm|sandbox|x86|aarch64|powerpc|avr32|m68k|nios2|sh4|mips|xtensa|riscv'
> 
> And the full results are at:
> https://gist.githubusercontent.com/trini/0f5f4c368de6c3f58d88cd55359e1468/raw/5f3a96de5c9ec3c7724ec182f9c513f255084a89/libfdt-size-change.txt
> 
> Commit #1 that it built is the baseline results, commit #2 is current
> dtc master and commit #3 is with the safety branch.  The good news is
> that nothing here is fatal (for targets that avail themselves of the
> CONFIG knobs to fail if binary size exceeds a certain target).  Picking
> the BSC9132QDS_NOR_DDRCLK100_SECURE target and going for more detail on
> the growth we have:
> https://gist.github.com/trini/2c2ef7e839889279ac22fabf05360e4c#file-bsc9132qds_nor_ddrclk100_secure-txt-L7
> 
> That particular paste also shows that gcc-6.3 does a better job than
> gcc-5.4 (growth is 400 with 5.4 and 384 with gcc-6.3).
> 
> Building for km_kirkwood_pci shows similar (but larger growth) results
> and are at https://gist.github.com/182af8446274dbda76553cbf924e2358
> 
> In sum, this isn't a deal-breaker yet, but is a real concern moving
> forward (and I will follow up to your other reply too).

Ok.  So with that I'm definitely inclined to go ahead and merge these
changes, but sounds like adding some "unsafety" options to reduce size
sooner rather than later would be a good idea.

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

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

[David Gibson]

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

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.

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

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

[Tom Rini]

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

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

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

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

[David Gibson]

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

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

[Warner Losh]

On Sat, Apr 14, 2018, 12:55 AM David Gibson <david@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@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.
>

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'thave the testing coverage libfdt has. Better to have an ifdef to
turn off the sanity checks than a whole new codebase.

Warner

-- 
> 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
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Warner Losh]

On Fri, Apr 13, 2018 at 9:43 PM, David Gibson <david@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@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.


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 ]]
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[David Gibson]

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

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

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

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

[Masahiro Yamada]

2018-04-14 23:42 GMT+09:00 Warner Losh <imp@bsdimp.com>:
> On Fri, Apr 13, 2018 at 9:43 PM, David Gibson <david@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@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.
>
>
> 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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[David Gibson]

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

On Tue, Apr 17, 2018 at 12:03:19PM +0900, Masahiro Yamada wrote:
> 2018-04-14 23:42 GMT+09:00 Warner Losh <imp@bsdimp.com>:
> > On Fri, Apr 13, 2018 at 9:43 PM, David Gibson <david@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@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.
> >
> >
> > 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.

276 bytes was just the increase for one specific patch.  The whole
series taken together, plus the existing checking code could be rather
larger.  It wouldn't be orders of magnitude larger though, probably
only in the vicinity of 1k.

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