Message-ID: <2025091209-curfew-safari-f6e0@gregkh>
Date: Fri, 12 Sep 2025 15:39:45 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: 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>, brgl@...ev.pl
Subject: Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

On Fri, Sep 12, 2025 at 04:26:56PM +0300, Laurent Pinchart wrote:
> On Fri, Sep 12, 2025 at 08:49:30PM +0800, Tzung-Bi Shih wrote:
> > On Fri, Sep 12, 2025 at 11:24:10AM +0200, Bartosz Golaszewski wrote:
> > > On Fri, 12 Sept 2025 at 11:09, Krzysztof Kozlowski wrote:
> > > > On 12/09/2025 10:17, Tzung-Bi Shih wrote:
> > > > > Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> > > > > Cc: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> > > > > Cc: Wolfram Sang <wsa+renesas@...g-engineering.com>
> > > >
> > > > Thanks for the work. Just a note, please start using b4, so above Cc
> > > > will be propagated to all patches. Folks above received only the cover
> > > > letter...
> > 
> > Thank you for bringing this to my attention.  I wasn't aware of that and
> > will ensure this is handled correctly in the future.
> > 
> > > Thanks to Krzysztof for making me aware of this. Could you please Cc
> > > my brgl@...ev.pl address on the next iteration.
> > 
> > Sure, will do.
> > 
> > > I haven't looked into the details yet but the small size of the first
> > > patch strikes me as odd. The similar changes I did for GPIO were quite
> > > big and they were designed just for a single sub-system.
> > > 
> > > During the talk you reference, after I suggested a library like this,
> > > Greg KH can be heard saying: do this for two big subsystems so that
> > > you're sure it's a generic solution. Here you're only using it in a
> > > single driver which makes me wonder if we can actually use it to
> > > improve bigger offenders, like for example I2C, or even replace the
> > > custom, SRCU-based solution in GPIO we have now. Have you considered
> > > at least doing a PoC in a wider kernel framework?
> > 
> > Yes, I'm happy to take this on.
> > 
> > To help me get started, could you please point me to some relevant code
> > locations?  Also, could you let me know if any specific physical devices
> > will be needed for testing?
> 
> One interesting test would be to move the logic to the cdev layer. The
> use-after-free problem isn't specific to one type of character device,
> and so shouldn't require a fix in every driver instantiating a cdev
> directly (or indirectly). See [1] for a previous attempt to handle this
> at the V4L2 level and [2] for an attempt to handle it at the cdev level.
> 
> In [1], two new functions named video_device_enter() and
> video_device_exit() flag the beginning and end of protected code
> sections. The equivalent in [2] is the manual get/put of cdev->qactive,
> and if I understand things correctly, your series creates a REVOCABLE()
> macro to do the same. I'm sure we'll bikesheed about names at some
> point, but for the time being, what I'd like to see if this being done
> in fs/char_dev.c to cover all entry points from userspace at the cdev
> level.
> 
> We then have video_device_unplug() in [1], which I think is more or less
> the equivalent of revocable_provider_free(). I don't think we'll be able
> to hide this completely from drivers, at least not in all cases. We
> should however design the API to make it easy for drivers, likely with
> subsystem-specific wrappers.
> 
> What I have in mind is roughly the following:
> 
> 1. Protect all access to the cdev from userspace with enter/exit calls
>    that flag if a call is in progress. This can be done with explicit
>    function calls, or with a scope guard as in your series.
> 
> 2. At .remove() time, start by flagging that the device is being
>    removed. That has to be an explicit call from drivers I believe,
>    likely using subsystem-specific wrappers to simplify things.
> 
> 3. Once the device is marked as being removed, all enter() calls should
>    fail at the cdev level.
> 
> 4. In .remove(), proceed to perform driver-specific operations that will
>    stop the device and wake up any userspace task blocked on a syscall
>    protected by enter()/remove(). This isn't needed for
>    drivers/subsystems that don't provide any blocking API, but is
>    required otherwise.
> 
> 5. Unregister, still in .remove(), the cdev (likely through
>    subsystem-specific APIs in most cases). This should block until all
>    protected sections have exited.
> 
> 6. The cdev is now unregistered, can't be opened anymore, and any
>    new syscall on any opened file handle will return an error. The
>    driver's .remove() function can proceed to free data, there won't be
>    any UAF caused by userspace.
> 
> [1] implemented this fairly naively with flags and spinlocks. An
> RCU-based implementation is probably more efficient, even if I don't
> know how performance-sensitive all this is.
> 
> Does this align with your design, and do you think you could give a try
> at pushing revocable resource handling to the cdev level ?
> 
> On a separate note, I'm not sure "revocable" is the right name here. I
> believe a revocable resource API is needed, and well-named, for
> in-kernel consumers (e.g. drivers consuming a GPIO or clock). For the
> userspace syscalls racing with .remove(), I don't think we're dealing
> with "revocable resources". Now, if a "revocable resources" API were to
> support the in-kernel users, and be usable as a building block to fix
> the cdev issue, I would have nothing against it, but the "revocable"
> name should be internal in that case, used in the cdev layer only, and
> not exposed to drivers (or even subsystem helpers that should wrap cdev
> functions instead).

I think the name makes sense as it matches up with how things are
working (the backend structure is "revoked"), but naming is tough :)

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.

And that's fine, we have "interns" that we can set loose on this type of
code conversions, I think we just need to wrap the access to the cdev
with this api, which will take a bit of rewriting in many drivers.

Anyway, just my thought, if someone else can see how this could drop
into the core cdev code without any changes needed, that would be great,
but I don't see it at the moment.  cdev is just too "raw" for that.

thanks,

greg k-h

Powered by blists - more mailing lists