libfdt: Safer access to strings section

fdt_string() is used to retrieve strings from a DT blob's strings section.
It's rarely used directly, but is widely used internally.

However, it doesn't do any bounds checking, which means in the case of a
corrupted blob it could access bad memory, which libfdt is supposed to
avoid.

This write a safe alternative to fdt_string, fdt_get_string().  It checks
both that the given offset is within the string section and that the string
it points to is properly \0 terminated within the section.  It also returns
the string's length as a convenience (since it needs to determine to do the
checks anyway).

fdt_string() is rewritten in terms of fdt_get_string() for compatibility.

Most of the diff here is actually testing infrastructure.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Tested-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>

libfdt/fdt_ro.c

@@ -76,17 +76,72 @@ static int fdt_nodename_eq_(const void *fdt, int offset,
 		return 0;
 }
 
+const char *fdt_get_string(const void *fdt, int stroffset, int *lenp)
+{
+	uint32_t absoffset = stroffset + fdt_off_dt_strings(fdt);
+	size_t len;
+	int err;
+	const char *s, *n;
+
+	err = fdt_ro_probe_(fdt);
+	if (err != 0)
+		goto fail;
+
+	err = -FDT_ERR_BADOFFSET;
+	if (absoffset >= fdt_totalsize(fdt))
+		goto fail;
+	len = fdt_totalsize(fdt) - absoffset;
+
+	if (fdt_magic(fdt) == FDT_MAGIC) {
+		if (stroffset < 0)
+			goto fail;
+		if (fdt_version(fdt) >= 17) {
+			if (stroffset >= fdt_size_dt_strings(fdt))
+				goto fail;
+			if ((fdt_size_dt_strings(fdt) - stroffset) < len)
+				len = fdt_size_dt_strings(fdt) - stroffset;
+		}
+	} else if (fdt_magic(fdt) == FDT_SW_MAGIC) {
+		if ((stroffset >= 0)
+		    || (stroffset < -fdt_size_dt_strings(fdt)))
+			goto fail;
+		if ((-stroffset) < len)
+			len = -stroffset;
+	} else {
+		err = -FDT_ERR_INTERNAL;
+		goto fail;
+	}
+
+	s = (const char *)fdt + absoffset;
+	n = memchr(s, '\0', len);
+	if (!n) {
+		/* missing terminating NULL */
+		err = -FDT_ERR_TRUNCATED;
+		goto fail;
+	}
+
+	if (lenp)
+		*lenp = n - s;
+	return s;
+
+fail:
+	if (lenp)
+		*lenp = err;
+	return NULL;
+}
+
 const char *fdt_string(const void *fdt, int stroffset)
 {
-	return (const char *)fdt + fdt_off_dt_strings(fdt) + stroffset;
+	return fdt_get_string(fdt, stroffset, NULL);
 }
 
 static int fdt_string_eq_(const void *fdt, int stroffset,
 			  const char *s, int len)
 {
-	const char *p = fdt_string(fdt, stroffset);
+	int slen;
+	const char *p = fdt_get_string(fdt, stroffset, &slen);
 
-	return (strlen(p) == len) && (memcmp(p, s, len) == 0);
+	return p && (slen == len) && (memcmp(p, s, len) == 0);
 }
 
 uint32_t fdt_get_max_phandle(const void *fdt)

libfdt/libfdt.h

@@ -287,6 +287,22 @@ int fdt_move(const void *fdt, void *buf, int bufsize);
 /* Read-only functions                                                */
 /**********************************************************************/
 
+/**
+ * fdt_get_string - retrieve a string from the strings block of a device tree
+ * @fdt: pointer to the device tree blob
+ * @stroffset: offset of the string within the strings block (native endian)
+ * @lenp: optional pointer to return the string's length
+ *
+ * fdt_get_string() retrieves a pointer to a single string from the
+ * strings block of the device tree blob at fdt, and optionally also
+ * returns the string's length in *lenp.
+ *
+ * returns:
+ *     a pointer to the string, on success
+ *     NULL, if stroffset is out of bounds, or doesn't point to a valid string
+ */
+const char *fdt_get_string(const void *fdt, int stroffset, int *lenp);
+
 /**
  * fdt_string - retrieve a string from the strings block of a device tree
  * @fdt: pointer to the device tree blob
@@ -297,7 +313,7 @@ int fdt_move(const void *fdt, void *buf, int bufsize);
  *
  * returns:
  *     a pointer to the string, on success
- *     NULL, if stroffset is out of bounds
+ *     NULL, if stroffset is out of bounds, or doesn't point to a valid string
  */
 const char *fdt_string(const void *fdt, int stroffset);
 

libfdt/version.lds

@@ -65,7 +65,7 @@ LIBFDT_1.2 {
 		fdt_stringlist_get;
 		fdt_resize;
 		fdt_overlay_apply;
-
+		fdt_get_string;
 	local:
 		*;
 };

tests/.gitignore

@@ -61,5 +61,6 @@ tmp.*
 /sw_tree1
 /sw_states
 /truncated_property
+/truncated_string
 /utilfdt_test
 /value-labels

tests/Makefile.tests

@@ -29,7 +29,7 @@ LIB_TESTS_L = get_mem_rsv \
 	check_path check_header
 LIB_TESTS = $(LIB_TESTS_L:%=$(TESTS_PREFIX)%)
 
-LIBTREE_TESTS_L = truncated_property
+LIBTREE_TESTS_L = truncated_property truncated_string
 LIBTREE_TESTS = $(LIBTREE_TESTS_L:%=$(TESTS_PREFIX)%)
 
 DL_LIB_TESTS_L = asm_tree_dump value-labels

tests/run_tests.sh

@@ -415,6 +415,7 @@ libfdt_tests () {
 
     # Tests for behaviour on various sorts of corrupted trees
     run_test truncated_property
+    run_test truncated_string
 
     # Check aliases support in fdt_path_offset
     run_dtc_test -I dts -O dtb -o aliases.dtb aliases.dts

tests/testdata.h

@@ -47,4 +47,5 @@ extern struct fdt_header bad_node_char;
 extern struct fdt_header bad_node_format;
 extern struct fdt_header bad_prop_char;
 extern struct fdt_header ovf_size_strings;
+extern struct fdt_header truncated_string;
 #endif /* ! __ASSEMBLY */

tests/testutils.c

@@ -88,7 +88,7 @@ void check_property(void *fdt, int nodeoffset, const char *name,
 		    int len, const void *val)
 {
 	const struct fdt_property *prop;
-	int retlen;
+	int retlen, namelen;
 	uint32_t tag, nameoff, proplen;
 	const char *propname;
 
@@ -106,8 +106,13 @@ void check_property(void *fdt, int nodeoffset, const char *name,
 	if (tag != FDT_PROP)
 		FAIL("Incorrect tag 0x%08x on property \"%s\"", tag, name);
 
-	propname = fdt_string(fdt, nameoff);
-	if (!propname || !streq(propname, name))
+	propname = fdt_get_string(fdt, nameoff, &namelen);
+	if (!propname)
+		FAIL("Couldn't get property name: %s", fdt_strerror(namelen));
+	if (namelen != strlen(propname))
+		FAIL("Incorrect prop name length: %d instead of %zd",
+		     namelen, strlen(propname));
+	if (!streq(propname, name))
 		FAIL("Property name mismatch \"%s\" instead of \"%s\"",
 		     propname, name);
 	if (proplen != retlen)

tests/trees.S

@@ -39,6 +39,9 @@ tree##_rsvmap_end:		;
 	FDTLONG(len)		; \
 	FDTLONG(tree##_##name - tree##_strings) ;
 
+#define PROP_EMPTY(tree, name) \
+	PROPHDR(tree, name, 0)	;
+
 #define PROP_INT(tree, name, val) \
 	PROPHDR(tree, name, 4) \
 	FDTLONG(val)		;
@@ -233,3 +236,26 @@ ovf_size_strings_strings:
 	ovf_size_strings_bad_string = ovf_size_strings_strings + 0x10000000
 ovf_size_strings_strings_end:
 ovf_size_strings_end:
+
+
+	/* truncated_string */
+	TREE_HDR(truncated_string)
+	EMPTY_RSVMAP(truncated_string)
+
+truncated_string_struct:
+	BEGIN_NODE("")
+	PROP_EMPTY(truncated_string, good_string)
+	PROP_EMPTY(truncated_string, bad_string)
+	END_NODE
+	FDTLONG(FDT_END)
+truncated_string_struct_end:
+
+truncated_string_strings:
+	STRING(truncated_string, good_string, "good")
+truncated_string_bad_string:
+	.byte	'b'
+	.byte	'a'
+	.byte	'd'
+	/* NOTE: terminating \0 deliberately missing */
+truncated_string_strings_end:
+truncated_string_end:

tests/truncated_string.c

@@ -0,0 +1,79 @@
+/*
+ * libfdt - Flat Device Tree manipulation
+ *	Testcase for misbehaviour on a truncated string
+ * Copyright (C) 2018 David Gibson, IBM Corporation.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public License
+ * as published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <stdint.h>
+
+#include <libfdt.h>
+
+#include "tests.h"
+#include "testdata.h"
+
+int main(int argc, char *argv[])
+{
+	void *fdt = &truncated_string;
+	const struct fdt_property *good, *bad;
+	int off, len;
+	const char *name;
+
+	test_init(argc, argv);
+
+	off = fdt_first_property_offset(fdt, 0);
+	good = fdt_get_property_by_offset(fdt, off, NULL);
+
+	off = fdt_next_property_offset(fdt, off);
+	bad = fdt_get_property_by_offset(fdt, off, NULL);
+
+	if (fdt32_to_cpu(good->len) != 0)
+		FAIL("Unexpected length for good property");
+	name = fdt_get_string(fdt, fdt32_to_cpu(good->nameoff), &len);
+	if (!name)
+		FAIL("fdt_get_string() failed on good property: %s",
+		     fdt_strerror(len));
+	if (len != 4)
+		FAIL("fdt_get_string() returned length %d (not 4) on good property",
+		     len);
+	if (!streq(name, "good"))
+		FAIL("fdt_get_string() returned \"%s\" (not \"good\") on good property",
+		     name);
+
+	if (fdt32_to_cpu(bad->len) != 0)
+		FAIL("Unexpected length for bad property\n");
+	name = fdt_get_string(fdt, fdt32_to_cpu(bad->nameoff), &len);
+	if (name)
+		FAIL("fdt_get_string() succeeded incorrectly on bad property");
+	else if (len != -FDT_ERR_TRUNCATED)
+		FAIL("fdt_get_string() gave unexpected error on bad property: %s",
+		     fdt_strerror(len));
+
+	/* Make sure the 'good' property breaks correctly if we
+	 * truncate the strings section */
+	fdt_set_size_dt_strings(fdt, fdt32_to_cpu(good->nameoff) + 4);
+	name = fdt_get_string(fdt, fdt32_to_cpu(good->nameoff), &len);
+	if (name)
+		FAIL("fdt_get_string() succeeded incorrectly on mangled property");
+	else if (len != -FDT_ERR_TRUNCATED)
+		FAIL("fdt_get_string() gave unexpected error on mangled property: %s",
+		     fdt_strerror(len));
+
+	PASS();
+}