On Thu, Jun 07, 2018 at 04:21:48PM +1000, Alexey Kardashevskiy wrote: > On 4/6/18 11:13 pm, David Gibson wrote: > > 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. > > > Are you pushing the updated dtc any time soon then? I just did. > > > > > > 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
