Re: Fwd: [BUG] HID: sensor: custom: use-after-free of sensor_inst via open misc device after remove (hard lockup)

[Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 6221 bytes --]

Hi,
On Fri, 2026-10-23 at 13:42 +0300, Семён Марьясин wrote:
> Hello,
> 
> hid-sensor-custom frees its per-sensor state (sensor_inst,
> devm-allocated) on remove while userspace
> can still hold the misc device /dev/HID-SENSOR-2000e1.N.auto open. A
> reader blocked in read() stays
> queued on sensor_inst->wait in freed memory; when it wakes up,
> finish_wait() spins on a garbage
> wait-queue lock with interrupts disabled and the machine hard-locks.
> Any read()/poll()/release() on such
> an fd after remove is a use-after-free as well.
> 

Please give a try with the attached diff.


Thanks,
Srinivas


> The Sashiko AI review already flagged this as a pre-existing issue
> on Haoxiang Li's "Fix fields lifetime issues" series
> ("The /dev/HID-SENSOR-* character device operations do not
> synchronize
> with device
> removal,
> leading to Use-After-Free on devm-managed structures"):
> https://lore.kernel.org/linux-input/20260707071545.3087073-1-haoxiang_li2024@163.com/
> The applied patches don't touch it, and the current mainline is
> unchanged. Below is a real-world hit.
> 
> Kernel: 7.2.8 (NixOS; drivers/hid/hid-sensor-custom.c is identical to
> current mainline,
>         including ad8fb82b0442 and 3789d0802ddb)
> Hardware: Dell Latitude 5420 (Tiger Lake, Intel ISH), BIOS 1.49.0;
> Dell's vendor ISH sensors appear as
>           HID-SENSOR-2000e1.N.auto (e.g. an ST VL53L1 time-of-flight
> sensor).
> Taint: O, from unrelated out-of-tree WWAN modules (qcserial,
> usb_wwan).
> 
> How it was hit
> --------------
> 
> 1. echo 1 > /sys/bus/platform/devices/HID-SENSOR-
> 2000e1.2.auto/enable_sensor
> 2. xxd /dev/HID-SENSOR-2000e1.2.auto        # streaming reader, left
> running
> 3. modprobe -r intel_ishtp_hid intel_ish_ipc && modprobe
> intel_ish_ipc
> 
> The hub went away and was re-enumerated fine; ~30 s later CPU0
> hard-locked in the still-running xxd
> and the hard-lockup detector panicked the machine (log recovered from
> EFI pstore).
> 
> Presumably a minimal reproducer is just a blocked reader plus
> unbinding the platform device
> (echo HID-SENSOR-2000e1.N.auto >
> /sys/bus/platform/drivers/hid_sensor_custom/unbind),
> but only the module-reload path above was actually tested so far.
> 
> Log (trimmed)
> -------------
> 
> [146608.877730] hid-sensor-hub 001F:8087:0AC2.0003: timeout waiting
> for response from ISHTP device
> [146611.885959] hid-sensor-hub 001F:8087:0AC2.0003: timeout waiting
> for response from ISHTP device
> [146614.893909] hid-sensor-hub 001F:8087:0AC2.0003: timeout waiting
> for response from ISHTP device
> [146617.901872] hid-sensor-hub 001F:8087:0AC2.0003: timeout waiting
> for response from ISHTP device
> [146619.972550] ish-hid {33AECD58-B679-4E54-9BD9-A04D34F0C226}:
> [hid-ish]: enum_devices_done OK, num_hid_devices=3
> [146620.031441] hid-sensor-hub 001F:8087:0AC2.0007: hidraw3: SENSOR
> HUB HID v2.00 Device [hid-ishtp 8087:0AC2] on
> [146643.503266] iwlwifi 0000:00:14.3: Queue 3 is stuck 46 70
> [146648.924391] watchdog: CPU0: Watchdog detected hard LOCKUP on cpu
> 0
> [146648.924796] CPU: 0 UID: 0 PID: 1316004 Comm: xxd Tainted: G
>    O        7.2.8 #1-NixOS PREEMPT(lazy)
> [146648.924804] Hardware name: Dell Inc. Latitude 5420/03P5X2, BIOS
> 1.49.0 09/01/2025
> [146648.924808] RIP: 0010:native_queued_spin_lock_slowpath+0x64/0x2c0
> [146648.924851] Call Trace:
> [146648.924855]  <TASK>
> [146648.924857]  _raw_spin_lock_irqsave+0x3d/0x50
> [146648.924872]  finish_wait+0x3b/0x90
> [146648.924886]  hid_sensor_custom_read+0x102/0x150
> [hid_sensor_custom]
> [146648.924901]  vfs_read+0xbf/0x390
> [146648.924934]  ksys_read+0xbf/0xf0
> [146648.924939]  do_syscall_64+0x134/0x6b0
> [146648.925010]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> [146648.925083]  </TASK>
> [146648.925086] Kernel panic - not syncing: Hard LOCKUP
> 
> Also attaching a full dmesg log (with only wi-fi noise filtered out).
> 
> Analysis (line numbers as in mainline 73bef5)
> ---------------------------------------------
> 
> - hid_sensor_custom_probe() allocates sensor_inst with devm_kzalloc()
> (line 980), so it is freed right
>   after hid_sensor_custom_remove() returns, regardless of open files.
> - The misc device's fops reach sensor_inst through
> container_of(file->private_data, ..., custom_dev);
>   misc_deregister() does not revoke already-open files, so
> read/poll/release keep dereferencing it.
> - hid_sensor_custom_dev_if_remove() (lines 751-758) does wake_up(),
> misc_deregister(), kfifo_free().
>   The woken reader in hid_sensor_custom_read() (line 664) re-
> evaluates
>   wait_event_interruptible(sensor_inst->wait,
> !kfifo_is_empty(&sensor_inst->data_fifo)), finds the fifo
>   empty and goes back to sleep on sensor_inst->wait. Nothing tells it
> the device is gone.
> - Once sensor_inst is freed and reused, the next wakeup of that
> reader
> runs finish_wait() on the freed
>   wait_queue_head: spin_lock_irqsave() on garbage, hard lockup.
> hid_sensor_custom_release() similarly
>   does clear_bit() on freed sensor_inst->misc_opened when the fd is
> finally closed.
> 
> Possible fix direction
> ----------------------
> 
> Tie sensor_inst's lifetime to its users rather than to devres, the
> way
> hidraw handles disconnect:
> refcount it (kref: one reference for the device, one per open file;
> kfifo_free() and kfree() on the last
> put), add a "removed" flag set in remove() before
> wake_up_interruptible_all(), include it in the
> wait_event condition, and make read()/poll() return -ENODEV /
> EPOLLHUP
> > EPOLLERR once removed.
> 
> The same review pointed out two more problems in this teardown path
> that a fix would naturally cover:
> remove() frees data_fifo before sensor_hub_remove_callback(), so a
> sample arriving in between is
> kfifo_in()'d into freed memory; and an enabled sensor is never
> sensor_hub_device_close()d on remove.
> 
> I can test patches on this machine.
> 
> Thanks,
> Semyon Maryasin
> 
> P.S.: sorry for a possible duplicate, I think I messed with the
> headers.

[-- Attachment #2: test_patch.diff --]
[-- Type: text/x-patch, Size: 7630 bytes --]

diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
index afffea894021..c12417cddab4 100644
--- a/drivers/hid/hid-sensor-custom.c
+++ b/drivers/hid/hid-sensor-custom.c
@@ -11,6 +11,7 @@
 #include <linux/init.h>
 #include <linux/miscdevice.h>
 #include <linux/kfifo.h>
+#include <linux/kref.h>
 #include <linux/sched.h>
 #include <linux/wait.h>
 #include <linux/poll.h>
@@ -37,6 +38,8 @@ struct hid_sensor_custom_field {
 
 struct hid_sensor_custom {
 	struct mutex mutex;
+	struct kref ref;
+	bool removed;
 	struct platform_device *pdev;
 	struct hid_sensor_hub_device *hsdev;
 	struct hid_sensor_hub_callbacks callbacks;
@@ -636,6 +639,15 @@ static void hid_sensor_custom_remove_attributes(struct hid_sensor_custom *
 	kfree(sensor_inst->fields);
 }
 
+static void hid_sensor_custom_free(struct kref *ref)
+{
+	struct hid_sensor_custom *sensor_inst =
+		container_of(ref, struct hid_sensor_custom, ref);
+
+	kfifo_free(&sensor_inst->data_fifo);
+	kfree(sensor_inst);
+}
+
 static ssize_t hid_sensor_custom_read(struct file *file, char __user *buf,
 				      size_t count, loff_t *f_ps)
 {
@@ -649,22 +661,35 @@ static ssize_t hid_sensor_custom_read(struct file *file, char __user *buf,
 	if (count < sizeof(struct hid_sensor_sample))
 		return -EINVAL;
 
-	do {
-		if (kfifo_is_empty(&sensor_inst->data_fifo)) {
-			if (file->f_flags & O_NONBLOCK)
-				return -EAGAIN;
+	for (;;) {
+		ret = mutex_lock_interruptible(&sensor_inst->mutex);
+		if (ret)
+			return ret;
 
-			ret = wait_event_interruptible(sensor_inst->wait,
-				!kfifo_is_empty(&sensor_inst->data_fifo));
-			if (ret)
-				return ret;
+		if (sensor_inst->removed) {
+			ret = -ENODEV;
+			goto unlock;
 		}
 		ret = kfifo_to_user(&sensor_inst->data_fifo, buf, count,
 				    &copied);
+		if (ret || copied)
+			goto unlock;
+		mutex_unlock(&sensor_inst->mutex);
+
+		if (file->f_flags & O_NONBLOCK)
+			return -EAGAIN;
+
+		ret = wait_event_interruptible(sensor_inst->wait,
+			READ_ONCE(sensor_inst->removed) ||
+			!kfifo_is_empty(&sensor_inst->data_fifo));
 		if (ret)
 			return ret;
+	}
 
-	} while (copied == 0);
+unlock:
+	mutex_unlock(&sensor_inst->mutex);
+	if (ret)
+		return ret;
 
 	return copied;
 }
@@ -677,6 +702,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);
+	kref_put(&sensor_inst->ref, hid_sensor_custom_free);
 
 	return 0;
 }
@@ -684,14 +710,22 @@ static int hid_sensor_custom_release(struct inode *inode, struct file *file)
 static int hid_sensor_custom_open(struct inode *inode, struct file *file)
 {
 	struct hid_sensor_custom *sensor_inst;
+	int ret;
 
 	sensor_inst = container_of(file->private_data,
 				   struct hid_sensor_custom, custom_dev);
-	/* We essentially have single reader and writer */
-	if (test_and_set_bit(0, &sensor_inst->misc_opened))
-		return -EBUSY;
+	mutex_lock(&sensor_inst->mutex);
+	if (sensor_inst->removed) {
+		ret = -ENODEV;
+	} else if (test_and_set_bit(0, &sensor_inst->misc_opened)) {
+		ret = -EBUSY;
+	} else {
+		kref_get(&sensor_inst->ref);
+		ret = stream_open(inode, file);
+	}
+	mutex_unlock(&sensor_inst->mutex);
 
-	return stream_open(inode, file);
+	return ret;
 }
 
 static __poll_t hid_sensor_custom_poll(struct file *file,
@@ -705,6 +739,9 @@ static __poll_t hid_sensor_custom_poll(struct file *file,
 
 	poll_wait(file, &sensor_inst->wait, wait);
 
+	if (READ_ONCE(sensor_inst->removed))
+		return EPOLLHUP | EPOLLERR;
+
 	if (!kfifo_is_empty(&sensor_inst->data_fifo))
 		mask = EPOLLIN | EPOLLRDNORM;
 
@@ -712,6 +749,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,
@@ -734,20 +772,17 @@ static int hid_sensor_custom_dev_if_add(struct hid_sensor_custom *sensor_inst)
 	sensor_inst->custom_dev.name = dev_name(&sensor_inst->pdev->dev);
 	sensor_inst->custom_dev.fops = &hid_sensor_custom_fops;
 	ret = misc_register(&sensor_inst->custom_dev);
-	if (ret) {
-		kfifo_free(&sensor_inst->data_fifo);
-		return ret;
-	}
-	return 0;
+	return ret;
 }
 
 static void hid_sensor_custom_dev_if_remove(struct hid_sensor_custom
 								*sensor_inst)
 {
-	wake_up(&sensor_inst->wait);
+	mutex_lock(&sensor_inst->mutex);
+	WRITE_ONCE(sensor_inst->removed, true);
+	mutex_unlock(&sensor_inst->mutex);
+	wake_up_interruptible_all(&sensor_inst->wait);
 	misc_deregister(&sensor_inst->custom_dev);
-	kfifo_free(&sensor_inst->data_fifo);
-
 }
 
 /*
@@ -970,8 +1005,7 @@ 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;
 
@@ -981,6 +1015,7 @@ static int hid_sensor_custom_probe(struct platform_device *pdev)
 	sensor_inst->hsdev = hsdev;
 	sensor_inst->pdev = pdev;
 	mutex_init(&sensor_inst->mutex);
+	kref_init(&sensor_inst->ref);
 	platform_set_drvdata(pdev, sensor_inst);
 
 	ret = hid_sensor_custom_get_known(hsdev, &match);
@@ -992,7 +1027,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_free;
 		}
 
 		return 0;
@@ -1002,7 +1037,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_free;
 	}
 
 	ret = sysfs_create_group(&sensor_inst->pdev->dev.kobj,
@@ -1027,6 +1062,8 @@ static int hid_sensor_custom_probe(struct platform_device *pdev)
 			   &enable_sensor_attr_group);
 err_remove_callback:
 	sensor_hub_remove_callback(hsdev, hsdev->usage);
+err_free:
+	kref_put(&sensor_inst->ref, hid_sensor_custom_free);
 
 	return ret;
 }
@@ -1038,14 +1075,18 @@ static void hid_sensor_custom_remove(struct platform_device *pdev)
 
 	if (sensor_inst->custom_pdev) {
 		platform_device_unregister(sensor_inst->custom_pdev);
-		return;
+		goto out;
 	}
 
 	hid_sensor_custom_dev_if_remove(sensor_inst);
 	hid_sensor_custom_remove_attributes(sensor_inst);
 	sysfs_remove_group(&sensor_inst->pdev->dev.kobj,
 			   &enable_sensor_attr_group);
+	if (sensor_inst->enable)
+		sensor_hub_device_close(hsdev);
 	sensor_hub_remove_callback(hsdev, hsdev->usage);
+out:
+	kref_put(&sensor_inst->ref, hid_sensor_custom_free);
 }
 
 static const struct platform_device_id hid_sensor_custom_ids[] = {
diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
index 90666ff629de..2bac8b82ce37 100644
--- a/drivers/hid/hid-sensor-hub.c
+++ b/drivers/hid/hid-sensor-hub.c
@@ -173,7 +173,8 @@ int sensor_hub_remove_callback(struct hid_sensor_hub_device *hsdev,
 	struct sensor_hub_data *pdata = hid_get_drvdata(hsdev->hdev);
 	unsigned long flags;
 
-	spin_lock_irqsave(&pdata->dyn_callback_lock, flags);
+	spin_lock_irqsave(&pdata->lock, flags);
+	spin_lock(&pdata->dyn_callback_lock);
 	list_for_each_entry(callback, &pdata->dyn_callback_list, list)
 		if (callback->usage_id == usage_id &&
 						callback->hsdev == hsdev) {
@@ -181,7 +182,8 @@ int sensor_hub_remove_callback(struct hid_sensor_hub_device *hsdev,
 			kfree(callback);
 			break;
 		}
-	spin_unlock_irqrestore(&pdata->dyn_callback_lock, flags);
+	spin_unlock(&pdata->dyn_callback_lock);
+	spin_unlock_irqrestore(&pdata->lock, flags);
 
 	return 0;
 }