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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Jul 16, 2026 at 04:52:27PM +0800, Chen-Yu Tsai wrote:
> On Wed, Jul 15, 2026 at 5:09 PM <sashiko-bot@kernel.org> 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.

> > [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!):
20260611203537.1786399-1-andriy.shevchenko@linux.intel.com

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

> 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