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
