libfdt: Ensure fdt_add_property frees allocated name string on failure

If fdt_add_property or fdt_property_placeholder fail after allocating
a string for the name, they return without freeing that string. This
does not change the structure of the tree, but in very specific cases
it could lead to undesirable space consumption.

Fix this by rolling back the string allocation in this situation.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Message-Id: <20190509094122.834-2-npiggin@gmail.com>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

6 files changed, 139 insertions, 7 deletions

diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
index 2e49855..9e76615 100644
--- a/libfdt/fdt_rw.c
+++ b/libfdt/fdt_rw.c
@@ -136,6 +136,14 @@ static int fdt_splice_struct_(void *fdt, void *p,
 	return 0;
 }
 
+/* Must only be used to roll back in case of error */
+static void fdt_del_last_string_(void *fdt, const char *s)
+{
+	int newlen = strlen(s) + 1;
+
+	fdt_set_size_dt_strings(fdt, fdt_size_dt_strings(fdt) - newlen);
+}
+
 static int fdt_splice_string_(void *fdt, int newlen)
 {
 	void *p = (char *)fdt
@@ -149,7 +157,7 @@ static int fdt_splice_string_(void *fdt, int newlen)
 	return 0;
 }
 
-static int fdt_find_add_string_(void *fdt, const char *s)
+static int fdt_find_add_string_(void *fdt, const char *s, int *allocated)
 {
 	char *strtab = (char *)fdt + fdt_off_dt_strings(fdt);
 	const char *p;
@@ -157,6 +165,8 @@ static int fdt_find_add_string_(void *fdt, const char *s)
 	int len = strlen(s) + 1;
 	int err;
 
+	*allocated = 0;
+
 	p = fdt_find_string_(strtab, fdt_size_dt_strings(fdt), s);
 	if (p)
 		/* found it */
@@ -167,6 +177,8 @@ static int fdt_find_add_string_(void *fdt, const char *s)
 	if (err)
 		return err;
 
+	*allocated = 1;
+
 	memcpy(new, s, len);
 	return (new - strtab);
 }
@@ -225,11 +237,12 @@ static int fdt_add_property_(void *fdt, int nodeoffset, const char *name,
 	int nextoffset;
 	int namestroff;
 	int err;
+	int allocated;
 
 	if ((nextoffset = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
 		return nextoffset;
 
-	namestroff = fdt_find_add_string_(fdt, name);
+	namestroff = fdt_find_add_string_(fdt, name, &allocated);
 	if (namestroff < 0)
 		return namestroff;
 
@@ -237,8 +250,11 @@ static int fdt_add_property_(void *fdt, int nodeoffset, const char *name,
 	proplen = sizeof(**prop) + FDT_TAGALIGN(len);
 
 	err = fdt_splice_struct_(fdt, *prop, 0, proplen);
-	if (err)
+	if (err) {
+		if (allocated)
+			fdt_del_last_string_(fdt, name);
 		return err;
+	}
 
 	(*prop)->tag = cpu_to_fdt32(FDT_PROP);
 	(*prop)->nameoff = cpu_to_fdt32(namestroff);
diff --git a/libfdt/fdt_sw.c b/libfdt/fdt_sw.c
index 9fa4a94..113f28a 100644
--- a/libfdt/fdt_sw.c
+++ b/libfdt/fdt_sw.c
@@ -262,7 +262,16 @@ int fdt_end_node(void *fdt)
 	return 0;
 }
 
-static int fdt_find_add_string_(void *fdt, const char *s)
+/* Must only be used to roll back in case of error */
+static void fdt_del_last_string_(void *fdt, const char *s)
+{
+	int strtabsize = fdt_size_dt_strings(fdt);
+	int len = strlen(s) + 1;
+
+	fdt_set_size_dt_strings(fdt, strtabsize - len);
+}
+
+static int fdt_find_add_string_(void *fdt, const char *s, int *allocated)
 {
 	char *strtab = (char *)fdt + fdt_totalsize(fdt);
 	const char *p;
@@ -270,11 +279,15 @@ static int fdt_find_add_string_(void *fdt, const char *s)
 	int len = strlen(s) + 1;
 	int struct_top, offset;
 
+	*allocated = 0;
+
 	p = fdt_find_string_(strtab - strtabsize, strtabsize, s);
 	if (p)
 		return p - strtab;
 
 	/* Add it */
+	*allocated = 1;
+
 	offset = -strtabsize - len;
 	struct_top = fdt_off_dt_struct(fdt) + fdt_size_dt_struct(fdt);
 	if (fdt_totalsize(fdt) + offset < struct_top)
@@ -289,16 +302,20 @@ int fdt_property_placeholder(void *fdt, const char *name, int len, void **valp)
 {
 	struct fdt_property *prop;
 	int nameoff;
+	int allocated;
 
 	FDT_SW_PROBE_STRUCT(fdt);
 
-	nameoff = fdt_find_add_string_(fdt, name);
+	nameoff = fdt_find_add_string_(fdt, name, &allocated);
 	if (nameoff == 0)
 		return -FDT_ERR_NOSPACE;
 
 	prop = fdt_grab_space_(fdt, sizeof(*prop) + FDT_TAGALIGN(len));
-	if (! prop)
+	if (! prop) {
+		if (allocated)
+			fdt_del_last_string_(fdt, name);
 		return -FDT_ERR_NOSPACE;
+	}
 
 	prop->tag = cpu_to_fdt32(FDT_PROP);
 	prop->nameoff = cpu_to_fdt32(nameoff);
diff --git a/tests/.gitignore b/tests/.gitignore
index 0bc78aa..d3f1434 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -56,6 +56,7 @@ tmp.*
 /references
 /root_node
 /rw_tree1
+/rw_oom
 /set_name
 /setprop
 /setprop_inplace
diff --git a/tests/Makefile.tests b/tests/Makefile.tests
index 1f8feed..5093aaa 100644
--- a/tests/Makefile.tests
+++ b/tests/Makefile.tests
@@ -15,7 +15,7 @@ LIB_TESTS_L = get_mem_rsv \
 	setprop_inplace nop_property nop_node \
 	sw_tree1 sw_states \
 	move_and_save mangle-layout nopulate \
-	open_pack rw_tree1 set_name setprop del_property del_node \
+	open_pack rw_tree1 rw_oom set_name setprop del_property del_node \
 	appendprop1 appendprop2 propname_escapes \
 	string_escapes references path-references phandle_format \
 	boot-cpuid incbin \
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 489a3d2..d350c3d 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -421,6 +421,8 @@ libfdt_tests () {
 	tree1_tests_rw noppy.$basetree
     done
 
+    run_test rw_oom
+
     run_dtc_test -I dts -O dtb -o subnode_iterate.dtb subnode_iterate.dts
     run_test subnode_iterate subnode_iterate.dtb
 
diff --git a/tests/rw_oom.c b/tests/rw_oom.c
new file mode 100644
index 0000000..a787dc2
--- /dev/null
+++ b/tests/rw_oom.c
@@ -0,0 +1,96 @@
+/*
+ * libfdt - Flat Device Tree manipulation
+ *	Testcase for fdt_nop_node()
+ * Copyright (C) 2006 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 <ctype.h>
+#include <stdint.h>
+
+#include <libfdt.h>
+
+#include "tests.h"
+#include "testdata.h"
+
+/* This is not derived programatically. May require adjustment to changes. */
+#define SPACE	285
+
+#define CHECK(code) \
+	{ \
+		err = (code); \
+		if (err) \
+			FAIL(#code ": %s", fdt_strerror(err)); \
+	}
+
+#define OFF_CHECK(off, code) \
+	{ \
+		(off) = (code); \
+		if (off < 0) \
+			FAIL(#code ": %s", fdt_strerror(off)); \
+	}
+
+int main(int argc, char *argv[])
+{
+	void *fdt;
+	int err;
+	int offset, s1;
+	int strsize1, strsize2;
+
+	/*
+	 * Check OOM path, and check that property is cleaned up if it fails
+	 * with OOM, rather than adding an orphan name.
+	 *
+	 * SW OOM is tested with the realloc/resize strategies.
+	 */
+	test_init(argc, argv);
+
+	fdt = xmalloc(SPACE);
+
+	/* First create empty tree with SW */
+	CHECK(fdt_create_empty_tree(fdt, SPACE));
+
+	CHECK(fdt_add_mem_rsv(fdt, TEST_ADDR_1, TEST_SIZE_1));
+	CHECK(fdt_add_mem_rsv(fdt, TEST_ADDR_2, TEST_SIZE_2));
+
+	CHECK(fdt_setprop_string(fdt, 0, "compatible", "test_oom"));
+	CHECK(fdt_setprop_u32(fdt, 0, "prop-int", TEST_VALUE_1));
+	CHECK(fdt_setprop_u64(fdt, 0, "prop-int64", TEST_VALUE64_1));
+	CHECK(fdt_setprop_string(fdt, 0, "prop-str", TEST_STRING_1));
+
+	OFF_CHECK(offset, fdt_add_subnode(fdt, 0, "subnode@1"));
+	s1 = offset;
+
+	strsize1 = fdt_size_dt_strings(fdt);
+	err = fdt_setprop_string(fdt, s1, "unique", "subnode1");
+	if (err != -FDT_ERR_NOSPACE)
+		FAIL("fdt_setprop_string(fdt, s1, \"compatible\", \"subnode1\"): %s", fdt_strerror(err));
+	strsize2 = fdt_size_dt_strings(fdt);
+
+	if (strsize1 != strsize2)
+		FAIL("fdt_setprop NOSPACE error failed to clean up allocated string\n");
+	err = 0;
+
+	/* Ensure we failed in the right place */
+	CHECK(fdt_setprop_string(fdt, s1, "unique", ""));
+
+	CHECK(fdt_pack(fdt));
+
+	PASS();
+}