linux-kernel - Re: [PATCH 2/2] device property: fix UAF in device_get_next_child

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z1cy3YuFXY0unhsY@smile.fi.intel.com>
Date: Mon, 9 Dec 2024 20:11:41 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J . Wysocki" <rafael.j.wysocki@...el.com>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Daniel Scally <djrscally@...il.com>, linux-acpi@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] device property: fix UAF in
 device_get_next_child_node()

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