[PATCH] HID: sensor-custom: Synchronize misc-device teardown with file lifetime

[PATCH] HID: sensor-custom: Synchronize misc-device teardown with file lifetime

[Srinivas Pandruvada]

The HID sensor custom misc device frees its per-sensor state in
remove() while userspace may still hold an open file descriptor. A
reader blocked in read() can remain queued on sensor_inst->wait after the
object has been freed, and a later wakeup races with teardown and
touches stale memory.

Synchronize the misc-device teardown with file lifetime before freeing
the per-sensor state. Set a removal flag, reject new opens, wake
blocked waiters, and wait for the last open file to close before
deregistering the misc device and freeing the FIFO. Fail in-flight read()
and poll() operations cleanly once removal is in progress so userspace
cannot access freed memory after the device disappears.

This avoids use-after-free on read(), poll(), release(), and open()
after the HID sensor custom character device has been removed

Reported-by: Семён Марьясин <simeon@maryasin.name>
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/hid-sensor-custom.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
index c2b425afd951..c9338ae47050 100644
--- a/drivers/hid/hid-sensor-custom.c
+++ b/drivers/hid/hid-sensor-custom.c
@@ -53,6 +53,14 @@ struct hid_sensor_custom {
 	struct kfifo data_fifo;
 	unsigned long misc_opened;
 	wait_queue_head_t wait;
+	wait_queue_head_t remove_wait;
+	/*
+	 * Shared with the file operations; use READ_ONCE()/WRITE_ONCE() for the
+	 * teardown state. The actual lifetime guarantee comes from misc_opened:
+	 * removal waits for the last fd to close before deregistering the misc
+	 * device and freeing the FIFO.
+	 */
+	bool removing;
 	struct platform_device *custom_pdev;
 };
 
@@ -653,6 +661,9 @@ static ssize_t hid_sensor_custom_read(struct file *file, char __user *buf,
 	sensor_inst = container_of(file->private_data,
 				   struct hid_sensor_custom, custom_dev);
 
+	if (READ_ONCE(sensor_inst->removing))
+		return -ENODEV;
+
 	if (count < sizeof(struct hid_sensor_sample))
 		return -EINVAL;
 
@@ -662,9 +673,12 @@ static ssize_t hid_sensor_custom_read(struct file *file, char __user *buf,
 				return -EAGAIN;
 
 			ret = wait_event_interruptible(sensor_inst->wait,
+				READ_ONCE(sensor_inst->removing) ||
 				!kfifo_is_empty(&sensor_inst->data_fifo));
 			if (ret)
 				return ret;
+			if (READ_ONCE(sensor_inst->removing))
+				return -ENODEV;
 		}
 		ret = kfifo_to_user(&sensor_inst->data_fifo, buf, count,
 				    &copied);
@@ -684,6 +698,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);
+	wake_up_all(&sensor_inst->remove_wait);
 
 	return 0;
 }
@@ -694,6 +709,8 @@ static int hid_sensor_custom_open(struct inode *inode, struct file *file)
 
 	sensor_inst = container_of(file->private_data,
 				   struct hid_sensor_custom, custom_dev);
+	if (READ_ONCE(sensor_inst->removing))
+		return -ENODEV;
 	/* We essentially have single reader and writer */
 	if (test_and_set_bit(0, &sensor_inst->misc_opened))
 		return -EBUSY;
@@ -712,6 +729,9 @@ static __poll_t hid_sensor_custom_poll(struct file *file,
 
 	poll_wait(file, &sensor_inst->wait, wait);
 
+	if (READ_ONCE(sensor_inst->removing))
+		return EPOLLERR;
+
 	if (!kfifo_is_empty(&sensor_inst->data_fifo))
 		mask = EPOLLIN | EPOLLRDNORM;
 
@@ -736,6 +756,7 @@ static int hid_sensor_custom_dev_if_add(struct hid_sensor_custom *sensor_inst)
 		return ret;
 
 	init_waitqueue_head(&sensor_inst->wait);
+	init_waitqueue_head(&sensor_inst->remove_wait);
 
 	sensor_inst->custom_dev.minor = MISC_DYNAMIC_MINOR;
 	sensor_inst->custom_dev.name = dev_name(&sensor_inst->pdev->dev);
@@ -751,10 +772,12 @@ 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)
 {
-	wake_up(&sensor_inst->wait);
+	WRITE_ONCE(sensor_inst->removing, true);
+	wake_up_all(&sensor_inst->wait);
+	wait_event(sensor_inst->remove_wait,
+		   !test_bit(0, &sensor_inst->misc_opened));
 	misc_deregister(&sensor_inst->custom_dev);
 	kfifo_free(&sensor_inst->data_fifo);
-
 }
 
 /*
-- 
2.56.0

Re: [PATCH] HID: sensor-custom: Synchronize misc-device teardown with file lifetime

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] HID: sensor-custom: race in hid_sensor_custom_open() on removal causes UAF
- [Critical] HID: sensor-custom: UAF on remove_wait in hid_sensor_custom_release()
- [High] HID: sensor-custom: uninterruptible wait in remove() hangs device teardown
--

commit 675e46b087ebefd9837f6bd3d07f849dbaf943f6
Author: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

HID: sensor-custom: Synchronize misc-device teardown with file lifetime

This commit synchronizes the HID sensor custom misc-device teardown with
open file descriptors by adding a removing flag and a remove_wait waitqueue
so that removal waits for the file to close before deregistering the misc
device and freeing the FIFO.

> diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
> index c2b425afd951b..c9338ae47050a 100644
> --- a/drivers/hid/hid-sensor-custom.c
> +++ b/drivers/hid/hid-sensor-custom.c

[ ... ]

> @@ -684,6 +698,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);
> +	wake_up_all(&sensor_inst->remove_wait);

[Severity: Critical]
Can this trigger a use-after-free on sensor_inst->remove_wait in
hid_sensor_custom_release() in drivers/hid/hid-sensor-custom.c when
userspace closes the file descriptor concurrently with device removal?

As soon as hid_sensor_custom_release() executes clear_bit() on
sensor_inst->misc_opened, another CPU in hid_sensor_custom_dev_if_remove()
can observe !test_bit(0, &sensor_inst->misc_opened) in wait_event(), finish
misc_deregister() and kfifo_free(), and return from
hid_sensor_custom_remove(), which allows devres to free sensor_inst:

CPU 0                                 CPU 1
hid_sensor_custom_release()           hid_sensor_custom_remove()
  clear_bit(0,                          hid_sensor_custom_dev_if_remove()
    &sensor_inst->misc_opened);           wait_event(sensor_inst->remove_wait,
                                            !test_bit(0, &sensor_inst->misc_opened));
                                          misc_deregister(&sensor_inst->custom_dev);
                                          kfifo_free(&sensor_inst->data_fifo);
                                        /* devm frees sensor_inst */
  wake_up_all(&sensor_inst->remove_wait); /* UAF on sensor_inst->remove_wait */

In addition, clear_bit() is a relaxed atomic operation and does not provide
a memory barrier before wake_up_all().

>  
>  	return 0;
>  }
> @@ -694,6 +709,8 @@ static int hid_sensor_custom_open(struct inode *inode, struct file *file)
>  
>  	sensor_inst = container_of(file->private_data,
>  				   struct hid_sensor_custom, custom_dev);
> +	if (READ_ONCE(sensor_inst->removing))
> +		return -ENODEV;
>  	/* We essentially have single reader and writer */
>  	if (test_and_set_bit(0, &sensor_inst->misc_opened))
>  		return -EBUSY;

[Severity: Critical]
What prevents hid_sensor_custom_open() in drivers/hid/hid-sensor-custom.c
from racing with hid_sensor_custom_dev_if_remove() when userspace opens the
misc device concurrently with removal?

Because misc_deregister(&sensor_inst->custom_dev) is called after
WRITE_ONCE(sensor_inst->removing, true) and wait_event(), misc_open() can
still invoke hid_sensor_custom_open() while teardown is starting. Checking
sensor_inst->removing and setting bit 0 in sensor_inst->misc_opened are not
serialized against hid_sensor_custom_dev_if_remove(), nor is
sensor_inst->removing rechecked after test_and_set_bit():

CPU 0                                 CPU 1
hid_sensor_custom_open()              hid_sensor_custom_dev_if_remove()
  if (READ_ONCE(sensor_inst->removing))
    /* sees false */
                                        WRITE_ONCE(sensor_inst->removing, true);
                                        wake_up_all(&sensor_inst->wait);
                                        wait_event(sensor_inst->remove_wait,
                                          !test_bit(0, &sensor_inst->misc_opened));
                                        /* misc_opened is still 0, returns */
  test_and_set_bit(0,
    &sensor_inst->misc_opened);
  stream_open(inode, file);
                                        misc_deregister(&sensor_inst->custom_dev);
                                        kfifo_free(&sensor_inst->data_fifo);
                                        /* devm frees sensor_inst */

Wouldn't this leave userspace with an open file descriptor whose
file->private_data points into freed sensor_inst and freed
sensor_inst->data_fifo on subsequent hid_sensor_custom_read(),
hid_sensor_custom_poll(), or hid_sensor_custom_release() calls?

[ ... ]

> @@ -751,10 +772,12 @@ 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)
>  {
> -	wake_up(&sensor_inst->wait);
> +	WRITE_ONCE(sensor_inst->removing, true);
> +	wake_up_all(&sensor_inst->wait);
> +	wait_event(sensor_inst->remove_wait,
> +		   !test_bit(0, &sensor_inst->misc_opened));
>  	misc_deregister(&sensor_inst->custom_dev);

[Severity: High]
Won't waiting with wait_event() in hid_sensor_custom_dev_if_remove() in
drivers/hid/hid-sensor-custom.c hang device removal in uninterruptible sleep
if a userspace process holds an open file descriptor without immediately
closing it?

When hid_sensor_custom_remove() calls hid_sensor_custom_dev_if_remove(), the
removing thread blocks in TASK_UNINTERRUPTIBLE until userspace invokes
close() and triggers hid_sensor_custom_release(). A process holding the fd
open may be idle, sleeping on another event, stopped, or waiting for a udev
remove notification before closing the device.

Furthermore, because misc_deregister(&sensor_inst->custom_dev) is called
only after wait_event() returns, device_destroy() on the misc device and the
removal uevent are delayed until the fd is already closed, which can
deadlock a userspace daemon that relies on the remove uevent to close its
file descriptor.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103015734.578613-1-srinivas.pandruvada@linux.intel.com?part=1

Re: [PATCH] HID: sensor-custom: Synchronize misc-device teardown with file lifetime

[Joshua Crofts]

On 2026-11-02 17:57, Srinivas Pandruvada wrote:
> The HID sensor custom misc device frees its per-sensor state in
> remove() while userspace may still hold an open file descriptor. A
> reader blocked in read() can remain queued on sensor_inst->wait after the
> object has been freed, and a later wakeup races with teardown and
> touches stale memory.
> 
> Synchronize the misc-device teardown with file lifetime before freeing
> the per-sensor state. Set a removal flag, reject new opens, wake
> blocked waiters, and wait for the last open file to close before
> deregistering the misc device and freeing the FIFO. Fail in-flight read()
> and poll() operations cleanly once removal is in progress so userspace
> cannot access freed memory after the device disappears.
> 
> This avoids use-after-free on read(), poll(), release(), and open()
> after the HID sensor custom character device has been removed
> 
> Reported-by: Семён Марьясин <simeon@maryasin.name>

Closes tag?

> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

-- 
Kind regards,
Joshua Crofts