[PATCH v2] platform/chrome: cros_hps_i2c: fix use-after-free in release() after unbind

[PATCH v2] platform/chrome: cros_hps_i2c: fix use-after-free in release() after unbind

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

Allocate hps with kzalloc_obj() and reference-count it: the binding
holds one reference and each open file another. remove() marks hps
removed and drops the runtime PM references that open files still hold,
before it disables runtime PM, so a release() after that only drops its
reference to hps. A file opened before an unbind then no longer affects
the device, including after the driver is bound again.

remove() now calls misc_deregister() first: misc_open() calls
hps_open() with misc_mtx held, and misc_deregister() takes misc_mtx and
unlists the device, so once it returns no open() is in progress and
none can start. The open count that remove() then drains cannot grow
again.

Fixes: 5f9952548d91 ("platform/chrome: add a driver for HPS")
Cc: stable@vger.kernel.org
Suggested-by: Tzung-Bi Shih <tzungbi@kernel.org>
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Chad Aldreda <ca@modelmiser.com>
---
v2:
- Follow Tzung-Bi's suggestion: reference-count hps and have remove()
  drop the runtime PM references of files that are still open, instead
  of holding the client device for each open file. A file left open
  across an unbind and rebind no longer affects the new binding.
- One mutex protects open_count and removed, in place of the rwsem and
  atomic_t in the suggestion. open() cannot run concurrently with
  remove() (see the changelog), so the lock only orders release()
  against remove() and against other opens and releases, which are
  rare.
- Not converted to revocable, which is not in mainline yet; this
  version can go to stable.
- Subject changed to name the bug.
v1: https://lore.kernel.org/r/20261107185258.4115770-2-ca@modelmiser.com

Reproduced in QEMU 10.2.4 (q35, KASAN) as for v1: 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, also with lockdep enabled. A
test-only print in the free path showed hps freed at the last close
after an unbind or delete_device, and in remove() when no file was
open. With a file left open across an unbind and bind, runtime_usage
reads 1 with both files open, 0 after closing the new file and 0 after
closing the old one (2, 1 and 0 without the patch). Not tested on HPS
hardware, and a release() racing remove() was not exercised. Unbinding
or deleting the client needs root.

For stable, before v7.0 this conflicts in two places: there is no
kzalloc_obj(), so probe should use kzalloc(sizeof(*hps), GFP_KERNEL),
and hps_release() ends in "return pm_runtime_put(dev);", which the new
release() body replaces.

The code, changelog and reproducer were written with an AI assistant
(Claude Opus 5.5), working from Tzung-Bi's suggested structure, and
the assistant ran the tests above. I ran the final version myself and
got the same results.

 drivers/platform/chrome/cros_hps_i2c.c | 75 ++++++++++++++++++++++----
 1 file changed, 66 insertions(+), 9 deletions(-)

diff --git a/drivers/platform/chrome/cros_hps_i2c.c b/drivers/platform/chrome/cros_hps_i2c.c
index 3b9485627831..bff063f58777 100644
--- a/drivers/platform/chrome/cros_hps_i2c.c
+++ b/drivers/platform/chrome/cros_hps_i2c.c
@@ -14,9 +14,12 @@
 #include <linux/fs.h>
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
+#include <linux/kref.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/pm_runtime.h>
+#include <linux/slab.h>
 
 #define HPS_ACPI_ID		"GOOG0020"
 
@@ -24,8 +27,27 @@ struct hps_drvdata {
 	struct i2c_client *client;
 	struct miscdevice misc_device;
 	struct gpio_desc *enable_gpio;
+
+	/* hps outlives the driver binding while files are open. */
+	struct kref kref;
+	/*
+	 * Protects open_count and removed. Once removed is set, release()
+	 * leaves the device alone: remove() has dropped the files' runtime PM
+	 * references.
+	 */
+	struct mutex lock;
+	unsigned int open_count;
+	bool removed;
 };
 
+static void hps_free(struct kref *kref)
+{
+	struct hps_drvdata *hps = container_of(kref, struct hps_drvdata, kref);
+
+	mutex_destroy(&hps->lock);
+	kfree(hps);
+}
+
 static void hps_set_power(struct hps_drvdata *hps, bool state)
 {
 	gpiod_set_value_cansleep(hps->enable_gpio, state);
@@ -36,18 +58,33 @@ 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);
+	mutex_lock(&hps->lock);
+	hps->open_count++;
+	mutex_unlock(&hps->lock);
+
+	kref_get(&hps->kref);
+	file->private_data = hps;
+	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 hps_drvdata *hps = file->private_data;
 
-	pm_runtime_put(dev);
+	mutex_lock(&hps->lock);
+	if (!hps->removed) {
+		hps->open_count--;
+		pm_runtime_put(&hps->client->dev);
+	}
+	mutex_unlock(&hps->lock);
 
+	kref_put(&hps->kref, hps_free);
 	return 0;
 }
 
@@ -62,10 +99,13 @@ static int hps_i2c_probe(struct i2c_client *client)
 	struct hps_drvdata *hps;
 	int ret;
 
-	hps = devm_kzalloc(&client->dev, sizeof(*hps), GFP_KERNEL);
+	hps = kzalloc_obj(*hps);
 	if (!hps)
 		return -ENOMEM;
 
+	kref_init(&hps->kref);
+	mutex_init(&hps->lock);
+
 	hps->misc_device.parent = &client->dev;
 	hps->misc_device.minor = MISC_DYNAMIC_MINOR;
 	hps->misc_device.name = "cros-hps";
@@ -83,32 +123,49 @@ static int hps_i2c_probe(struct i2c_client *client)
 	if (IS_ERR(hps->enable_gpio)) {
 		ret = PTR_ERR(hps->enable_gpio);
 		dev_err(&client->dev, "failed to get enable gpio: %d\n", ret);
-		return ret;
+		goto err_put;
 	}
 
 	ret = misc_register(&hps->misc_device);
 	if (ret) {
 		dev_err(&client->dev, "failed to initialize misc device: %d\n", ret);
-		return ret;
+		goto err_put;
 	}
 
 	hps_set_power(hps, false);
 	pm_runtime_enable(&client->dev);
 	return 0;
+
+err_put:
+	kref_put(&hps->kref, hps_free);
+	return ret;
 }
 
 static void hps_i2c_remove(struct i2c_client *client)
 {
 	struct hps_drvdata *hps = i2c_get_clientdata(client);
 
-	pm_runtime_disable(&client->dev);
+	/* After misc_deregister(), no open() is running or can start. */
 	misc_deregister(&hps->misc_device);
 
+	/* Drop the runtime PM references of files that are still open. */
+	mutex_lock(&hps->lock);
+	hps->removed = true;
+	while (hps->open_count) {
+		hps->open_count--;
+		pm_runtime_put_noidle(&client->dev);
+	}
+	mutex_unlock(&hps->lock);
+
+	pm_runtime_disable(&client->dev);
+
 	/*
 	 * Re-enable HPS, in order to return it to its default state
 	 * (i.e. powered on).
 	 */
 	hps_set_power(hps, true);
+
+	kref_put(&hps->kref, hps_free);
 }
 
 static int hps_suspend(struct device *dev)
-- 
2.43.0