[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

[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

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

[srinivas pandruvada]

On Mon, 2026-11-02 at 17:57 -0800, 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
> 
Hi Simeon,

Shashiko found issue with this change, so hold on for testing.

Thanks,
Srinivas

> 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
>  								*sen
> sor_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);
> -
>  }
>  
>  /*