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

["Halder, Ayan Kumar" <ayankuma@amd.com>]

Hi David/all,

Apologies for the long delay.

On 14/10/2026 06:40, David Gibson wrote:
> 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.
I have added it at the top of fdt_get_path(), after the buflen check:

     if (!can_assume(VALID_INPUT)
         && ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0))
         return err;

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

I have fixed all the three in v2. Please see "[PATCH v2] libfdt: Fix 
fdt_get_path() reading before the start of the buffer".

- Ayan

>