From 47d7c01ba8a1241e919ab56dd01ba245b38fef8e Mon Sep 17 00:00:00 2001 From: David Gibson Date: Fri, 17 Apr 2026 20:36:42 +1000 Subject: [PATCH] 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 Signed-off-by: David Gibson --- libfdt/fdt_ro.c | 2 ++ libfdt/fdt_rw.c | 27 +++++++++++---- libfdt/libfdt.h | 7 +++- tests/.gitignore | 1 + tests/Makefile.tests | 3 +- tests/meson.build | 1 + tests/run_tests.sh | 1 + tests/testdata.h | 1 + tests/trees.S | 17 ++++++++++ tests/truncated_memrsv.c | 9 +++++ tests/unterminated_memrsv.c | 67 +++++++++++++++++++++++++++++++++++++ 11 files changed, 127 insertions(+), 9 deletions(-) create mode 100644 tests/unterminated_memrsv.c 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 fdt_num_mem_rsv(const void *fdt) 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 @@ int fdt_add_mem_rsv(void *fdt, uint64_t address, uint64_t size) 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_add_mem_rsv(void *fdt, uint64_t address, uint64_t size) 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 @@ int fdt_open_into(const void *fdt, void *buf, int bufsize) 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_open_into(const void *fdt, void *buf, int bufsize) 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 @@ int fdt_generate_phandle(const void *fdt, uint32_t *phandle); * 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 @@ tmp.* /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 @@ 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 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 @@ tests += [ '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 @@ libfdt_tests () { 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 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 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_rsvmap_end: 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 @@ int main(int argc, char *argv[]) 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 + */ + +#include +#include +#include +#include + +#include + +#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(); +}