[PATCH] platform/x86: dell-smo8800: keep the state until the last file is closed

[PATCH] platform/x86: dell-smo8800: keep the state until the last file is closed

[Chad Aldreda]

struct smo8800_device is allocated with devm_kzalloc(), so it is freed
when the driver is unbound, but a /dev/freefall file opened before the
unbind still reaches it through file->private_data. release() then
clears misc_opened in freed memory, and a read() blocked in
wait_event_interruptible() is not woken by remove(). It sleeps on a wait
queue inside the freed structure until a signal arrives, and then uses
it. Unbinding the driver needs root. With KASAN, closing the file after
an unbind gives a slab-use-after-free in smo8800_misc_release(), and a
blocked reader interrupted after the unbind gives further reports in
smo8800_misc_read() and prepare_to_wait_event().

Allocate the structure with kzalloc() and give it a kref. open() takes a
reference and release() drops it. The device's own reference is dropped
by a devm action, which runs after smo8800_remove() on unbind and when
probe fails. misc_open() calls ->open with misc_mtx held and
misc_deregister() takes misc_mtx, so the device reference is held
whenever ->open runs. Once the misc device is deregistered, by remove()
or by the probe error path, a removed flag is set and the wait queue
woken. A read() blocked at that point returns -ENODEV unless an event
arrived first, and a later read() returns -ENODEV.

The problem was found with a Coccinelle script that reports device nodes
in devm memory [1]. The fix, the reproducer and this message were
written with an AI assistant (Claude) and checked by hand. The fix was
tested in QEMU, with KASAN enabled, using a test-only module that
registers an "smo8800" platform device with a simulated interrupt, not
on Dell hardware.

Link: https://lore.kernel.org/r/20261106193517.3267013-2-ca@modelmiser.com [1]
Fixes: 4738d8aabe89 ("platform: x86: dell-smo8800: Dell Latitude freefall driver (ACPI SMO8800/SMO8810)")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Chad Aldreda <ca@modelmiser.com>
---
The Coccinelle script reports a devm_k*() allocation when it, or an
address inside it, is passed to misc_register(), cdev_add(),
cdev_device_add(), video_register_device() or v4l2_device_register()
in the same function, or when the allocated struct embeds one of those
objects. It reports dell-smo8800 for the misc_register() of
&smo8800->miscdev.

Reproduced in QEMU 10.2.4 (q35, KASAN, booted with kasan_multi_shot). A
test-only module registers a platform device named "smo8800" with an
interrupt from an irq_sim domain, so the driver binds without the ACPI
device, and a module parameter raises the interrupt. A process opens
/dev/freefall and the driver is unbound through sysfs. Without this
patch:

  close after the unbind:                  1 KASAN report (release)
  read() blocked across the unbind:        not woken; after a signal,
                                           7 reports (read, wait queue,
                                           release)
  unbind, bind, then close the old file:   1 report
  close before the unbind (control):       0

With it there are none: the blocked read() returns -ENODEV when the
driver is unbound, a bind after the unbind works, and an interrupt
still wakes a reader with a count. Not tested on Dell hardware.

An interrupt that arrives while remove() runs, between the reader's
count check and its removed check, is dropped and the read returns
-ENODEV. remove() frees the IRQ before setting removed, so no event can
be lost after that.

kzalloc() rather than kzalloc_obj(), which first appears in v7.0: the
patch then applies as is from v6.14. On v6.6 to v6.13 only the include
hunk needs adjusting (those trees have no dell-smo8800-ids.h); v5.15 and
v6.1 need a manual backport (remove() returns int there), and before
v5.15 the driver was an ACPI driver.

 drivers/platform/x86/dell/dell-smo8800.c | 35 ++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
index 8872f9b57fce..8880bf39f895 100644
--- a/drivers/platform/x86/dell/dell-smo8800.c
+++ b/drivers/platform/x86/dell/dell-smo8800.c
@@ -13,9 +13,11 @@
 #include <linux/fs.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
+#include <linux/kref.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/slab.h>
 #include <linux/uaccess.h>
 #include "dell-smo8800-ids.h"
 
@@ -26,8 +28,22 @@ struct smo8800_device {
 	unsigned long misc_opened;   /* whether the device is open */
 	wait_queue_head_t misc_wait; /* Wait queue for the misc dev */
 	struct device *dev;          /* acpi device */
+	struct kref kref;            /* device + open file */
+	bool removed;                /* the device has been removed */
 };
 
+static void smo8800_free(struct kref *kref)
+{
+	kfree(container_of(kref, struct smo8800_device, kref));
+}
+
+static void smo8800_put(void *data)
+{
+	struct smo8800_device *smo8800 = data;
+
+	kref_put(&smo8800->kref, smo8800_free);
+}
+
 static irqreturn_t smo8800_interrupt_quick(int irq, void *data)
 {
 	struct smo8800_device *smo8800 = data;
@@ -60,11 +76,15 @@ static ssize_t smo8800_misc_read(struct file *file, char __user *buf,
 
 	atomic_set(&smo8800->counter, 0);
 	retval = wait_event_interruptible(smo8800->misc_wait,
-				(data = atomic_xchg(&smo8800->counter, 0)));
+				(data = atomic_xchg(&smo8800->counter, 0)) ||
+				READ_ONCE(smo8800->removed));
 
 	if (retval)
 		return retval;
 
+	if (!data)
+		return -ENODEV;
+
 	retval = 1;
 
 	byte_data = min_t(u32, data, 255);
@@ -84,6 +104,7 @@ static int smo8800_misc_open(struct inode *inode, struct file *file)
 		return -EBUSY; /* already open */
 
 	atomic_set(&smo8800->counter, 0);
+	kref_get(&smo8800->kref);
 	return 0;
 }
 
@@ -93,6 +114,7 @@ static int smo8800_misc_release(struct inode *inode, struct file *file)
 					 struct smo8800_device, miscdev);
 
 	clear_bit(0, &smo8800->misc_opened); /* release the device */
+	smo8800_put(smo8800);
 	return 0;
 }
 
@@ -108,11 +130,16 @@ static int smo8800_probe(struct platform_device *device)
 	int err;
 	struct smo8800_device *smo8800;
 
-	smo8800 = devm_kzalloc(&device->dev, sizeof(*smo8800), GFP_KERNEL);
+	/* An open file can outlive the device, so it holds a reference. */
+	smo8800 = kzalloc(sizeof(*smo8800), GFP_KERNEL);
 	if (!smo8800) {
 		dev_err(&device->dev, "failed to allocate device data\n");
 		return -ENOMEM;
 	}
+	kref_init(&smo8800->kref);
+	err = devm_add_action_or_reset(&device->dev, smo8800_put, smo8800);
+	if (err)
+		return err;
 
 	smo8800->dev = &device->dev;
 	smo8800->miscdev.minor = MISC_DYNAMIC_MINOR;
@@ -151,6 +178,8 @@ static int smo8800_probe(struct platform_device *device)
 
 error:
 	misc_deregister(&smo8800->miscdev);
+	WRITE_ONCE(smo8800->removed, true);
+	wake_up_interruptible(&smo8800->misc_wait);
 	return err;
 }
 
@@ -160,6 +189,8 @@ static void smo8800_remove(struct platform_device *device)
 
 	free_irq(smo8800->irq, smo8800);
 	misc_deregister(&smo8800->miscdev);
+	WRITE_ONCE(smo8800->removed, true);
+	wake_up_interruptible(&smo8800->misc_wait);
 	dev_dbg(&device->dev, "device /dev/freefall unregistered\n");
 }
 
-- 
2.43.0