On Tue, Mar 23, 2021 at 01:09:25PM +1300, Simon Glass wrote: > At present it is possible to have two root nodes and even access nodes > in the 'second' root. Such trees should not be considered valid. This > was discovered as part of a security investigation into U-Boot verified > boot. > > Add a check for this to fdt_check_full(). > > Signed-off-by: Simon Glass > Reported-by: Arie Haenel > Reported-by: Julien Lenoir Applied, thanks. > --- > > Changes in v2: > - Use FDT_ERR_BADSTRUCTURE instead of FDT_ERR_BADLAYOUT > > libfdt/fdt_check.c | 7 +++++++ > tests/Makefile.tests | 4 +++- > tests/dumptrees.c | 1 + > tests/run_tests.sh | 2 +- > tests/testdata.h | 1 + > tests/trees.S | 19 +++++++++++++++++++ > 6 files changed, 32 insertions(+), 2 deletions(-) > > diff --git a/libfdt/fdt_check.c b/libfdt/fdt_check.c > index 9ddfdbf..13595a2 100644 > --- a/libfdt/fdt_check.c > +++ b/libfdt/fdt_check.c > @@ -19,6 +19,7 @@ int fdt_check_full(const void *fdt, size_t bufsize) > unsigned int depth = 0; > const void *prop; > const char *propname; > + bool expect_end = false; > > if (bufsize < FDT_V1_SIZE) > return -FDT_ERR_TRUNCATED; > @@ -41,6 +42,10 @@ int fdt_check_full(const void *fdt, size_t bufsize) > if (nextoffset < 0) > return nextoffset; > > + /* If we see two root nodes, something is wrong */ > + if (expect_end && tag != FDT_END) > + return -FDT_ERR_BADSTRUCTURE; > + > switch (tag) { > case FDT_NOP: > break; > @@ -60,6 +65,8 @@ int fdt_check_full(const void *fdt, size_t bufsize) > if (depth == 0) > return -FDT_ERR_BADSTRUCTURE; > depth--; > + if (depth == 0) > + expect_end = true; > break; > > case FDT_PROP: > diff --git a/tests/Makefile.tests b/tests/Makefile.tests > index cb66c9f..fe5cae8 100644 > --- a/tests/Makefile.tests > +++ b/tests/Makefile.tests > @@ -32,7 +32,9 @@ LIB_TESTS_L = get_mem_rsv \ > fs_tree1 > LIB_TESTS = $(LIB_TESTS_L:%=$(TESTS_PREFIX)%) > > -LIBTREE_TESTS_L = truncated_property truncated_string truncated_memrsv > +LIBTREE_TESTS_L = truncated_property truncated_string truncated_memrsv \ > + two_roots > + > LIBTREE_TESTS = $(LIBTREE_TESTS_L:%=$(TESTS_PREFIX)%) > > DL_LIB_TESTS_L = asm_tree_dump value-labels > diff --git a/tests/dumptrees.c b/tests/dumptrees.c > index aecb326..02ca092 100644 > --- a/tests/dumptrees.c > +++ b/tests/dumptrees.c > @@ -24,6 +24,7 @@ static struct { > TREE(ovf_size_strings), > TREE(truncated_property), TREE(truncated_string), > TREE(truncated_memrsv), > + TREE(two_roots) > }; > > #define NUM_TREES (sizeof(trees) / sizeof(trees[0])) > diff --git a/tests/run_tests.sh b/tests/run_tests.sh > index 4b8dada..82543fc 100755 > --- a/tests/run_tests.sh > +++ b/tests/run_tests.sh > @@ -518,7 +518,7 @@ libfdt_tests () { > run_test check_full $good > done > for bad in truncated_property.dtb truncated_string.dtb \ > - truncated_memrsv.dtb; do > + truncated_memrsv.dtb two_roots.dtb; do > run_test check_full -n $bad > done > } > diff --git a/tests/testdata.h b/tests/testdata.h > index 0d08efb..d03f352 100644 > --- a/tests/testdata.h > +++ b/tests/testdata.h > @@ -55,4 +55,5 @@ extern struct fdt_header bad_prop_char; > extern struct fdt_header ovf_size_strings; > extern struct fdt_header truncated_string; > extern struct fdt_header truncated_memrsv; > +extern struct fdt_header two_roots; > #endif /* ! __ASSEMBLY */ > diff --git a/tests/trees.S b/tests/trees.S > index efab287..e2380b7 100644 > --- a/tests/trees.S > +++ b/tests/trees.S > @@ -279,3 +279,22 @@ truncated_memrsv_rsvmap: > truncated_memrsv_rsvmap_end: > > truncated_memrsv_end: > + > + > + /* two root nodes */ > + TREE_HDR(two_roots) > + EMPTY_RSVMAP(two_roots) > + > +two_roots_struct: > + BEGIN_NODE("") > + END_NODE > + BEGIN_NODE("") > + END_NODE > + FDTLONG(FDT_END) > +two_roots_struct_end: > + > +two_roots_strings: > +two_roots_strings_end: > + > +two_roots_end: > + -- 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