[PATCH v3] libfdt: Fix fdt_get_path() reading before the start of the buffer

[Ayan Kumar Halder <ayan.kumar.halder@amd.com>]

Asking fdt_get_path() for an offset past the root node's FDT_END_NODE
tag makes it read before the start of the caller's buffer. Such an
offset is not a node offset, so fdt_get_path() should return
-FDT_ERR_BADOFFSET.

fdt_get_path() trims a component off the buffer for each level the walk
comes back up, comparing the components it has against the depth from
fdt_next_node(). On leaving the root node, fdt_next_node() takes the
depth negative and returns the offset just past that tag rather than an
error, as commit ce4d9c0f ("libfdt: Rework fdt_next_node()") intended so
that fdt_node_end_offset_() can find where a node ends. The loop only
tests the offset, so it runs once more and tries to trim a component
that is not there, scanning back past the start of the buffer.

Stop the walk once the depth goes negative, as
fdt_subnode_offset_namelen() and fdt_node_end_offset_() already do, and
reject the offset up front with fdt_check_node_offset_() where input is
not assumed valid. Add a bad-offset case to tests/get_path.c; run it
under valgrind (tests/run_tests.sh -m) to catch the read.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes in v3:
 - Reworded the opening sentence to say fdt_get_path() should return
   -FDT_ERR_BADOFFSET (Simon).
 - tests/get_path.c: allocate the buffer with xmalloc() so valgrind sees
   the read before the start of the block, rather than only when the scan
   runs past the stack pointer (Simon).
 - tests/get_path.c: accept only -FDT_ERR_BADOFFSET, since the up-front
   fdt_check_node_offset_() makes -FDT_ERR_BADSTRUCTURE unreachable for
   these offsets (Simon).

Changes in v2:
 - Reworded the commit message: say FDT_END_NODE rather than "end tag",
   state the problem first, and make clear it needs a bad nodeoffset
   (Simon, David).
 - Referenced commit ce4d9c0f ("libfdt: Rework fdt_next_node()") for why
   fdt_next_node() returns that offset (David).
 - Also reject the offset up front with fdt_check_node_offset_(), under
   can_assume(VALID_INPUT) (Simon, David).
 - Added a bad-offset case to tests/get_path.c (Simon, David).

 libfdt/fdt_ro.c  |  8 ++++++--
 tests/get_path.c | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index fed2f9f..05903e3 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -583,7 +583,7 @@ const char *fdt_get_symbol(const void *fdt, const char *name)
 int fdt_get_path(const void *fdt, int nodeoffset, char *buf, int buflen)
 {
 	int pdepth = 0, p = 0;
-	int offset, depth, namelen;
+	int offset, depth, namelen, err;
 	const char *name;
 
 	FDT_RO_PROBE(fdt);
@@ -591,8 +591,12 @@ int fdt_get_path(const void *fdt, int nodeoffset, char *buf, int buflen)
 	if (buflen < 2)
 		return -FDT_ERR_NOSPACE;
 
+	if (!can_assume(VALID_INPUT)
+	    && ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0))
+		return err;
+
 	for (offset = 0, depth = 0;
-	     (offset >= 0) && (offset <= nodeoffset);
+	     (offset >= 0) && (depth >= 0) && (offset <= nodeoffset);
 	     offset = fdt_next_node(fdt, offset, &depth)) {
 		while (pdepth > depth) {
 			do {
diff --git a/tests/get_path.c b/tests/get_path.c
index 7349898..da8b26a 100644
--- a/tests/get_path.c
+++ b/tests/get_path.c
@@ -63,6 +63,36 @@ static void check_path(void *fdt, const char *path)
 	check_path_buf(fdt, path, pathlen, 2);
 }
 
+/*
+ * An offset past the root node's FDT_END_NODE tag is not a node offset,
+ * but fdt_next_node() still returns it rather than an error (see commit
+ * ce4d9c0f "libfdt: Rework fdt_next_node()"), so fdt_get_path() has to
+ * recognise it from the depth going negative.  Before it did, the path
+ * trimming scanned back past the start of the buffer for such an offset.
+ */
+static void check_path_bad_offset(void *fdt)
+{
+	int offset, depth = 0, badoffset = 0;
+	int buflen = 64;
+	char *buf = xmalloc(buflen);
+	int len, i;
+
+	/* The offset fdt_next_node() reports once it has left the tree. */
+	for (offset = 0; offset >= 0; offset = fdt_next_node(fdt, offset, &depth))
+		badoffset = offset;
+
+	for (i = 0; i < 8; i++) {
+		memset(buf, POISON, buflen);
+
+		len = fdt_get_path(fdt, badoffset + i, buf, buflen);
+		if (len != -FDT_ERR_BADOFFSET)
+			FAIL("fdt_get_path(%d) returns %d instead of "
+			     "-FDT_ERR_BADOFFSET", badoffset + i, len);
+	}
+
+	free(buf);
+}
+
 int main(int argc, char *argv[])
 {
 	void *fdt;
@@ -76,5 +106,7 @@ int main(int argc, char *argv[])
 	check_path(fdt, "/subnode@1/subsubnode");
 	check_path(fdt, "/subnode@2/subsubnode@0");
 
+	check_path_bad_offset(fdt);
+
 	PASS();
 }
-- 
2.25.1