Re: [PATCH v3 02/13] device property: Add fwnode_graph_get_next_port_endpoint()

[sashiko-bot@kernel.org]

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

New issues:
- [High] fwnode_graph_get_next_port_endpoint() breaks graph abstraction by hardcoding name matching, which fails for ACPI nodes.

Pre-existing issues:
- [High] Preexisting bug in fwnode_get_next_child_node() causing Use-After-Free and infinite loops when crossing from primary to secondary fwnodes.
--

commit 94e97ca138e57ff4e7c6032a05de92b5aec2bd3d
Author: Chen-Yu Tsai <wenst@chromium.org>

device property: Add fwnode_graph_get_next_port_endpoint()

This introduces a helper function to get the endpoint under a known port
firmware node. It is intended to be used in USB power sequencing to check
whether a graph connection exists and if the other side is a supported
connector type.

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 is a pre-existing issue, but does calling fwnode_get_next_child_node()
here expose a use-after-free and infinite loop when the parent has both
primary and secondary fwnodes?

When the primary iterator completes, it drops its reference to the child
node and returns NULL. The fwnode_get_next_child_node() function then passes
this already freed primary child directly to the secondary iterator:

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);
}

Would the secondary iterator drop the reference again and cause a double
free? 

Could this also create an infinite loop where the secondary iterator treats
the invalid node as missing, returns its first child, and passes it back to
the primary iterator on the next loop iteration?

> +		if (fwnode_name_eq(prev, "endpoint"))

[Severity: High]
Does this hardcoded string match cause a regression for ACPI-based devices
relying on this API?

ACPI graph endpoint nodes are natively identified by the endpoint property,
and their names often do not directly match "endpoint" (as seen in
is_acpi_graph_node).

By bypassing the graph fwnode abstractions like graph_get_next_endpoint,
will this silently fail to find graph endpoints on ACPI platforms?

> +			break;
> +	} while (prev);
> +
> +	return prev;
> +}
> +EXPORT_SYMBOL_GPL(fwnode_graph_get_next_port_endpoint);

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