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

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

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

2018-04-16 Thread Masahiro Yamada 
2018-04-14 23:42 GMT+09:00 Warner Losh :
> On Fri, Apr 13, 2018 at 9:43 PM, David Gibson 
> 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 
>> 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 <
>> da...@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 <
>> da...@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 
>> > > > >> > > > ---
>> > > > >> > > >  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

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

2018-04-15 Thread David Gibson 
On Sat, Apr 14, 2018 at 08:42:23AM -0600, Warner Losh wrote:
> On Fri, Apr 13, 2018 at 9:43 PM, David Gibson 
> 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 
> > 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 <
> > da...@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 <
> > da...@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 
> > > > > >> > > > ---
> > > > > >> > > >  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

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

2018-04-14 Thread Warner Losh 
On Fri, Apr 13, 2018 at 9:43 PM, David Gibson 
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 
> 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 <
> da...@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 <
> da...@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 
> > > > >> > > > ---
> > > > >> > > >  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

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

2018-04-14 Thread Warner Losh 
On Sat, Apr 14, 2018, 12:55 AM David Gibson 
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 
> 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 <
> da...@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 <
> da...@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 
> > > > >> > > > ---
> > > > >> > > >  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

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

2018-04-14 Thread David Gibson 
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  
> > > 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  
> > > >> 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 
> > > >> > >  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 
> > > >> > > > ---
> > > >> > > >  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

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

2018-04-13 Thread Tom Rini 
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  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  
> > >> 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 
> > >> > >  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 
> > >> > > > ---
> > >> > > >  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,

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

2018-04-12 Thread David Gibson 
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  
> > > > 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  
> > > > >> 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 
> > > > >> > >  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 
> > > > >> > > > ---
> > > > >> > > >  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

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

2018-04-12 Thread Tom Rini 
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  
> > > 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  
> > > >> 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 
> > > >> > >  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 
> > > >> > > > ---
> > > >> > > >  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:

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

2018-04-11 Thread David Gibson 
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  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  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  
> >> > > 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 
> >> > > > ---
> >> > > >  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.

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

2018-04-11 Thread David Gibson 
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  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  
> > >> 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 
> > >> > >  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 
> > >> > > > ---
> > >> > > >  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


signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

2018-04-10 Thread Tom Rini 
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  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  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  
> >> > > 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 
> >> > > > ---
> >> > > >  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


signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

2018-04-10 Thread Simon Glass 
+U-Boot, Tom, Masahiro

Hi David,

On 10 April 2018 at 01:22, David Gibson  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  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  
>> > > 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 
>> > > > ---
>> > > >  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