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

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

[Ayan Kumar Halder]

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 this needs a bad nodeoffset.

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

v1: Message-ID <20261001121036.2910243-1-ayan.kumar.halder@amd.com>

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

diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index 11f2e2e..555bba0 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -573,7 +573,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);
@@ -581,8 +581,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..1681747 100644
--- a/tests/get_path.c
+++ b/tests/get_path.c
@@ -63,6 +63,34 @@ 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.
+ * Run under valgrind (run_tests.sh -m) to catch that read.
+ */
+static void check_path_bad_offset(void *fdt)
+{
+	int offset, depth = 0, badoffset = 0;
+	char buf[64];
+	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, sizeof(buf));
+
+		len = fdt_get_path(fdt, badoffset + i, buf, sizeof(buf));
+		if (len != -FDT_ERR_BADOFFSET && len != -FDT_ERR_BADSTRUCTURE)
+			FAIL("fdt_get_path(%d) returns %d instead of an error",
+			     badoffset + i, len);
+	}
+}
+
 int main(int argc, char *argv[])
 {
 	void *fdt;
@@ -76,5 +104,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

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

[Simon Glass]

Hi Ayan,

On 2026-10-30T17:06:02, Ayan Kumar Halder <ayan.kumar.halder@amd.com> wrote:
> libfdt: Fix fdt_get_path() reading before the start of the buffer
>
> 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 this needs a bad nodeoffset.

The last sentence reads oddly. Perhaps: '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.
> [...]
>
>  libfdt/fdt_ro.c  |  8 ++++++--
>  tests/get_path.c | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+), 2 deletions(-)

> diff --git a/tests/get_path.c b/tests/get_path.c
> @@ -63,6 +63,34 @@ static void check_path(void *fdt, const char *path)
> +static void check_path_bad_offset(void *fdt)
> +{
> +	int offset, depth = 0, badoffset = 0;
> +	char buf[64];
> +	int len, i;

I tried this test against the old fdt_get_path(), i.e. with this
patch's fix in libfdt/fdt_ro.c removed (both the up-front
fdt_check_node_offset_() call and the 'depth >= 0' loop condition),
keeping only the new test. A plain run still passes, since
fdt_get_name() fails on the FDT_END tag and the function returns
-FDT_ERR_BADOFFSET anyway. Valgrind catches it, but only because the
stray read happens to land below the stack pointer ('129 bytes below
stack pointer'), which depends on the frame layout. Please can you
allocate the buffer with xmalloc() so that valgrind (or ASan) reliably
sees the read before the start of the block?

> diff --git a/tests/get_path.c b/tests/get_path.c
> @@ -63,6 +63,34 @@ static void check_path(void *fdt, const char *path)
> +		len = fdt_get_path(fdt, badoffset + i, buf, sizeof(buf));
> +		if (len != -FDT_ERR_BADOFFSET && len != -FDT_ERR_BADSTRUCTURE)
> +			FAIL("fdt_get_path(%d) returns %d instead of an error",
> +			     badoffset + i, len);

With the up-front fdt_check_node_offset_() call, all of these offsets
(misaligned, the FDT_END tag and past the end of the struct block) get
-FDT_ERR_BADOFFSET, so I don't think -FDT_ERR_BADSTRUCTURE needs to be
accepted. Checking only for -FDT_ERR_BADOFFSET would make the test
stricter.

Regards,
Simon

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

[Halder, Ayan Kumar]

On 01/11/2026 18:25, Simon Glass wrote:
> Hi Ayan,
Hi Simon,
>
> On 2026-10-30T17:06:02, Ayan Kumar Halder <ayan.kumar.halder@amd.com> wrote:
>> libfdt: Fix fdt_get_path() reading before the start of the buffer
>>
>> 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 this needs a bad nodeoffset.
> The last sentence reads oddly. Perhaps: '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.
>> [...]
>>
>>   libfdt/fdt_ro.c  |  8 ++++++--
>>   tests/get_path.c | 30 ++++++++++++++++++++++++++++++
>>   2 files changed, 36 insertions(+), 2 deletions(-)
>> diff --git a/tests/get_path.c b/tests/get_path.c
>> @@ -63,6 +63,34 @@ static void check_path(void *fdt, const char *path)
>> +static void check_path_bad_offset(void *fdt)
>> +{
>> +	int offset, depth = 0, badoffset = 0;
>> +	char buf[64];
>> +	int len, i;
> I tried this test against the old fdt_get_path(), i.e. with this
> patch's fix in libfdt/fdt_ro.c removed (both the up-front
> fdt_check_node_offset_() call and the 'depth >= 0' loop condition),
> keeping only the new test. A plain run still passes, since
> fdt_get_name() fails on the FDT_END tag and the function returns
> -FDT_ERR_BADOFFSET anyway. Valgrind catches it, but only because the
> stray read happens to land below the stack pointer ('129 bytes below
> stack pointer'), which depends on the frame layout. Please can you
> allocate the buffer with xmalloc() so that valgrind (or ASan) reliably
> sees the read before the start of the block?
>
>> diff --git a/tests/get_path.c b/tests/get_path.c
>> @@ -63,6 +63,34 @@ static void check_path(void *fdt, const char *path)
>> +		len = fdt_get_path(fdt, badoffset + i, buf, sizeof(buf));
>> +		if (len != -FDT_ERR_BADOFFSET && len != -FDT_ERR_BADSTRUCTURE)
>> +			FAIL("fdt_get_path(%d) returns %d instead of an error",
>> +			     badoffset + i, len);
> With the up-front fdt_check_node_offset_() call, all of these offsets
> (misaligned, the FDT_END tag and past the end of the struct block) get
> -FDT_ERR_BADOFFSET, so I don't think -FDT_ERR_BADSTRUCTURE needs to be
> accepted. Checking only for -FDT_ERR_BADOFFSET would make the test
> stricter.

I fixed the patch as per your comments.

Please have a look at "[PATCH v3] libfdt: Fix fdt_get_path() reading 
before the start of the buffer"

- Ayan

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4906 bytes --]

On Fri, Oct 30, 2026 at 05:06:02PM +0000, Ayan Kumar Halder wrote:
> 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 this needs a bad nodeoffset.
> 
> 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>

I agree with Simon's comments, otherwise LGTM.

> ---
> 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).
> 
> v1: Message-ID <20261001121036.2910243-1-ayan.kumar.halder@amd.com>
> 
>  libfdt/fdt_ro.c  |  8 ++++++--
>  tests/get_path.c | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index 11f2e2e..555bba0 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -573,7 +573,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);
> @@ -581,8 +581,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..1681747 100644
> --- a/tests/get_path.c
> +++ b/tests/get_path.c
> @@ -63,6 +63,34 @@ 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.
> + * Run under valgrind (run_tests.sh -m) to catch that read.
> + */
> +static void check_path_bad_offset(void *fdt)
> +{
> +	int offset, depth = 0, badoffset = 0;
> +	char buf[64];
> +	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, sizeof(buf));
> +
> +		len = fdt_get_path(fdt, badoffset + i, buf, sizeof(buf));
> +		if (len != -FDT_ERR_BADOFFSET && len != -FDT_ERR_BADSTRUCTURE)
> +			FAIL("fdt_get_path(%d) returns %d instead of an error",
> +			     badoffset + i, len);
> +	}
> +}
> +
>  int main(int argc, char *argv[])
>  {
>  	void *fdt;
> @@ -76,5 +104,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
> 
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]