On Tue, Jun 12, 2018 at 12:55:08PM +1000, Alexey Kardashevskiy wrote: > On 12/6/18 12:21 pm, David Gibson wrote: > > On Fri, Jun 08, 2018 at 04:10:04PM +1000, Alexey Kardashevskiy wrote: > >> On 7/6/18 7:37 pm, David Gibson wrote: > >>> 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. > >> > >> > >> I was rather asking about dtc submodule update in qemu, it is not there yet: > >> > >> [vpl1 qemu]$ git submodule status dtc > >> e54388015af1fb4bf04d0bca99caba1074d9cc42 dtc (v1.4.6) > > > > Oh, right. Any particular things you're looking for in it? > > > The fdt_check_full() function to check the FDT I build in SLOF and pass > back to QEMU for PHB hotplug which wants to know the actual XICS phandle value. I've now made a dtc/libfdt release including that function. Once 3.1 opens we can pull it into qemu. -- 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
