Re: [PATCH 2/2] device property: fix UAF in device_get_next_child_node()

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

On Thu, Nov 28, 2024 at 03:16:04PM -0800, Dmitry Torokhov wrote:
> On Thu, Nov 28, 2024 at 03:20:09PM +0200, Andy Shevchenko wrote:
> > On Wed, Nov 27, 2024 at 09:39:35PM -0800, Dmitry Torokhov wrote:
> > > fwnode_get_next_child_node() always drops reference to the node passed
> > > as the "child" argument,
> > 
> > As commented previously,it might be just a documentation bug. So, please
> 
> No, absolutely not. Consider calling device_get_next_child_node() with
> "child" pointing to the last child of the primary fwnode.
> fwnode_get_next_child_node() will drop the reference to "child"
> rendering it unusable, and return NULL. The code will go and call
> fwnode_get_next_child_node(fwnode->secondary, child) with invalid child
> pointer, resulting in UAF condition.
> 
> Also, consider what happens next. Let's say we did not crash and instead
> returned first child of the secondary node (let's assume primary is an
> OF node and secondary is a software node). On the next iteration of
> device_get_next_child_node() we will call
> fwnode_get_next_child_node(fwnode, child) which results in passing
> swnode child to of_fwnode_get_next_child_node() which may or may not
> work. It all is very fragile.

It seems some of OF calls will crash on this.

> That is why it is best to check if the child argument is indeed a child
> to a given parent before calling fwnode_get_next_child_node() on them.

> > elaborate on the use case before this patch that leads to an issue.
> > 
> > > which makes "child" pointer no longer valid
> > > and we can not use it to scan the secondary node in case there are no
> > > more children in primary one.
> > > 
> > > Also, it is not obvious whether it is safe to pass children of the
> > > secondary node to fwnode_get_next_child_node() called on the primary
> > > node in subsequent calls to device_get_next_child_node().
> > > 
> > > Fix the issue by checking whether the child node passed in is indeed a
> > > child of primary or secondary node, and do not call
> > > fwnode_get_next_child_node() for the wrong parent node. Also set the
> > > "child" to NULL after unsuccessful call to fwnode_get_next_child_node()
> > > on primary node to make sure secondary node's children are scanned from
> > > the beginning.
> > 
> > To me it sounds over complicated. Why not just take reference to the child once
> > more and put it after we find next in either cases?
> 
> You want to "reset" when switching from primary node over to secondary
> instead of hoping that passing child pointer which is not really a child
> to secondary node will somehow cause fwnode_get_next_child_node() to
> return first its child.

At some point we need to switch from primary to a secondary, yes, we need
to "reset" it somehow to switch the type.

> > Current solution provides
> > a lot of duplication and makes function less understandable.
> 
> The simplicity of the old version is deceiving. See the explanation
> above.


-- 
With Best Regards,
Andy Shevchenko