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

[Herve Codina <herve.codina@bootlin.com>]

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é