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. -- Alexey -- To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
