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: <20250922174010.GC1391379@nvidia.com>
Date: Mon, 22 Sep 2025 14:40:10 -0300
From: Jason Gunthorpe <jgg@...dia.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	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 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!

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;

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.

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.

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

Jason