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? > > 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. > -- Alexey
Attachment:
signature.asc
Description: OpenPGP digital signature
