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@xxxxxxxxxxxxx>
> Signed-off-by: Srinivas Pandruvada
> <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> ---
>  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);
> -
>  }
>  
>  /*