* Re: [PATCH] platform/chrome: cros_hps_i2c: hold the client device for an open file
2026-11-07 18:53 [PATCH] platform/chrome: cros_hps_i2c: hold the client device for an open file Chad Aldreda
@ 2026-11-10 9:58 ` Tzung-Bi Shih
0 siblings, 0 replies; 2+ messages in thread
From: Tzung-Bi Shih @ 2026-11-10 9:58 UTC (permalink / raw)
To: Chad Aldreda
Cc: Dan Callaghan, Benson Leung, Sami Kyöstilä,
chrome-platform, stable, linux-kernel, Greg Kroah-Hartman,
Rafael J. Wysocki, Danilo Krummrich, Johan Hovold,
Bartosz Golaszewski
On Sat, Nov 07, 2026 at 06:53:05PM +0000, Chad Aldreda wrote:
> hps_release() finds struct hps_drvdata through file->private_data and
> calls pm_runtime_put() on hps->client->dev. hps comes from
> devm_kzalloc(), so it is freed when the driver is unbound, and a later
> close() of a file opened before the unbind reads freed memory. If the
> client was removed with delete_device, the i2c_client may have been
> freed as well, and pm_runtime_put() runs on it. KASAN reports a
> slab-use-after-free in hps_release() and, after delete_device, further
> ones in the runtime PM code.
>
> release() needs only the client device, so take a reference to it in
> open() and keep it in file->private_data, and leave hps alone in
> release(). hps is still valid in open(): misc_open() calls it with
> misc_mtx held, and hps_i2c_remove() calls misc_deregister(), which takes
> misc_mtx, before devres frees hps. Unless the driver has been bound
> again, runtime PM on the device is disabled by the time such a release()
> runs, so pm_runtime_put() drops the usage count and its idle request
> fails with -EACCES.
>
> Fixes: 5f9952548d91 ("platform/chrome: add a driver for HPS")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Chad Aldreda <ca@modelmiser.com>
> ---
> Found with the coccinelle script posted at
> https://lore.kernel.org/r/20261106193517.3267013-2-ca@modelmiser.com
>
> Reproduced in QEMU 10.2.4 (q35, KASAN). The driver binds to a client
> created with new_device on the i801 SMBus adapter, with the "enable"
> GPIO supplied by gpio-mockup through a test-only lookup module. A
> process opens /dev/cros-hps, the client is unbound or deleted through
> sysfs, and the process then closes the file. Without this patch KASAN
> reports one use-after-free after an unbind and six after delete_device;
> with it there are none, and an unbind followed by a bind works. Not
> tested on HPS hardware. Unbinding or deleting the client needs root.
>
> Unchanged by this patch: if the driver is bound again while a file
> opened before the unbind is still open, that file's pm_runtime_put()
> lands on the new instance's usage count, so the sensor stays powered
> until the old file is closed. The counts measured the same with and
> without the patch (2, then 1, then 0); before it, that put went through
> freed memory.
>
> For stable: before v7.0, hps_release() ends in
> "return pm_runtime_put(dev);". There it should end:
>
> pm_runtime_put(dev);
> put_device(dev);
>
> return 0;
>
> This is independent of the revocable work for cros_ec_chardev; it only
> stops release() from using the driver data.
>
> drivers/platform/chrome/cros_hps_i2c.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_hps_i2c.c b/drivers/platform/chrome/cros_hps_i2c.c
> index 3b9485627831..290b136ce4fa 100644
> --- a/drivers/platform/chrome/cros_hps_i2c.c
> +++ b/drivers/platform/chrome/cros_hps_i2c.c
> @@ -36,17 +36,26 @@ static int hps_open(struct inode *inode, struct file *file)
> struct hps_drvdata *hps = container_of(file->private_data,
> struct hps_drvdata, misc_device);
> struct device *dev = &hps->client->dev;
> + int ret;
> +
> + ret = pm_runtime_resume_and_get(dev);
> + if (ret)
> + return ret;
>
> - return pm_runtime_resume_and_get(dev);
> + /*
> + * hps is freed when the driver is unbound, which can happen before
> + * release(), so release() uses only the device, held here.
> + */
> + file->private_data = get_device(dev);
> + return 0;
> }
>
> static int hps_release(struct inode *inode, struct file *file)
> {
> - struct hps_drvdata *hps = container_of(file->private_data,
> - struct hps_drvdata, misc_device);
> - struct device *dev = &hps->client->dev;
> + struct device *dev = file->private_data;
>
> pm_runtime_put(dev);
> + put_device(dev);
>
> return 0;
> }
Thank you for looking into this and sharing the coccinelle script.
I don't think holding the reference of &hps->client->dev for the fops
lifecycle is the right direction:
1. Once the driver is unbound in hps_i2c_remove(), any still-open file
descriptors from that binding should become non-functional. Continuing
holding the runtime PM reference just for preventing the UAF makes less
sense.
2. As you noted under "---", it bleeds into the new instance's usage_count
if the driver is bound again before the old file is closed.
3. Instead, hps_i2c_remove() should clean up any remaining runtime PM
references before calling pm_runtime_disable(), and a late
hps_release() on a file opened before unbind should simply be a
no-op.
For an immediate fix, I'm wondering if we should use a similar pattern to
what you used (devm_kzalloc -> kzalloc + kref + removed flag) for the other
issues discovered by the coccinelle script:
struct hps_drvdata {
...
atomic_t open_count;
struct kref kref;
struct rw_semaphore rwsem;
bool removed;
};
static int hps_open(struct inode *inode, struct file *file)
{
...
atomic_inc(&hps->open_count);
kref_get(&hps->kref);
...
}
static int hps_release(struct inode *inode, struct file *file)
{
...
scoped_guard(rwsem_read, &hps->rwsem) {
if (!hps->removed) {
pm_runtime_put(&hps->client->dev);
atomic_dec(&hps->open_count);
}
}
kref_put(&hps->kref, ...);
...
}
static void hps_i2c_remove(struct i2c_client *client)
{
...
scoped_guard(rwsem_write, &hps->rwsem) {
hps->removed = true;
while (atomic_dec_if_positive(&hps->open_count) >= 0)
pm_runtime_put_noidle(&client->dev);
}
kref_put(&hps->kref, ...);
...
}
---
This is actually a good candidate for using the 'revocable'[1] (just like
the other drivers found by your coccinelle script where open files
outlive devm_kzalloc() driver data on unbind).
static int hps_open(struct inode *inode, struct file *file)
{
...
atomic_inc(&hps->open_count);
revocable_get(hps->rev);
file->private_data = hps->rev;
...
}
static int hps_release(struct inode *inode, struct file *file)
{
...
struct revocable *rev = file->private_data;
revocable_try_access_or_skip_scoped(rev, hps) {
pm_runtime_put(&hps->client->dev);
atomic_dec(&hps->open_count);
}
revocable_put(rev);
...
}
static int hps_i2c_probe(struct i2c_client *client)
{
...
hps->rev = revocable_alloc(hps);
...
}
static void hps_i2c_remove(struct i2c_client *client)
{
...
revocable_revoke(hps->rev);
while (atomic_dec_if_positive(&hps->open_count) >= 0)
pm_runtime_put_noidle(&client->dev);
...
revocable_put(hps->rev);
}
[1] https://lore.kernel.org/all/20261102161710.24076-1-tzungbi@kernel.org
^ permalink raw reply [flat|nested] 2+ messages in thread