[PATCH] platform/chrome: cros_hps_i2c: hold the client device for an open file

[PATCH] platform/chrome: cros_hps_i2c: hold the client device for an open file

[Chad Aldreda]

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;
 }
-- 
2.43.0

Re: [PATCH] platform/chrome: cros_hps_i2c: hold the client device for an open file

[Tzung-Bi Shih]

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