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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, Sep 22, 2025 at 02:40:10PM -0300, Jason Gunthorpe wrote:
> On Mon, Sep 22, 2025 at 05:55:43PM +0200, Danilo Krummrich wrote:
> > I fully agree with that, in C there is indeed no value of a revocable type when
> > subsystems can guarantee "sane unregistration semantics".
> 
> Indeed, I agree with your message. If I look at the ec_cros presented
> here, there is no reason for any revocable. In fact it already seems
> like an abuse of the idea.
> 
> The cros_ec.c spawns a MFD with a platform_device of "cros-ec-chardev"
> that is alreay properly lifetime controlled - the platform is already
> removed during the remove of the cros_ec.c.
> 
> So, there is no need for a revocable that spans two drivers like this!

It's a very common pattern, you have a struct device, and a userspace
access, and need to "split" them apart, as you say below.  This logic
here handles that pattern.

See the many talks about this in the past at Plumbers and other
conferences, this isn't a new thing, and it isn't quite as simple as I
think you are making it out to be to solve.

> The bug is that cros_ec_chardev.c doesn't implement itself correctly
> and doesn't have a well designed remove() for something that creates a
> char dev. This issue should be entirely handled within
> cros_ec_chardev.c and not leak out to other files.
> 
> 1) Using a miscdevice means loosing out on any refcount managed
> memory. When using a file you need some per-device memory that lives
> for as long as all files are open. So don't use miscdev, the better
> pattern is:
> 
> struct chardev_data {
> 	struct device dev;
> 	struct cdev cdev;

Woah, no, that is totally wrong.

> Now you get to have a struct device linked refcount and a free
> function. The file can savely hold onto a chardev_data for its
> lifetime.

You have 2 different reference counts for the same structure.  A bug
that should never be done (yes, it's done a lot in the kernel, it's
wrong.)

And really, let's not abuse cdev anymore than it currently is please.
There's a reason that miscdevice returns just a pointer.  Right now cdev
can be used in 3-4 different ways, let's not come up with another way to
abuse that api :)

> 2) Add locking so the file can exist after the driver is removed:
> 
> struct chardev_data {
> [..]
> 	struct rw_semaphore sem;
> 	struct cros_ec_dev *ec_dev;
> };
> 
> Refcount the chardev_data::dev in the file operations open/release,
> refer to it via the private_data.
> 
> 3) At the start of every fop take the sem and check the ec_dev:
> 
> ACQUIRE(rwsem_read, ecdev_sem)(&data->sem);
> if (ACQUIRE_ERR(ecdev_sem) || !data->ec_dev)
>    return -ENODEV;
> 
> 4) After unregistering the cdev, but before destroying the device take
> the write side of the rwsem and NULL ec_dev.
> 
> 5) Purge all the devm usage from cros_ec_chardev, the only allocation
> is refcounted instead.
> 
> Simple. No need for any synchronize_srcu() for such a simple
> non-performance oriented driver.

There is no performance issue here, but there is lifetime rule logic
here that I really really do not want to have to "open code" for every
user.  revokable gives this to us in a simple way that we can "know" is
being used correctly.

And again, you can't have a single structure with two reference counts,
that's not allowed :)

> Yes, this can be made better, there is a bit too much boilerplate, but
> revocable is not the way for cros_ec.

I strongly disagree, sorry.

greg k-h