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