[PATCH 1/2] HID: sensor: custom: Fix use-after-free when the node is open across removal

[PATCH 1/2] HID: sensor: custom: Fix use-after-free when the node is open across removal

[Chad Aldreda]

hid_sensor_custom_probe() allocates its state, which embeds the
miscdevice, with devm_kzalloc(). When the sensor hub goes away, for
example on USB disconnect, remove() deregisters the miscdevice and
devres frees the state, but a file that is still open keeps
file->private_data pointing into it: release() then writes the freed
misc_opened bit, and a reader blocked in read() is woken by remove()
but finds the kfifo empty and goes back to sleep on a wait queue that
devres then frees.

Reference count the state: probe and every open hold a reference, and
the last one frees the kfifo and the state. remove() marks the state
removed before waking readers, so a blocked or later read returns
-ENODEV and poll() reports EPOLLHUP, and the kfifo is no longer freed
before the hub callback is unregistered.

Found with a coccinelle check for device nodes embedded in devm_
memory. Reproduced in QEMU with a uhid program that creates a sensor
hub with one custom-sensor collection, opens the misc node, blocks a
reader in read(), destroys the uhid device and closes the node. Before
this change KASAN reports nine slab-use-after-frees: eight in
hid_sensor_custom_read() and the wait-queue code when the stuck reader
is finally killed, and a write in hid_sensor_custom_release(). The
reader stays blocked until killed. After it there are no reports and
the blocked read() returns -ENODEV. Not tested on real sensor hub
hardware, and the known-custom-sensor path that registers a separate
platform device was not exercised.

Fixes: 4a7de0519df5 ("HID: sensor: Custom and Generic sensor support")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Chad Aldreda <ca@modelmiser.com>
---
 drivers/hid/hid-sensor-custom.c | 45 ++++++++++++++++++++++++++++-----
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
index c2b425afd9..47800efdaf 100644
--- a/drivers/hid/hid-sensor-custom.c
+++ b/drivers/hid/hid-sensor-custom.c
@@ -15,6 +15,7 @@
 #include <linux/wait.h>
 #include <linux/poll.h>
 #include <linux/bsearch.h>
+#include <linux/kref.h>
 #include <linux/platform_device.h>
 #include <linux/hid-sensor-hub.h>
 
@@ -54,8 +55,25 @@ struct hid_sensor_custom {
 	unsigned long misc_opened;
 	wait_queue_head_t wait;
 	struct platform_device *custom_pdev;
+	/* Held by the driver binding and by each open file. */
+	struct kref kref;
+	bool removed;
 };
 
+static void hid_sensor_custom_free(struct kref *kref)
+{
+	struct hid_sensor_custom *sensor_inst =
+		container_of(kref, struct hid_sensor_custom, kref);
+
+	kfifo_free(&sensor_inst->data_fifo);
+	kfree(sensor_inst);
+}
+
+static void hid_sensor_custom_put(struct hid_sensor_custom *sensor_inst)
+{
+	kref_put(&sensor_inst->kref, hid_sensor_custom_free);
+}
+
 /* Header for each sample to user space via dev interface */
 struct hid_sensor_sample {
 	u32 usage_id;
@@ -658,11 +676,14 @@ static ssize_t hid_sensor_custom_read(struct file *file, char __user *buf,
 
 	do {
 		if (kfifo_is_empty(&sensor_inst->data_fifo)) {
+			if (READ_ONCE(sensor_inst->removed))
+				return -ENODEV;
 			if (file->f_flags & O_NONBLOCK)
 				return -EAGAIN;
 
 			ret = wait_event_interruptible(sensor_inst->wait,
-				!kfifo_is_empty(&sensor_inst->data_fifo));
+				!kfifo_is_empty(&sensor_inst->data_fifo) ||
+				READ_ONCE(sensor_inst->removed));
 			if (ret)
 				return ret;
 		}
@@ -684,6 +705,7 @@ static int hid_sensor_custom_release(struct inode *inode, struct file *file)
 				   struct hid_sensor_custom, custom_dev);
 
 	clear_bit(0, &sensor_inst->misc_opened);
+	hid_sensor_custom_put(sensor_inst);
 
 	return 0;
 }
@@ -698,6 +720,8 @@ static int hid_sensor_custom_open(struct inode *inode, struct file *file)
 	if (test_and_set_bit(0, &sensor_inst->misc_opened))
 		return -EBUSY;
 
+	kref_get(&sensor_inst->kref);
+
 	return stream_open(inode, file);
 }
 
@@ -714,6 +738,8 @@ static __poll_t hid_sensor_custom_poll(struct file *file,
 
 	if (!kfifo_is_empty(&sensor_inst->data_fifo))
 		mask = EPOLLIN | EPOLLRDNORM;
+	else if (READ_ONCE(sensor_inst->removed))
+		mask = EPOLLHUP | EPOLLERR;
 
 	return mask;
 }
@@ -751,10 +777,9 @@ static int hid_sensor_custom_dev_if_add(struct hid_sensor_custom *sensor_inst)
 static void hid_sensor_custom_dev_if_remove(struct hid_sensor_custom
 								*sensor_inst)
 {
+	WRITE_ONCE(sensor_inst->removed, true);
 	wake_up(&sensor_inst->wait);
 	misc_deregister(&sensor_inst->custom_dev);
-	kfifo_free(&sensor_inst->data_fifo);
-
 }
 
 /*
@@ -977,11 +1002,12 @@ static int hid_sensor_custom_probe(struct platform_device *pdev)
 	int ret;
 	const struct hid_sensor_custom_match *match;
 
-	sensor_inst = devm_kzalloc(&pdev->dev, sizeof(*sensor_inst),
-				   GFP_KERNEL);
+	sensor_inst = kzalloc_obj(*sensor_inst);
 	if (!sensor_inst)
 		return -ENOMEM;
 
+	kref_init(&sensor_inst->kref);
+
 	sensor_inst->callbacks.capture_sample = hid_sensor_capture_sample;
 	sensor_inst->callbacks.send_event = hid_sensor_send_event;
 	sensor_inst->callbacks.pdev = pdev;
@@ -999,7 +1025,7 @@ static int hid_sensor_custom_probe(struct platform_device *pdev)
 		if (ret) {
 			dev_err(&pdev->dev,
 				"register_platform_device failed\n");
-			return ret;
+			goto err_put;
 		}
 
 		return 0;
@@ -1009,7 +1035,7 @@ static int hid_sensor_custom_probe(struct platform_device *pdev)
 					   &sensor_inst->callbacks);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "callback reg failed\n");
-		return ret;
+		goto err_put;
 	}
 
 	ret = hid_sensor_custom_add_attributes(sensor_inst);
@@ -1034,6 +1060,8 @@ static int hid_sensor_custom_probe(struct platform_device *pdev)
 	hid_sensor_custom_remove_attributes(sensor_inst);
 err_remove_callback:
 	sensor_hub_remove_callback(hsdev, hsdev->usage);
+err_put:
+	hid_sensor_custom_put(sensor_inst);
 
 	return ret;
 }
@@ -1045,6 +1073,7 @@ static void hid_sensor_custom_remove(struct platform_device *pdev)
 
 	if (sensor_inst->custom_pdev) {
 		platform_device_unregister(sensor_inst->custom_pdev);
+		hid_sensor_custom_put(sensor_inst);
 		return;
 	}
 
@@ -1054,6 +1083,8 @@ static void hid_sensor_custom_remove(struct platform_device *pdev)
 			   &enable_sensor_attr_group);
 	hid_sensor_custom_remove_attributes(sensor_inst);
 	sensor_hub_remove_callback(hsdev, hsdev->usage);
+	/* An open file may still hold the last reference. */
+	hid_sensor_custom_put(sensor_inst);
 }
 
 static const struct platform_device_id hid_sensor_custom_ids[] = {
-- 
2.43.0

[PATCH 2/2] HID: sensor: custom: Set the owner of the misc file operations

[Chad Aldreda]

hid_sensor_custom_fops has no .owner, so an open file on the misc node
does not pin the module: with the node open, the module can be
unloaded, and the file's later release() calls into freed module text.

Set .owner to THIS_MODULE so misc_open()'s fops_get() takes a module
reference for each open file.

Found by review of the previous fix. Tested in QEMU with the driver
built as a module and the uhid reproducer holding the node open. Without
this change, rmmod succeeds and the later close() oopses in filp_flush()
on a page fault in the unloaded module. With it, rmmod fails while the
node is open, and succeeds after close() with no oops.

Fixes: 4a7de0519df5 ("HID: sensor: Custom and Generic sensor support")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Chad Aldreda <ca@modelmiser.com>
---
 drivers/hid/hid-sensor-custom.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
index 47800efdaf..bee612eb1d 100644
--- a/drivers/hid/hid-sensor-custom.c
+++ b/drivers/hid/hid-sensor-custom.c
@@ -745,6 +745,7 @@ static __poll_t hid_sensor_custom_poll(struct file *file,
 }
 
 static const struct file_operations hid_sensor_custom_fops = {
+	.owner = THIS_MODULE,
 	.open =  hid_sensor_custom_open,
 	.read =  hid_sensor_custom_read,
 	.release = hid_sensor_custom_release,
-- 
2.43.0

Re: [PATCH 1/2] HID: sensor: custom: Fix use-after-free when the node is open across removal

[Chad Aldreda]

Please drop both patches of this series. Srinivas posted the same fixes
on Nov 4 in "[PATCH v2 0/7] HID: sensor: custom: use-after-freec after
remove": his 4/7 takes the same refcount-and-removed-flag approach as
1/2 here, his 2/7 is identical to 2/2, and his 1/7 also synchronizes
hub callback removal with raw events. I missed it before sending.
Sorry for the noise.