Re: [PATCH v4 02/14] device property: Add fwnode_graph_get_next_port_endpoint()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the core `fwnode_get_next_child_node()` API has a severe pre-existing bug when iterating over a node with both primary and secondary fwnodes, leading to a double-free/Use-After-Free and an infinite loop.
--

--- Patch [2]: [PATCH v4 02/14] device property: Add fwnode_graph_get_next_port_endpoint() ---
commit 1373ba83196f2e29826248ffdc2c43881f3a22fa
Author: Chen-Yu Tsai <wenst@chromium.org>

device property: Add fwnode_graph_get_next_port_endpoint()

This commit introduces a new helper function to iterate over endpoint
firmware nodes under a specific port node. It will be used by the
upcoming USB power sequencing integration to verify graph connections
and supported connector types.

> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index c5e7e00f1b0cd..7f1fff3b55c74 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c

[ ... ]

> @@ -1099,6 +1099,31 @@ int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name)
>  }
>  EXPORT_SYMBOL(fwnode_irq_get_byname);
>  
> +/**
> + * fwnode_graph_get_next_port_endpoint - Get next endpoint firmware node in port
> + * @port: Pointer to the target port firmware node
> + * @prev: Previous endpoint node or %NULL to get the first
> + *
> + * The caller is responsible for calling fwnode_handle_put() on the returned
> + * fwnode pointer. Note that this function also puts a reference to @prev
> + * unconditionally.
> + *
> + * Return: an endpoint firmware node pointer or %NULL if no more endpoints
> + * are available.
> + */
> +struct fwnode_handle *fwnode_graph_get_next_port_endpoint(const struct fwnode_handle *port,
> +							  struct fwnode_handle *prev)
> +{
> +	do {
> +		prev = fwnode_get_next_child_node(port, prev);

[Severity: High]
This isn't a bug introduced by this patch, but the core fwnode iteration 
API has a pre-existing double-free and infinite loop issue when used on 
nodes with both primary and secondary fwnodes.

When the primary fwnode's get_next_child_node operation reaches the end,
it drops the reference to the last child and returns NULL. The core API 
fwnode_get_next_child_node() then passes this already-put child pointer 
to the secondary ops:

drivers/base/property.c:fwnode_get_next_child_node() {
    ...
    next = fwnode_call_ptr_op(fwnode, get_next_child_node, child);
    if (next)
        return next;

    /* When no more children in primary, continue with secondary */
    return fwnode_call_ptr_op(fwnode->secondary, get_next_child_node, child);
}

This causes secondary ops like software_node_get_next_child() to call 
fwnode_handle_put(child) a second time on the already dropped node, 
resulting in a use-after-free or double free.

Furthermore, the secondary op returns its first child. On the next iteration
of this do/while loop, this secondary child is passed back to the primary OF 
op. Since to_of_node() on a secondary node evaluates to NULL, the OF iterator 
resets to its first child, creating an infinite loop alternating between 
primary and secondary children.

Is it safe for fwnode_graph_get_next_port_endpoint() to rely on this 
iterator before the underlying issue in fwnode_get_next_child_node() 
is resolved?

> +		if (fwnode_name_eq(prev, "endpoint"))
> +			break;
> +	} while (prev);
> +
> +	return prev;
> +}
> +EXPORT_SYMBOL_GPL(fwnode_graph_get_next_port_endpoint);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709095726.704448-1-wenst@chromium.org?part=2