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

[David Gibson <david@gibson.dropbear.id.au>]

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

On Thu, Oct 08, 2026 at 01:15:44PM +0100, Simon Glass wrote:
> 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.

Huh, good catch.  Looks like I deliberately changed it to be that way
in ce4d9c0f ("libfdt: Rework fdt_next_node()"), but I'd forgotten.

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

I'm not entirely sure where you're suggesting putting this.  I think
the original proposed fix is correct, although having an additional
offset test for robustness might not be a bad idea as well.

> 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

Right.  I now think the fix itself is good, but some changes around it
are desirable:
  * Correct the commit message as indicated
  * Update the commit message to explicitly reference commit ce4d9c0f,
    which explains why fdt_next_node() works the way it does
  * Add a test case

-- 
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 --]