libfdt: Fix bugs with unchecked usage of fdt_num_mem_rsv() fdt_num_mem_rsv() can return an error if the memory reservation block is not properly terminated with a (0, 0) entry. However several other places in libfdt called it without checking for error returns, and could therefore return strange results, or in the case of fdt_open_into() crash. Fix this by always checking the return value. Add some addition tests to catch this bug. Reported-by: Moshe Strauss <moshestrauss10@gmail.com> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c index 63494fb..11f2e2e 100644 --- a/libfdt/fdt_ro.c +++ b/libfdt/fdt_ro.c
@@ -191,6 +191,8 @@ int i; const struct fdt_reserve_entry *re; + FDT_RO_PROBE(fdt); + for (i = 0; (re = fdt_mem_rsv(fdt, i)) != NULL; i++) { if (fdt64_ld_(&re->size) == 0) return i;
diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c index 90ea14e..bea45ed 100644 --- a/libfdt/fdt_rw.c +++ b/libfdt/fdt_rw.c
@@ -166,7 +166,11 @@ FDT_RW_PROBE(fdt); - re = fdt_mem_rsv_w_(fdt, fdt_num_mem_rsv(fdt)); + err = fdt_num_mem_rsv(fdt); + if (err < 0) + return err; + + re = fdt_mem_rsv_w_(fdt, err); err = fdt_splice_mem_rsv_(fdt, re, 0, 1); if (err) return err; @@ -179,10 +183,15 @@ int fdt_del_mem_rsv(void *fdt, int n) { struct fdt_reserve_entry *re = fdt_mem_rsv_w_(fdt, n); + int num; FDT_RW_PROBE(fdt); - if (n >= fdt_num_mem_rsv(fdt)) + num = fdt_num_mem_rsv(fdt); + if (num < 0) + return num; + + if (n >= num) return -FDT_ERR_NOTFOUND; return fdt_splice_mem_rsv_(fdt, re, 1, 0); @@ -439,8 +448,10 @@ FDT_RO_PROBE(fdt); - mem_rsv_size = (fdt_num_mem_rsv(fdt)+1) - * sizeof(struct fdt_reserve_entry); + err = fdt_num_mem_rsv(fdt); + if (err < 0) + return err; + mem_rsv_size = (err + 1) * sizeof(struct fdt_reserve_entry); if (can_assume(LATEST) || fdt_version(fdt) >= 17) { struct_size = fdt_size_dt_struct(fdt); @@ -498,12 +509,14 @@ int fdt_pack(void *fdt) { - int mem_rsv_size; + int err, mem_rsv_size; FDT_RW_PROBE(fdt); - mem_rsv_size = (fdt_num_mem_rsv(fdt)+1) - * sizeof(struct fdt_reserve_entry); + err = fdt_num_mem_rsv(fdt); + if (err < 0) + return err; + mem_rsv_size = (err+1) * sizeof(struct fdt_reserve_entry); fdt_packblocks_(fdt, fdt, mem_rsv_size, fdt_size_dt_struct(fdt), fdt_size_dt_strings(fdt)); fdt_set_totalsize(fdt, fdt_data_size_(fdt));
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h index fd7637c..8c7770e 100644 --- a/libfdt/libfdt.h +++ b/libfdt/libfdt.h
@@ -475,7 +475,12 @@ * or any other (0,0) entries reserved for expansion. * * returns: - * the number of entries + * the number of entries, on success + * -FDT_ERR_ALIGNMENT, + * -FDT_ERR_BADMAGIC, + * -FDT_ERR_BADVERSION, + * -FDT_ERR_BADSTATE, + * -FDT_ERR_TRUNCATED, standard meanings */ int fdt_num_mem_rsv(const void *fdt);
diff --git a/tests/.gitignore b/tests/.gitignore index 3376ed9..a1f53dd 100644 --- a/tests/.gitignore +++ b/tests/.gitignore
@@ -72,6 +72,7 @@ /truncated_property /truncated_string /truncated_memrsv +/unterminated_memrsv /utilfdt_test /value-labels /get_next_tag_invalid_prop_len
diff --git a/tests/Makefile.tests b/tests/Makefile.tests index 05bb3b2..94b3cf9 100644 --- a/tests/Makefile.tests +++ b/tests/Makefile.tests
@@ -32,7 +32,8 @@ 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 unterminated_memrsv LIBTREE_TESTS = $(LIBTREE_TESTS_L:%=$(TESTS_PREFIX)%)
diff --git a/tests/meson.build b/tests/meson.build index 37bfd47..4e669ce 100644 --- a/tests/meson.build +++ b/tests/meson.build
@@ -96,6 +96,7 @@ 'truncated_memrsv', 'truncated_property', 'truncated_string', + 'unterminated_memrsv', ] test_deps = [testutil_dep, util_dep, libfdt_dep]
diff --git a/tests/run_tests.sh b/tests/run_tests.sh index f07092b..34110c1 100755 --- a/tests/run_tests.sh +++ b/tests/run_tests.sh
@@ -495,6 +495,7 @@ run_test truncated_property run_test truncated_string run_test truncated_memrsv + run_test unterminated_memrsv # Check aliases support in fdt_path_offset run_dtc_test -I dts -O dtb -o aliases.dtb "$SRCDIR/aliases.dts"
diff --git a/tests/testdata.h b/tests/testdata.h index fcebc2c..b29a759 100644 --- a/tests/testdata.h +++ b/tests/testdata.h
@@ -55,6 +55,7 @@ extern struct fdt_header ovf_size_strings; extern struct fdt_header truncated_string; extern struct fdt_header truncated_memrsv; +extern struct fdt_header unterminated_memrsv; extern struct fdt_header two_roots; extern struct fdt_header named_root; #endif /* ! __ASSEMBLER__ */
diff --git a/tests/trees.S b/tests/trees.S index d69f7f1..3de95fa 100644 --- a/tests/trees.S +++ b/tests/trees.S
@@ -291,6 +291,23 @@ truncated_memrsv_end: + /* unterminated_memrsv */ + treehdr unterminated_memrsv + +unterminated_memrsv_rsvmap: + rsvmape TEST_ADDR_1H, TEST_ADDR_1L, TEST_SIZE_1H, TEST_SIZE_1L +unterminated_memrsv_rsvmap_end: + +unterminated_memrsv_struct: + beginn "" + endn + fdtlong FDT_END +unterminated_memrsv_struct_end: + +unterminated_memrsv_strings: +unterminated_memrsv_strings_end: + +unterminated_memrsv_end: /* two root nodes */ treehdr two_roots
diff --git a/tests/truncated_memrsv.c b/tests/truncated_memrsv.c index d78036c..b7ab4e7 100644 --- a/tests/truncated_memrsv.c +++ b/tests/truncated_memrsv.c
@@ -15,9 +15,12 @@ #include "tests.h" #include "testdata.h" +#define SPACE 65536 + int main(int argc, char *argv[]) { void *fdt = &truncated_memrsv; + void *buf; int err; uint64_t addr, size; @@ -46,5 +49,11 @@ FAIL("fdt_get_mem_rsv(1) returned %d instead of -FDT_ERR_BADOFFSET", err); + buf = xmalloc(SPACE); + err = fdt_open_into(fdt, buf, SPACE); + if (err != -FDT_ERR_TRUNCATED) + FAIL("fdt_open_into() returned %d instead of -FDT_ERR_TRUNCATED", + err); + PASS(); }
diff --git a/tests/unterminated_memrsv.c b/tests/unterminated_memrsv.c new file mode 100644 index 0000000..441b8d7 --- /dev/null +++ b/tests/unterminated_memrsv.c
@@ -0,0 +1,67 @@ +// SPDX-License-Identifier: LGPL-2.1-or-later +/* + * libfdt - Flat Device Tree manipulation + * Testcase for misbehaviour on an unterminated memrsv map + * Copyright Red Hat + * + * Based on a proof of concept report from: + * Moshe Strauss <moshestrauss10@gmail.com> + */ + +#include <stdlib.h> +#include <stdio.h> +#include <string.h> +#include <stdint.h> + +#include <libfdt.h> + +#include "tests.h" +#include "testdata.h" + +#define SPACE 65536 + +int main(int argc, char *argv[]) +{ + void *fdt = &unterminated_memrsv; + void *buf; + int err; + uint64_t addr, size; + + test_init(argc, argv); + + err = fdt_check_header(fdt); + if (err != 0) + FAIL("Bad header: %s", fdt_strerror(err)); + + err = fdt_num_mem_rsv(fdt); + if (err != -FDT_ERR_TRUNCATED) + FAIL("fdt_num_mem_rsv() returned %d instead of -FDT_ERR_TRUNCATED", + err); + + err = fdt_get_mem_rsv(fdt, 0, &addr, &size); + if (err != 0) + FAIL("fdt_get_mem_rsv() failed on first entry: %s", + fdt_strerror(err)); + if ((addr != TEST_ADDR_1) || (size != TEST_SIZE_1)) + FAIL("Entry doesn't match: (0x%llx, 0x%llx) != (0x%llx, 0x%llx)", + (unsigned long long)addr, (unsigned long long)size, + TEST_ADDR_1, TEST_SIZE_1); + + err = fdt_add_mem_rsv(fdt, TEST_ADDR_2, TEST_SIZE_2); + if (err != -FDT_ERR_TRUNCATED) + FAIL("fdt_add_mem_rsv() returned %d instead of -FDT_ERR_TRUNCATED", + err); + + err = fdt_del_mem_rsv(fdt, 0); + if (err != -FDT_ERR_TRUNCATED) + FAIL("fdt_del_mem_rsv() returned %d instead of -FDT_ERR_TRUNCATED", + err); + + buf = xmalloc(SPACE); + err = fdt_open_into(fdt, buf, SPACE); + if (err != -FDT_ERR_TRUNCATED) + FAIL("fdt_open_into() returned %d instead of -FDT_ERR_TRUNCATED", + err); + + PASS(); +}