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>

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;

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));

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);
 

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

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)%)
 

tests/meson.build

@@ -96,6 +96,7 @@ tests += [
   'truncated_memrsv',
   'truncated_property',
   'truncated_string',
+  'unterminated_memrsv',
 ]
 
 test_deps = [testutil_dep, util_dep, libfdt_dep]

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"

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__ */

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

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();
 }

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();
+}