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

[Chad Aldreda <ca@modelmiser.com>]

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() drops the
runtime PM references that open files still hold and sets hps->client to
NULL, 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>
---
v3:
- Use hps->client, set to NULL in remove(), to tell release() that the
  device has gone, in place of the removed flag (Tzung-Bi).
- Based on chrome-platform for-next (71c3b4fe7391). In mainline,
  4dd10fbbeb9c ("treewide: Convert devm_kmalloc() family to the
  devm_kmalloc_obj() family") changes the line this patch replaces to
  devm_kzalloc_obj(&client->dev, *hps); the result is kzalloc_obj()
  either way.
v2: https://lore.kernel.org/r/20261110123230.1251547-2-ca@modelmiser.com
- 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.
- One mutex in place of the rwsem and atomic_t in the suggestion.
- Not converted to revocable, which is not in mainline yet; this
  version can go to stable.
v1: https://lore.kernel.org/r/20261107185258.4115770-2-ca@modelmiser.com

Tested in QEMU 10.2.4 (q35, KASAN, lockdep) with the reproducer from v2:
a file is held open across an unbind of the client, across a
delete_device, and across an unbind and bind, and in a fourth run it is
closed before the client is deleted. There are no KASAN reports, lockdep
reports or warnings in any of the four, and across the unbind and bind
runtime_usage reads 1, 0 and 0, as with v2. With the line that sets
hps->client to NULL removed, the delete_device run reports
use-after-free in __pm_runtime_idle() and the unbind run a runtime PM
usage count underflow. 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 hps_release() ends in
"return pm_runtime_put(dev);", so the release() hunk conflicts, and
there is no kzalloc_obj(), so probe() needs kzalloc(sizeof(*hps),
GFP_KERNEL) to build.

The code, changelog and reproducer were written with an AI assistant
(Claude Opus 5.5), 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 | 74 ++++++++++++++++++++++----
 1 file changed, 65 insertions(+), 9 deletions(-)

diff --git a/drivers/platform/chrome/cros_hps_i2c.c b/drivers/platform/chrome/cros_hps_i2c.c
index 3b9485627831..436afc4f50d3 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,26 @@ 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 client against remove(). remove() sets
+	 * client to NULL after dropping the files' runtime PM references;
+	 * release() then leaves the device alone.
+	 */
+	struct mutex lock;
+	unsigned int open_count;
 };
 
+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 +57,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->client) {
+		hps->open_count--;
+		pm_runtime_put(&hps->client->dev);
+	}
+	mutex_unlock(&hps->lock);
 
+	kref_put(&hps->kref, hps_free);
 	return 0;
 }
 
@@ -62,10 +98,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 +122,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);
+	while (hps->open_count) {
+		hps->open_count--;
+		pm_runtime_put_noidle(&client->dev);
+	}
+	hps->client = NULL;
+	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