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

[sashiko-bot@kernel.org]

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