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

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

[Ayan Kumar Halder]

fdt_get_path() walks the tree from the root, appending each node's name
to the caller's buffer as it goes down and removing the last component
again as it comes back up.  It keeps the number of components currently
in the buffer in pdepth, and removes components while pdepth is greater
than the depth reported by fdt_next_node().

fdt_next_node() reports that it has left the tree by taking the depth
negative, but the offset it returns at that point is the offset of the
end tag, which is not negative.  The loop only tests the offset, so for a
nodeoffset at or past the end tag it runs one more time with a negative
depth.  pdepth still counts the components in the buffer, so the removal
loop is asked to remove more components than are there: it takes the
index down to zero and carries on scanning backwards for a '/' that is
not in the buffer.

Stop the walk when the depth shows it has left the tree.  Offsets that
far along are rejected by the error paths below in any case, so this
does not change what callers see.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
 libfdt/fdt_ro.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index 11f2e2e..32689c7 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -582,7 +582,7 @@ int fdt_get_path(const void *fdt, int nodeoffset, char *buf, int buflen)
 		return -FDT_ERR_NOSPACE;
 
 	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 {
-- 
2.25.1

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

[Herve Codina]

Hi Ayan,

On Thu, 1 Oct 2026 13:10:36 +0100
Ayan Kumar Halder <ayan.kumar.halder@amd.com> wrote:

> fdt_get_path() walks the tree from the root, appending each node's name
> to the caller's buffer as it goes down and removing the last component
> again as it comes back up.  It keeps the number of components currently
> in the buffer in pdepth, and removes components while pdepth is greater
> than the depth reported by fdt_next_node().
> 
> fdt_next_node() reports that it has left the tree by taking the depth
> negative, but the offset it returns at that point is the offset of the
> end tag, which is not negative.  The loop only tests the offset, so for a

fdt_next_node() returns an error (-FDT_ERR_NOTFOUND) when it reaches the
FDT_END tag [1].

I don't understand your point. Maybe I've missed something.
Can you clarify?

[1] https://github.com/dgibson/dtc/blob/7a1e017926004ecff5fce62d62d42ce9f3e00082/libfdt/fdt.c#L283

> nodeoffset at or past the end tag it runs one more time with a negative
> depth.  pdepth still counts the components in the buffer, so the removal
> loop is asked to remove more components than are there: it takes the
> index down to zero and carries on scanning backwards for a '/' that is
> not in the buffer.
> 
> Stop the walk when the depth shows it has left the tree.  Offsets that
> far along are rejected by the error paths below in any case, so this
> does not change what callers see.

If fdt_next_node() can return an offset (positive value) which is not pointing
to a node without any errors (negative returned value), fdt_next_node() has
to be fixed not callers.

Can you give details leading fdt_next_node() returning a valid offset which
doesn't point to a node?

Best regards,
Hervé

> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
>  libfdt/fdt_ro.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index 11f2e2e..32689c7 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -582,7 +582,7 @@ int fdt_get_path(const void *fdt, int nodeoffset, char *buf, int buflen)
>  		return -FDT_ERR_NOSPACE;
>  
>  	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 {

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

[Halder, Ayan Kumar]

On 01/10/2026 16:10, Herve Codina wrote:
> Hi Ayan,

Hi Herve,

Thanks for the review.

>
> On Thu, 1 Oct 2026 13:10:36 +0100
> Ayan Kumar Halder <ayan.kumar.halder@amd.com> wrote:
>
>> fdt_get_path() walks the tree from the root, appending each node's name
>> to the caller's buffer as it goes down and removing the last component
>> again as it comes back up.  It keeps the number of components currently
>> in the buffer in pdepth, and removes components while pdepth is greater
>> than the depth reported by fdt_next_node().
>>
>> fdt_next_node() reports that it has left the tree by taking the depth
>> negative, but the offset it returns at that point is the offset of the
>> end tag, which is not negative.  The loop only tests the offset, so for a
> fdt_next_node() returns an error (-FDT_ERR_NOTFOUND) when it reaches the
> FDT_END tag [1].
>
> I don't understand your point. Maybe I've missed something.
> Can you clarify?
Sorry, my wording was wrong.  I meant the FDT_END_NODE case, just above
the one you linked:

     case FDT_END_NODE:
         if (depth && ((--(*depth)) < 0))
             return nextoffset;

Leaving the root node is an FDT_END_NODE tag, so --(*depth) takes the
depth from 0 to -1 and fdt_next_node() returns nextoffset, which is not
negative.  FDT_END is only reached on the next call.

>
> [1] https://github.com/dgibson/dtc/blob/7a1e017926004ecff5fce62d62d42ce9f3e00082/libfdt/fdt.c#L283
>
>> nodeoffset at or past the end tag it runs one more time with a negative
>> depth.  pdepth still counts the components in the buffer, so the removal
>> loop is asked to remove more components than are there: it takes the
>> index down to zero and carries on scanning backwards for a '/' that is
>> not in the buffer.
>>
>> Stop the walk when the depth shows it has left the tree.  Offsets that
>> far along are rejected by the error paths below in any case, so this
>> does not change what callers see.
> If fdt_next_node() can return an offset (positive value) which is not pointing
> to a node without any errors (negative returned value), fdt_next_node() has
> to be fixed not callers.
>
> Can you give details leading fdt_next_node() returning a valid offset which
> doesn't point to a node?
The offset is the one past root's FDT_END_NODE tag, so on
tests/test_tree1.dtb:

   offset   depth
   540      2
   560      -1    <- FDT_END_NODE: offset >= 0, depth < 0
   -4       -1    <- FDT_ERR_BADOFFSET, from the next call

Does it make sense ?

I discovered this issue by building libfdt with AddressSanitizer.

( I am new to this, so it may be my misunderstanding )

- Ayan

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

[Herve Codina]

On Thu, 1 Oct 2026 18:18:54 +0100
"Halder, Ayan Kumar" <ayankuma@amd.com> wrote:

> On 01/10/2026 16:10, Herve Codina wrote:
> > Hi Ayan,  
> 
> Hi Herve,
> 
> Thanks for the review.
> 
> >
> > On Thu, 1 Oct 2026 13:10:36 +0100
> > Ayan Kumar Halder <ayan.kumar.halder@amd.com> wrote:
> >  
> >> fdt_get_path() walks the tree from the root, appending each node's name
> >> to the caller's buffer as it goes down and removing the last component
> >> again as it comes back up.  It keeps the number of components currently
> >> in the buffer in pdepth, and removes components while pdepth is greater
> >> than the depth reported by fdt_next_node().
> >>
> >> fdt_next_node() reports that it has left the tree by taking the depth
> >> negative, but the offset it returns at that point is the offset of the
> >> end tag, which is not negative.  The loop only tests the offset, so for a  
> > fdt_next_node() returns an error (-FDT_ERR_NOTFOUND) when it reaches the
> > FDT_END tag [1].
> >
> > I don't understand your point. Maybe I've missed something.
> > Can you clarify?  
> Sorry, my wording was wrong.  I meant the FDT_END_NODE case, just above
> the one you linked:
> 
>      case FDT_END_NODE:
>          if (depth && ((--(*depth)) < 0))
>              return nextoffset;
> 
> Leaving the root node is an FDT_END_NODE tag, so --(*depth) takes the
> depth from 0 to -1 and fdt_next_node() returns nextoffset, which is not
> negative.  FDT_END is only reached on the next call.

Ok

> 
> >
> > [1] https://github.com/dgibson/dtc/blob/7a1e017926004ecff5fce62d62d42ce9f3e00082/libfdt/fdt.c#L283
> >  
> >> nodeoffset at or past the end tag it runs one more time with a negative
> >> depth.  pdepth still counts the components in the buffer, so the removal
> >> loop is asked to remove more components than are there: it takes the
> >> index down to zero and carries on scanning backwards for a '/' that is
> >> not in the buffer.
> >>
> >> Stop the walk when the depth shows it has left the tree.  Offsets that
> >> far along are rejected by the error paths below in any case, so this
> >> does not change what callers see.  
> > If fdt_next_node() can return an offset (positive value) which is not pointing
> > to a node without any errors (negative returned value), fdt_next_node() has
> > to be fixed not callers.
> >
> > Can you give details leading fdt_next_node() returning a valid offset which
> > doesn't point to a node?  
> The offset is the one past root's FDT_END_NODE tag, so on
> tests/test_tree1.dtb:
> 
>    offset   depth
>    540      2
>    560      -1    <- FDT_END_NODE: offset >= 0, depth < 0
>    -4       -1    <- FDT_ERR_BADOFFSET, from the next call
> 
> Does it make sense ?

It could but I still don't see where the issue is.

I mean, in the fdt_get_path() we have the following loop:

	for (offset = 0, depth = 0;
	     (offset >= 0) && (offset <= nodeoffset);
	     offset = fdt_next_node(fdt, offset, &depth)) {
		...
	}

fdt_next_node() returns an offset past the root's FDT_END_NODE tag.

In that case, how the 'offset <= nodeoffset' condition can be true in order
to continue the loop?

nodeoffset is an offset to a node. It is either the root node or a node
inside the root tree. And so this offset points to something before the
root's FDT_END_NODE tag offset.

Do I miss something?

Best regards,
Hervé

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

[David Gibson]

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

On Thu, Oct 01, 2026 at 01:10:36PM +0100, Ayan Kumar Halder wrote:
> fdt_get_path() walks the tree from the root, appending each node's name
> to the caller's buffer as it goes down and removing the last component
> again as it comes back up.  It keeps the number of components currently
> in the buffer in pdepth, and removes components while pdepth is greater
> than the depth reported by fdt_next_node().
> 
> fdt_next_node() reports that it has left the tree by taking the depth
> negative, but the offset it returns at that point is the offset of the
> end tag, which is not negative.  The loop only tests the offset, so for a
> nodeoffset at or past the end tag it runs one more time with a negative
> depth.  pdepth still counts the components in the buffer, so the removal
> loop is asked to remove more components than are there: it takes the
> index down to zero and carries on scanning backwards for a '/' that is
> not in the buffer.
> 
> Stop the walk when the depth shows it has left the tree.  Offsets that
> far along are rejected by the error paths below in any case, so this
> does not change what callers see.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>

I think this is a bug in fdt_next_node() rather than in
fdt_get_path().  It should not be returning offsets that are not valid
node offsets (excepting negative error codes, of course).

> ---
>  libfdt/fdt_ro.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index 11f2e2e..32689c7 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -582,7 +582,7 @@ int fdt_get_path(const void *fdt, int nodeoffset, char *buf, int buflen)
>  		return -FDT_ERR_NOSPACE;
>  
>  	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 {
> -- 
> 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 --]

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

[Simon Glass]

Hi,

On Tue, 6 Oct 2026 at 04:01, David Gibson <david@gibson.dropbear.id.au> wrote:
>
> On Thu, Oct 01, 2026 at 01:10:36PM +0100, Ayan Kumar Halder wrote:
> > fdt_get_path() walks the tree from the root, appending each node's name
> > to the caller's buffer as it goes down and removing the last component
> > again as it comes back up.  It keeps the number of components currently
> > in the buffer in pdepth, and removes components while pdepth is greater
> > than the depth reported by fdt_next_node().
> >
> > fdt_next_node() reports that it has left the tree by taking the depth
> > negative, but the offset it returns at that point is the offset of the
> > end tag, which is not negative.  The loop only tests the offset, so for a
> > nodeoffset at or past the end tag it runs one more time with a negative
> > depth.  pdepth still counts the components in the buffer, so the removal
> > loop is asked to remove more components than are there: it takes the
> > index down to zero and carries on scanning backwards for a '/' that is
> > not in the buffer.
> >
> > Stop the walk when the depth shows it has left the tree.  Offsets that
> > far along are rejected by the error paths below in any case, so this
> > does not change what callers see.
> >
> > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>
> I think this is a bug in fdt_next_node() rather than in
> fdt_get_path().  It should not be returning offsets that are not valid
> node offsets (excepting negative error codes, of course).

The bug is only with an invalid nodeoffset past the root's FDT_END_NODE

I don't think fdt_next_node() should change - fdt_node_end_offset_()
relies on it returning the offset after the FDT_END_NODE when depth
goes negative, and fdt_nop_node() and fdt_del_node() use that. For the
last child of a node, that offset is the parent's FDT_END_NODE, so it
is not a node offset.

One option would be to reject nodeoffset up front with fdt_check_node_offset_()

For the commit message, as you mention, use 'FDT_END_NODE' rather than
'end tag' and make it clear this needs a bad nodeoffset - also try to
explain the problem in the first paragraph.

Also please add a bad-offset case to tests/get_path.c

Regards,
Simon

>
> > ---
> >  libfdt/fdt_ro.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > index 11f2e2e..32689c7 100644
> > --- a/libfdt/fdt_ro.c
> > +++ b/libfdt/fdt_ro.c
> > @@ -582,7 +582,7 @@ int fdt_get_path(const void *fdt, int nodeoffset, char *buf, int buflen)
> >               return -FDT_ERR_NOSPACE;
> >
> >       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 {
> > --
> > 2.25.1
> >
> >

[..]

Regards,
SImon