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

@@ -225,7 +225,7 @@ int fdt_next_node(const void *fdt, int offset, int *depth);
  * @fdt:	FDT blob
  * @offset:	Offset of node to check
  *
- * Return: offset of first subnode, or -FDT_ERR_NOTFOUND if there is none
+ * returns: offset of first subnode, or -FDT_ERR_NOTFOUND if there is none
  */
 int fdt_first_subnode(const void *fdt, int offset);
 
@@ -237,8 +237,8 @@ int fdt_first_subnode(const void *fdt, int offset);
  * After first calling fdt_first_subnode(), call this function repeatedly to
  * get direct subnodes of a parent node.
  *
- * Return: offset of next subnode, or -FDT_ERR_NOTFOUND if there are no more
- *         subnodes
+ * returns: offset of next subnode, or -FDT_ERR_NOTFOUND if there are no more
+ *          subnodes
  */
 int fdt_next_subnode(const void *fdt, int offset);
 
@@ -307,7 +307,7 @@ fdt_set_hdr_(size_dt_struct)
  * fdt_header_size - return the size of the tree's header
  * @fdt: pointer to a flattened device tree
  *
- * Return: size of DTB header in bytes
+ * returns: size of DTB header in bytes
  */
 size_t fdt_header_size(const void *fdt);
 
@@ -315,7 +315,7 @@ size_t fdt_header_size(const void *fdt);
  * fdt_header_size_ - internal function to get header size from a version number
  * @version: device tree version number
  *
- * Return: size of DTB header in bytes
+ * returns: size of DTB header in bytes
  */
 size_t fdt_header_size_(uint32_t version);
 
@@ -462,7 +462,7 @@ static inline uint32_t fdt_get_max_phandle(const void *fdt)
  * highest phandle value in the device tree blob) will be returned in the
  * @phandle parameter.
  *
- * Return: 0 on success or a negative error-code on failure
+ * returns: 0 on success or a negative error-code on failure
  */
 int fdt_generate_phandle(const void *fdt, uint32_t *phandle);
 
@@ -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);
 
@@ -510,7 +515,7 @@ int fdt_get_mem_rsv(const void *fdt, int n, uint64_t *address, uint64_t *size);
  * useful for finding subnodes based on a portion of a larger string,
  * such as a full path.
  *
- * Return: offset of the subnode or -FDT_ERR_NOTFOUND if name not found.
+ * returns: offset of the subnode or -FDT_ERR_NOTFOUND if name not found.
  */
 #ifndef SWIG /* Not available in Python */
 int fdt_subnode_offset_namelen(const void *fdt, int parentoffset,
@@ -551,7 +556,7 @@ int fdt_subnode_offset(const void *fdt, int parentoffset, const char *name);
  * Identical to fdt_path_offset(), but only consider the first namelen
  * characters of path as the path name.
  *
- * Return: offset of the node or negative libfdt error value otherwise
+ * returns: offset of the node or negative libfdt error value otherwise
  */
 #ifndef SWIG /* Not available in Python */
 int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen);
@@ -749,7 +754,7 @@ static inline struct fdt_property *fdt_get_property_by_offset_w(void *fdt,
  * Identical to fdt_get_property(), but only examine the first namelen
  * characters of name for matching the property name.
  *
- * Return: pointer to the structure representing the property, or NULL
+ * returns: pointer to the structure representing the property, or NULL
  *         if not found
  */
 #ifndef SWIG /* Not available in Python */
@@ -851,7 +856,7 @@ const void *fdt_getprop_by_offset(const void *fdt, int offset,
  * Identical to fdt_getprop(), but only examine the first namelen
  * characters of name for matching the property name.
  *
- * Return: pointer to the property's value or NULL on error
+ * returns: pointer to the property's value or NULL on error
  */
 #ifndef SWIG /* Not available in Python */
 const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
@@ -924,8 +929,8 @@ uint32_t fdt_get_phandle(const void *fdt, int nodeoffset);
  * Identical to fdt_get_alias(), but only examine the first @namelen
  * characters of @name for matching the alias name.
  *
- * Return: a pointer to the expansion of the alias named @name, if it exists,
- *	   NULL otherwise
+ * returns: a pointer to the expansion of the alias named @name, if it exists,
+ *	    NULL otherwise
  */
 #ifndef SWIG /* Not available in Python */
 const char *fdt_get_alias_namelen(const void *fdt,
@@ -955,8 +960,8 @@ const char *fdt_get_alias(const void *fdt, const char *name);
  * Identical to fdt_get_symbol(), but only examine the first @namelen
  * characters of @name for matching the symbol name.
  *
- * Return: a pointer to the expansion of the symbol named @name, if it exists,
- *	   NULL otherwise
+ * returns: a pointer to the expansion of the symbol named @name, if it exists,
+ *	    NULL otherwise
  */
 #ifndef SWIG /* Not available in Python */
 const char *fdt_get_symbol_namelen(const void *fdt,
@@ -1220,7 +1225,7 @@ int fdt_node_offset_by_compatible(const void *fdt, int startoffset,
  * one or more strings, each terminated by \0, as is found in a device tree
  * "compatible" property.
  *
- * Return: 1 if the string is found in the list, 0 not found, or invalid list
+ * returns: 1 if the string is found in the list, 0 not found, or invalid list
  */
 int fdt_stringlist_contains(const char *strlist, int listlen, const char *str);
 
@@ -1230,7 +1235,7 @@ int fdt_stringlist_contains(const char *strlist, int listlen, const char *str);
  * @nodeoffset: offset of a tree node
  * @property: name of the property containing the string list
  *
- * Return:
+ * returns:
  *   the number of strings in the given property
  *   -FDT_ERR_BADVALUE if the property value is not NUL-terminated
  *   -FDT_ERR_NOTFOUND if the property does not exist
@@ -1274,7 +1279,7 @@ int fdt_stringlist_search(const void *fdt, int nodeoffset, const char *property,
  * If non-NULL, the length of the string (on success) or a negative error-code
  * (on failure) will be stored in the integer pointer to by lenp.
  *
- * Return:
+ * returns:
  *   A pointer to the string at the given index in the string list or NULL on
  *   failure. On success the length of the string will be stored in the memory
  *   location pointed to by the lenp parameter, if non-NULL. On failure one of
@@ -1364,7 +1369,7 @@ int fdt_size_cells(const void *fdt, int nodeoffset);
  * of the name. It is useful when you want to manipulate only one value of
  * an array and you have a string that doesn't end with \0.
  *
- * Return: 0 on success, negative libfdt error value otherwise
+ * returns: 0 on success, negative libfdt error value otherwise
  */
 #ifndef SWIG /* Not available in Python */
 int fdt_setprop_inplace_namelen_partial(void *fdt, int nodeoffset,
@@ -1484,7 +1489,7 @@ static inline int fdt_setprop_inplace_u64(void *fdt, int nodeoffset,
  * @val: new value of the 32-bit cell
  *
  * This is an alternative name for fdt_setprop_inplace_u32()
- * Return: 0 on success, negative libfdt error number otherwise.
+ * returns: 0 on success, negative libfdt error number otherwise.
  */
 static inline int fdt_setprop_inplace_cell(void *fdt, int nodeoffset,
 					   const char *name, uint32_t val)
@@ -2084,7 +2089,7 @@ static inline int fdt_setprop_u64(void *fdt, int nodeoffset, const char *name,
  *
  * This is an alternative name for fdt_setprop_u32()
  *
- * Return: 0 on success, negative libfdt error value otherwise.
+ * returns: 0 on success, negative libfdt error value otherwise.
  */
 static inline int fdt_setprop_cell(void *fdt, int nodeoffset, const char *name,
 				   uint32_t val)
@@ -2294,7 +2299,7 @@ static inline int fdt_appendprop_u64(void *fdt, int nodeoffset,
  *
  * This is an alternative name for fdt_appendprop_u32()
  *
- * Return: 0 on success, negative libfdt error value otherwise.
+ * returns: 0 on success, negative libfdt error value otherwise.
  */
 static inline int fdt_appendprop_cell(void *fdt, int nodeoffset,
 				      const char *name, uint32_t val)
@@ -2405,8 +2410,8 @@ int fdt_delprop(void *fdt, int nodeoffset, const char *name);
  * creating subnodes based on a portion of a larger string, such as a
  * full path.
  *
- * Return: structure block offset of the created subnode (>=0),
- *	   negative libfdt error value otherwise
+ * returns: structure block offset of the created subnode (>=0),
+ *          negative libfdt error value otherwise
  */
 #ifndef SWIG /* Not available in Python */
 int fdt_add_subnode_namelen(void *fdt, int parentoffset,

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