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

On Fri, Jul 17, 2026 at 5:04 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Thu, Jul 16, 2026 at 04:52:27PM +0800, Chen-Yu Tsai wrote:
> > On Wed, Jul 15, 2026 at 5:09 PM <[email protected]> wrote:
>
> ...
>
> > > > +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 iterating over endpoints using
> > > fwnode_get_next_child_node() can cause an infinite loop and a use-after-free
> > > if a device has secondary fwnodes attached.
> > >
> > > If a device port has both primary children and a secondary fwnode, the primary
> > > ops finish and drop the reference to the last primary child, returning NULL.
> > > Then, fwnode_get_next_child_node() incorrectly passes this same dropped child
> > > pointer to the secondary ops:
> > >
> > > 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);
> > > }
> > >
> > > This can result in a double-put. Furthermore, when traversing secondary
> > > children, the primary ops don't recognize the software node, treat it as NULL,
> > > and return the first primary child again, creating an infinite loop.
>
> Yeah, it has to be implemented in a way how fwnode_graph_get_next_endpoint() is done.

If your patch series addresses the issue, I guess we can consider it
covered? Sashiko labeled this a pre-existing.

> > > [Severity: Medium]
> > > Does this fail to unconditionally put the prev reference if the provided port
> > > is invalid?
> > >
> > > If a caller passes a valid prev node but port is NULL or an error pointer,
> > > fwnode_get_next_child_node() returns early without dropping prev:
> > >
> > > drivers/base/property.c:fwnode_get_next_child_node() {
> > >         struct fwnode_handle *next;
> > >
> > >         if (IS_ERR_OR_NULL(fwnode))
> > >                 return NULL;
> > >     ...
> > > }
> > >
> > > Could this lead to a reference count leak, violating the documented API
> > > contract that prev is unconditionally put?
> >
> > This seems like what the other fwnode_get_next_*() functions are doing.
> > I'd say this is a pre-existing issue as well.
>
> See this series (not yet applied, you even have chance to review it!):
> [email protected]

I believe that series addresses the first issue, not this one, even
though it partially mitigates it.

The contract says "puts a reference to @prev unconditionally." What it
doesn't say is that it requires @node to be valid, i.e. the function
is a no-op if node == NULL.

"node != NULL" being an implicit requirement seems to be the norm for
the fwnode_* and of_* APIs. It's not explicitly spelled out and Sashiko
lacks the context for it.

I'm not sure this should be changed within this patch. It would end up
as a mismatch between the various functions and possibly lead to more
confusion.

> > > > +             if (fwnode_name_eq(prev, "endpoint"))
> > >
> > > [Severity: High]
> > > Does hardcoding this string match break the fwnode graph abstraction for
> > > non-DeviceTree platforms?
> > >
> > > By strictly matching the "endpoint" prefix, this bypasses the provider-specific
> > > fwnode_operations, which could silently ignore valid ACPI and software node
> > > endpoints that don't follow this exact naming scheme. Shouldn't this rely on
> > > the backend-specific graph_get_next_endpoint operations instead?
> >
> > From drivers/acpi/property.c it seems that ACPI graphs follow the same
> > structure. I don't have visibility into ACPI implementations though.
>
> Sashiko might be right. ACPI has device and data nodes, for device nodes the
> name will be FourCC, so never longer than 4 characters. For data nodes, it
> takes their names, which are arbitrary strings and seems should follow the given
> schema. You need Sakari Ailus to review this patch.

OK. Will add Sakari in the next version.


ChenYu



> > We also have the following in include/linux/fwnode.h:
> >
> > #define SWNODE_GRAPH_PORT_NAME_FMT              "port@%u"
> > #define SWNODE_GRAPH_ENDPOINT_NAME_FMT          "endpoint@%u"
> >
> > So this should not be a problem.
> >
> > > > +                     break;
> > > > +     } while (prev);
> > > > +
> > > > +     return prev;
> > > > +}
>
> --
> With Best Regards,
> Andy Shevchenko
>
>