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

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250922201709.GE1391379@nvidia.com>
Date: Mon, 22 Sep 2025 17:17:09 -0300
From: Jason Gunthorpe <jgg@...dia.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Danilo Krummrich <dakr@...nel.org>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	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>,
	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 Mon, Sep 22, 2025 at 08:42:23PM +0200, Greg Kroah-Hartman wrote:
> 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.

This is two struct devices, two device drivers and a fops.  There is
no reason to trigger revoke from the parent driver to a child driver,
that's just mis-layering and obfuscation.

It already subtly relies on the parent not triggering revoke until the
child is removed because it added this to the cros-ec-chardev driver:

@@ -166,7 +183,12 @@ static int cros_ec_chardev_open(struct inode *inode, struct file *filp)
	struct cros_ec_dev *ec_dev = dev_get_drvdata(mdev->parent);

 	if (!priv)
 		return -ENOMEM;

-	priv->ec_dev = ec_dev;
+	priv->ec_dev_rev = revocable_alloc(ec_dev->revocable_provider);

It would UAF if the revoke is triggered by the parent driver at the
wrong time.

So why is the parent involved at all? Why does it have to violate
modularity?

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

There are more complex situations, but this isn't one of them. All
this needs is to fence the file operations of a single cdev. I've
solved it enough times now to know exactly how simple this really is..

 > struct chardev_data {
> > 	struct device dev;
> > 	struct cdev cdev;
> 
> Woah, no, that is totally wrong.

Sigh. I'm sure we've had this exchange before. Please
re-read the documentation for cdev_device_add():

 * This function should be used whenever the struct cdev and the
 * struct device are members of the same structure whose lifetime is
 * managed by the struct device.    ^^^^^^^^^^^^

We created this helper specifically to clean up the refcount bugs
around cdev usage. It supports the above pattern which is the natural
and easy way to use cdev.

> 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 :)

It is true there are many abuses, but converting cdev users to use
cdev_device_add() is the right and best option IMHO.

miscdev is not a good option in cases like this because you don't get
a nice natural kref around the memory, among other limitations.

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

I strongly prefer Laurent's direction to add a helper file_operations
that automatically wraps all ops in a lock.

I think all this series does is create a weirdly named lock that
drivers still have to open code.

The fact the very first proposed user is already abusing it to
needlessly violate driver modularity is really depressing. Let's not
create another devm :\

Jason