On Sun, May 27, 2018 at 08:24:53PM -0600, Simon Glass wrote: > Hi David, > > On 12 April 2018 at 16:23, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote: > > > > On Thu, Apr 12, 2018 at 11:28:49AM -0600, Simon Glass wrote: > > > Hi David, > > > > > > On 11 April 2018 at 22:52, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote: > > > > It's always been a design goal of libfdt that it should be safe to > > > > use, even on a badly corrupted fdt image. "Safe" here meaning that it > > > > should never access memory outside the blob's stated boundaries. > > > > > > > > The existing code is pretty good about this with regards to accesses > > > > to the structure block, thanks to the fdt_offset_ptr() helper > > > > function. However, accesses to the strings and memory reservation > > > > blocks were less careful and could easily lead to wild pointer > > > > dereferences in the case of a bad blob. > > > > > > > > This series makes a number of improvements to libfdt safety, capping > > > > it off with a new fdt_check_full() function which acts like an "fsck" > > > > for fdt blobs. Along the way we make some other cleanups. > > > > > > > > Changes since v1: > > > > * Assorted minor polishes based on review comments > > > > * Added an extra patch to clean up sequential write mode state > > > > checking > > > > > > > > David Gibson (13): > > > > libfdt: Clean up header checking functions > > > > libfdt: Improve sequential write state checking > > > > libfdt: Make fdt_check_header() more thorough > > > > libfdt: Safer access to strings section > > > > libfdt: Propagate name errors in fdt_getprop_by_offset() > > > > libfdt: Safer access to memory reservations > > > > Consolidate utilfdt_read_len() variants > > > > libfdt: Add fdt_header_size() > > > > Use size_t for blob lengths in utilfdt_read* > > > > tests: Remove unused #define > > > > tests: Better handling of valgrind errors saving blobs > > > > tests: Use valgrind client requests for better checking > > > > libfdt: Add fdt_check_full() function > > > > > > > > fdtdump.c | 4 +- > > > > fdtget.c | 2 +- > > > > fdtoverlay.c | 6 +- > > > > fdtput.c | 2 +- > > > > libfdt/fdt.c | 76 +++++++++++++++++- > > > > libfdt/fdt_overlay.c | 6 +- > > > > libfdt/fdt_ro.c | 189 ++++++++++++++++++++++++++++++++++++++++----- > > > > libfdt/fdt_rw.c | 28 +++---- > > > > libfdt/fdt_sw.c | 97 ++++++++++++++++++----- > > > > libfdt/libfdt.h | 35 ++++++++- > > > > libfdt/libfdt_env.h | 1 + > > > > libfdt/libfdt_internal.h | 5 +- > > > > libfdt/version.lds | 2 +- > > > > tests/.gitignore | 5 ++ > > > > tests/Makefile.tests | 6 +- > > > > tests/check_full.c | 63 +++++++++++++++ > > > > tests/check_header.c | 128 ++++++++++++++++++++++++++++++ > > > > tests/dumptrees.c | 2 + > > > > tests/mangle-layout.c | 8 +- > > > > tests/mangle-layout.supp | 7 -- > > > > tests/open_pack.supp | 7 -- > > > > tests/run_tests.sh | 14 ++++ > > > > tests/sw_states.c | 140 +++++++++++++++++++++++++++++++++ > > > > tests/sw_tree1.supp | 18 ----- > > > > tests/testdata.h | 2 + > > > > tests/tests.h | 1 + > > > > tests/testutils.c | 78 +++++++++++++++++-- > > > > tests/trees.S | 46 +++++++++++ > > > > tests/truncated_memrsv.c | 63 +++++++++++++++ > > > > tests/truncated_property.c | 2 + > > > > tests/truncated_string.c | 81 +++++++++++++++++++ > > > > util.c | 23 ++---- > > > > util.h | 20 +---- > > > > 33 files changed, 1016 insertions(+), 151 deletions(-) > > > > create mode 100644 tests/check_full.c > > > > create mode 100644 tests/check_header.c > > > > delete mode 100644 tests/mangle-layout.supp > > > > delete mode 100644 tests/open_pack.supp > > > > create mode 100644 tests/sw_states.c > > > > delete mode 100644 tests/sw_tree1.supp > > > > create mode 100644 tests/truncated_memrsv.c > > > > create mode 100644 tests/truncated_string.c > > > > > > Is there a patchwork client I can use to try these patches please, or > > > a git tree somewhere? For some reason I cannot find it. > > > > There's no patchwork, but you can get the git tree at > > https://github.com/dgibson/dtc/tree/safety > > OK thank you. > > It is probably best that you apply this and I will see if I can take a > look at cutting the size back afterwards. Ok, will do. > How about something like: > > static inline fdt_do_checks(void) > { > #ifdef FDT_MINIMAL > return false; > #else > return true; > #endif > } > > Then, in various places: > > if (fdt_do_checks() && check_something()) > return -FDT_ERR_... > > I suppose we could add more options, but the main one from my side is > to remove all sanity / safety checks for minimal code size. Seems reasonable. Given that we already have just a few helpers for the checking, it might be simpler to just inline the #ifdef in that handful of functions, rather than making another indirection. But it probably doesn't matter much either way. -- 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
