On 26/07/2018 12:07, David Gibson wrote: > 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. Any update here? afaict DTC is still 1.4.6 in the ppc-for-3.1 branch. -- Alexey
