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 Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Chad Aldreda --- 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 #include #include +#include #include #include +#include #include +#include #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