Message-ID: <20250912145312.GK31682@pendragon.ideasonboard.com>
Date: Fri, 12 Sep 2025 17:53:12 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Bartosz Golaszewski <brgl@...ev.pl>, Tzung-Bi Shih <tzungbi@...nel.org>,
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>,
	Krzysztof Kozlowski <krzk@...nel.org>,
	Benson Leung <bleung@...omium.org>,
	"Rafael J . Wysocki" <rafael@...nel.org>,
	Danilo Krummrich <dakr@...nel.org>,
	Jonathan Corbet <corbet@....net>, Shuah Khan <shuah@...nel.org>,
	Dawid Niedzwiecki <dawidn@...gle.com>, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, chrome-platform@...ts.linux.dev,
	linux-kselftest@...r.kernel.org,
	Wolfram Sang <wsa+renesas@...g-engineering.com>,
	Dan Williams <dan.j.williams@...el.com>
Subject: Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

On Fri, Sep 12, 2025 at 04:40:27PM +0200, Greg KH wrote:
> On Fri, Sep 12, 2025 at 05:26:46PM +0300, Laurent Pinchart wrote:
> > (CC'ing Dan Williams)
> > 
> > On Fri, Sep 12, 2025 at 04:19:53PM +0200, Greg KH wrote:
> > > On Fri, Sep 12, 2025 at 04:59:16PM +0300, Laurent Pinchart wrote:
> > > > On Fri, Sep 12, 2025 at 03:46:27PM +0200, Bartosz Golaszewski wrote:
> > > > > On Fri, Sep 12, 2025 at 3:39 PM Greg Kroah-Hartman wrote:
> > > > > >
> > > > > > I have no objection moving this to the cdev api, BUT given that 'struct
> > > > > > cdev' is embedded everywhere, I don't think it's going to be a simple
> > > > > > task, but rather have to be done one-driver-at-a-time like the patch in
> > > > > > this series does it.
> > > > > 
> > > > > I don't think cdev is the right place for this as user-space keeping a
> > > > > reference to a file-descriptor whose "backend" disappeared is not the
> > > > > only possible problem. We can easily create a use-case of a USB I2C
> > > > > expander being used by some in-kernel consumer and then unplugged.
> > > > > This has nothing to do with the character device. I believe the
> > > > > sub-system level is the right place for this and every driver
> > > > > subsystem would have to integrate it separately, taking its various
> > > > > quirks into account.
> > > > 
> > > > That's why I mentioned in-kernel users previously. Drivers routinely
> > > > acquire resources provided by other drivers, and having a way to revoke
> > > > those is needed.
> > > > 
> > > > It is a different but related problem compared to userspace racing with
> > > > .remove(). Could we solve both using the same backend concepts ?
> > > > Perhaps, time will tell, and if that works nicely, great. But we still
> > > > have lots of drivers exposing character devices to userspace (usually
> > > > through a subsystem-specific API, drivers that create a cdev manually
> > > > are the minority). That problem is in my opinion more urgent than
> > > > handling the removal of in-kernel resources, because it's more common,
> > > > and is easily triggerable by userspace. The good news is that it should
> > > > also be simpler to solve, we should be able to address the enter/exit
> > > > part entirely in cdev, and limit the changes to drivers in .remove() to
> > > > the strict minimum.
> > > > 
> > > > What I'd like to see is if the proposed implementation of revocable
> > > > resources can be used as a building block to fix the cdev issue. If it
> > > > ca, great, let's solve it then. If it can't, that's still fine, it will
> > > > still be useful for in-kernel resources, even if we need a different
> > > > implementation for cdev.
> > > 
> > > Patch 5/5 in this series does just this for a specific use of a cdev in
> > > the driver.  Is that what you are looking for?
> > 
> > Not quite, I would like to see the enter/exit (aka revocable scope if my
> > understanding is correct) being pushed to char_dev.c, as Dan did in [1].
> > I'm fine having to add an extra function call in the .remove() path of
> > drivers, but I'm not fine with having to mark revocable sections
> > manually in drivers. That part belongs to cdev.
> > 
> > [1] https://lore.kernel.org/r/161117153248.2853729.2452425259045172318.stgit@dwillia2-desk3.amr.corp.intel.com
> 
> Dan's proposal here is a good start, but the "sleep in cdev_del() until
> the device drains all existing opens" is going to not really work well
> for what we want.

I think you missed the fact that the code doesn't wait for all open file
handles to be closed. It waits for all in-progress syscalls to return
from the driver. That's a big difference.

> So sure, make a new cdev api to use this, that's fine, then we will have
> what, 5 different ways to use a cdev?  :)
> 
> Seriously, that would be good, then we can work to convert things over,
> but I think overall it will look much the same as what patch 5/5 does
> here.  But details matter, I don't really known for sure...

What I don't want to see is drivers using this new API directly to
protect the cdev race. That would be a big step in the wrong direction.
Patch 5/5 needs to move driver code to the cdev level. That shouldn't be
difficult, so I really not see why it can't be done in v4 to see how it
will look like.

> Either way, I think this patch series stands on its own, it doesn't
> require cdev to implement it, drivers can use it to wrap a cdev if they
> want to.

No, drivers should *not* do that manually. That's a recipe for disaster
that we would regret later.

> We have other structures that want to do this type of thing
> today as is proof with the rust implementation for the devm api.

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists