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@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx> > > > >> > > > --- > > > >> > > > 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:
signature.asc
Description: PGP signature
