Thread (40 messages) 40 messages, 11 authors, 2025-09-23

Re: [PATCH v3 0/5] platform/chrome: Fix a possible UAF via revocable

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Date: 2025-09-13 16:14:41
Also in: chrome-platform, linux-kselftest, lkml

On Sat, Sep 13, 2025 at 11:55:45PM +0800, Tzung-Bi Shih wrote:
On Fri, Sep 12, 2025 at 05:54:16PM +0300, Laurent Pinchart wrote:
quoted
On Fri, Sep 12, 2025 at 04:44:56PM +0200, Bartosz Golaszewski wrote:
quoted
On Fri, Sep 12, 2025 at 4:40 PM Greg Kroah-Hartman wrote:
quoted
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.

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...

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.  We have other structures that want to do this type of thing
today as is proof with the rust implementation for the devm api.
Yeah, I'm not against this going upstream. If more development is
needed for this to be usable in other parts of the kernel, that can be
done gradually. Literally no subsystem ever was perfect on day 1.
To be clear, I'm not against the API being merged for the use cases that
would benefit from it, but I don't want to see drivers using it to
protect from the cdev/unregistration race.
Based on the discussion thread, my main takeaways are:

- Current `revocable` is considered a low level API.  We shouldn't (and
  likely can't) stop drivers, like the one in patch 5/5 in the series,
  from using it directly to fix UAFs.
Why shouldn't we ? We have enough precedents where driver authors rushed
to adopt brand new APIs without understand the implications.
devm_kzalloc() is a prime example of a small new API that very quickly
got misused everywhere. If we had taken the time to clearly explain when
it should be used and when it should *not* be used, we wouldn't be
plagued by as many device removal race conditions today. Let's not
repeat the same mistake, I'd like this new API to make things better,
not worse.
- Subsystems (like cdev) should build on this API to provide an easier
  interface for their drivers to manage revocable resources.

I'll create a PoC based on this.
I'm looking forward to that. Please let me know if there's anything you
would like to discuss. I didn't dive deep in technical details in this
thread, and I don't expect anyone to guess what I have in mind if I
failed to express it :-) I'm very confident the cdev race condition can
be fixed in a neat way, so let's do that.
quoted
quoted
Tzung-Bi: I'm not sure if you did submit anything but I'd love to see
this discussed during Linux Plumbers in Tokyo, it's the perfect fit
for the kernel summit.
Yes, and I just realized that in addition to the website submission, a
separate email is also required (or at least encouraged).  I've just sent
that email and am hoping it's not too late.
-- 
Regards,

Laurent Pinchart
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help