Re: [PATCH v7 02/16] device property: Add fwnode_graph_get_next_port_endpoint()

[sashiko-bot@kernel.org]

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

New issues:
- [High] The new helper `fwnode_graph_get_next_port_endpoint` ignores the ACPI-specific compatibility logic for identifying endpoint nodes by hardcoding an OF-specific naming convention.

Pre-existing issues:
- [Critical] `fwnode_get_next_child_node` improperly handles transitioning from the primary fwnode to the secondary fwnode, causing double frees, reference leaks, and an infinite loop. This problem wasn't introduced by this patch, but it is a critical pre-existing issue exposed by new loops.
- [High] Calling `mutex_lock(&fwnode_link_lock)` while holding `raw_spin_lock_irqsave(&devtree_lock)` causes a sleeping in atomic context BUG when the last reference to a dynamic OF node is dropped during iteration. This problem wasn't introduced by this patch, but it is a severe pre-existing locking violation.
--

--- Patch [2]: [PATCH v7 02/16] device property: Add fwnode_graph_get_next_port_endpoint() ---
commit 0be52c3289b5692b190b7583fe9b05fd00691fca
Author: Chen-Yu Tsai <wenst@chromium.org>

device property: Add fwnode_graph_get_next_port_endpoint()

Due to design constraints of the power sequencing API, the consumer must first
be sure that the other side is actually a provider, or it will continually get
-EPROBE_DEFER when requesting the power sequencing descriptor. This commit
introduces a helper to get the endpoint under a known port firmware node.

> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 0bd45f8b01c14..62c6221d65d05 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: Critical]
This is a pre-existing issue, but does fwnode_get_next_child_node() improperly
handle transitioning from the primary fwnode to the secondary fwnode?

When the primary fwnode iterator exhausts its children, it drops the reference
to the child node and returns NULL.

Inside fwnode_get_next_child_node(), it then passes this already-freed child
pointer to the secondary fwnode's iterator:

drivers/base/property.c:fwnode_get_next_child_node() {
    ...
    /* Try to find a child in primary fwnode */
    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);
}

Checking is_software_node(child) inside the secondary iterator accesses the
freed node's ops, causing a UAF. Since child is not a software node, the
iterator returns the first software node child. On the subsequent iteration,
this software node is passed back into the primary OF iterator, which ignores
it and restarts from the first OF child, resulting in an infinite loop.

Can this be triggered by any device graph traversal on a parent node possessing
both primary and secondary children?


[Severity: High]
This is also a pre-existing issue, but is there a risk of a sleeping in atomic
context BUG here when dropping the final reference to a dynamic OF node?

Iterator functions within the OF subsystem (like of_get_next_status_child) drop
the reference to the prev fwnode using of_node_put() while explicitly holding
the devtree_lock raw spinlock.

If the node being put is dynamically allocated (e.g., from DT overlays) and this
is its final reference, of_node_put() synchronously invokes of_node_release().

This release function then calls fwnode_links_purge(), which attempts to
acquire the fwnode_link_lock mutex:

drivers/base/core.c:fwnode_links_purge_suppliers() {
    ...
    guard(mutex)(&fwnode_link_lock);
    ...
}

Will this lock nesting hierarchy violation trigger a BUG on both RT and non-RT
kernels when terminating the loop on the final reference of a dynamically
allocated node?

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

[Severity: High]
Does this strict naming check ignore the ACPI-specific compatibility logic for
identifying endpoint nodes?

In the ACPI subsystem, graph nodes are often identified by the presence of an
endpoint property rather than their exact node name (e.g., as handled by
is_acpi_graph_node()).

If drivers using this generic fwnode_graph_* API on ACPI platforms don't have
the literal name "endpoint", will they fail to find valid endpoints and break
device functionality?

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

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