[PATCH] HID: letsketch: fix UAF on inrange_timer at driver unbind

[PATCH] HID: letsketch: fix UAF on inrange_timer at driver unbind

[Manish Khadka]

letsketch_driver does not provide a .remove callback, but
letsketch_probe() arms a per-device timer:

    timer_setup(&data->inrange_timer, letsketch_inrange_timeout, 0);

The timer is re-armed from letsketch_raw_event() with a 100 ms
timeout on every pen-in-range report, and its callback dereferences
data->input_tablet to deliver a synthetic BTN_TOOL_PEN release:

    static void letsketch_inrange_timeout(struct timer_list *t)
    {
            struct letsketch_data *data =
                    timer_container_of(data, t, inrange_timer);
            struct input_dev *input = data->input_tablet;

            input_report_key(input, BTN_TOOL_PEN, 0);
            input_sync(input);
    }

letsketch_data is allocated with devm_kzalloc(), and its input_dev
fields are devm-allocated via letsketch_setup_input_tablet().  On
device unbind (USB unplug or rmmod), the HID core runs its default
teardown and devm cleanup frees both letsketch_data and the input
devices.  Because no .remove callback exists, nothing drains the
timer first: if raw_event armed it within ~100 ms of the unbind, the
pending timer fires on freed memory.  This is a UAF read of data and
of data->input_tablet, followed by input_report_key() / input_sync()
into the freed input_dev.

Fix by adding a .remove callback that drains the timer before
hid_hw_stop().  timer_delete_sync() on its own is not sufficient: a
URB completion still in flight during hid_hw_stop() can call
mod_timer() and re-arm the timer after the drain.

Introduce a spinlock and a 'removing' flag on letsketch_data.
letsketch_remove() sets the flag under the lock, then drains the
timer, then stops the hardware.  letsketch_raw_event() now arms
mod_timer() under the same lock and skips arming if the flag is set,
so no path can re-arm the timer after remove() has drained it.

Fixes: 33a5c2793451 ("HID: Add new Letsketch tablet driver")
Cc: stable@vger.kernel.org
Signed-off-by: Manish Khadka <maskmemanish@gmail.com>
---
 drivers/hid/hid-letsketch.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-letsketch.c b/drivers/hid/hid-letsketch.c
index 11e21f988723..0dc9496d05f8 100644
--- a/drivers/hid/hid-letsketch.c
+++ b/drivers/hid/hid-letsketch.c
@@ -36,6 +36,7 @@
  */
 #include <linux/device.h>
 #include <linux/input.h>
+#include <linux/cleanup.h>
 #include <linux/hid.h>
 #include <linux/module.h>
 #include <linux/timer.h>
@@ -62,6 +63,8 @@ struct letsketch_data {
 	struct input_dev *input_tablet;
 	struct input_dev *input_tablet_pad;
 	struct timer_list inrange_timer;
+	spinlock_t lock;	/* serialises arming inrange_timer vs. teardown */
+	bool removing;		/* set during teardown; gates mod_timer */
 };
 
 static int letsketch_open(struct input_dev *dev)
@@ -189,9 +192,15 @@ static int letsketch_raw_event(struct hid_device *hdev,
 				 get_unaligned_le16(raw_data + 6));
 		/*
 		 * There is no out of range event, so use a timer for this
-		 * when in range we get an event approx. every 8 ms.
+		 * when in range we get an event approx. every 8 ms.  Skip
+		 * arming if the driver is being torn down so the timer
+		 * cannot outlive devm-freed data after letsketch_remove().
 		 */
-		mod_timer(&data->inrange_timer, jiffies + msecs_to_jiffies(100));
+		scoped_guard(spinlock_irqsave, &data->lock) {
+			if (!data->removing)
+				mod_timer(&data->inrange_timer,
+					  jiffies + msecs_to_jiffies(100));
+		}
 		break;
 	case 0xe0: /* Pad data */
 		input = data->input_tablet_pad;
@@ -291,6 +300,7 @@ static int letsketch_probe(struct hid_device *hdev, const struct hid_device_id *
 		return -ENOMEM;
 
 	data->hdev = hdev;
+	spin_lock_init(&data->lock);
 	timer_setup(&data->inrange_timer, letsketch_inrange_timeout, 0);
 	hid_set_drvdata(hdev, data);
 
@@ -305,6 +315,23 @@ static int letsketch_probe(struct hid_device *hdev, const struct hid_device_id *
 	return hid_hw_start(hdev, HID_CONNECT_HIDRAW);
 }
 
+static void letsketch_remove(struct hid_device *hdev)
+{
+	struct letsketch_data *data = hid_get_drvdata(hdev);
+
+	/*
+	 * Block raw_event from arming inrange_timer during teardown so
+	 * timer_delete_sync() below cannot race with a fresh mod_timer()
+	 * issued from a URB completion handler still in flight while
+	 * hid_hw_stop() is running.
+	 */
+	scoped_guard(spinlock_irqsave, &data->lock)
+		data->removing = true;
+
+	timer_delete_sync(&data->inrange_timer);
+	hid_hw_stop(hdev);
+}
+
 static const struct hid_device_id letsketch_devices[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LETSKETCH, USB_DEVICE_ID_WP9620N) },
 	{ }
@@ -315,6 +342,7 @@ static struct hid_driver letsketch_driver = {
 	.name = "letsketch",
 	.id_table = letsketch_devices,
 	.probe = letsketch_probe,
+	.remove = letsketch_remove,
 	.raw_event = letsketch_raw_event,
 };
 module_hid_driver(letsketch_driver);
-- 
2.43.0

Re: [PATCH] HID: letsketch: fix UAF on inrange_timer at driver unbind

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing timer cleanup on the probe error path leads to a Use-After-Free.
- [Medium] Unnecessary spinlock and state flag introduced instead of using `timer_shutdown_sync()`.
--

commit 2d88d7acbcf7e811559cfcb0df7e33f55178711b
Author: Manish Khadka <maskmemanish@gmail.com>

HID: letsketch: fix UAF on inrange_timer at driver unbind

This commit adds a remove callback to the letsketch driver to clean up
the inrange_timer during device unbind. It introduces a spinlock and a
removing flag to prevent the timer from being re-armed by in-flight
events while teardown is occurring.

> diff --git a/drivers/hid/hid-letsketch.c b/drivers/hid/hid-letsketch.c
> index 11e21f9887232..0dc9496d05f80 100644
> --- a/drivers/hid/hid-letsketch.c
> +++ b/drivers/hid/hid-letsketch.c

[ ... ]

> @@ -305,6 +315,23 @@ static int letsketch_probe(struct hid_device *hdev, const struct hid_device_id *
>  	return hid_hw_start(hdev, HID_CONNECT_HIDRAW);
>  }

While not introduced by this patch, if hid_hw_start() fails and returns an
error, does this leave a pending timer that will fire on freed memory?

Looking at hid_hw_start(), it enables hardware I/O before attempting to
connect. If the connection fails, it stops I/O and returns an error:

hid_hw_start() {
        hdev->ll_driver->start(hdev);
        if (hid_connect(hdev, connect_mask)) {
                hdev->ll_driver->stop(hdev);
                return error;
        }
}

During the window where I/O is active, an incoming event could trigger
letsketch_raw_event() and arm the timer.

When letsketch_probe() directly returns this error, devres immediately frees
the letsketch_data structure without draining the timer.

> +static void letsketch_remove(struct hid_device *hdev)
> +{
> +	struct letsketch_data *data = hid_get_drvdata(hdev);
> +
> +	/*
> +	 * Block raw_event from arming inrange_timer during teardown so
> +	 * timer_delete_sync() below cannot race with a fresh mod_timer()
> +	 * issued from a URB completion handler still in flight while
> +	 * hid_hw_stop() is running.
> +	 */
> +	scoped_guard(spinlock_irqsave, &data->lock)
> +		data->removing = true;
> +
> +	timer_delete_sync(&data->inrange_timer);
> +	hid_hw_stop(hdev);
> +}

Could this synchronization be achieved without adding a new spinlock and state
flag by using timer_shutdown_sync() instead?

The timer_shutdown_sync() API permanently disables the timer, ensuring that
any subsequent mod_timer() calls are silent no-ops.

Alternatively, if hid_hw_stop() were called before cleaning up the timer, it
would wait for all URBs to complete, which structurally guarantees no new
events can arrive to re-arm the timer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515153551.76162-1-maskmemanish@gmail.com?part=1

[PATCH v2] HID: letsketch: fix UAF on inrange_timer at driver unbind

[Manish Khadka]

letsketch_driver does not provide a .remove callback, but
letsketch_probe() arms a per-device timer:

    timer_setup(&data->inrange_timer, letsketch_inrange_timeout, 0);

The timer is re-armed from letsketch_raw_event() with a 100 ms
timeout on every pen-in-range report, and its callback dereferences
data->input_tablet to deliver a synthetic BTN_TOOL_PEN release.

letsketch_data is allocated with devm_kzalloc(), and its input_dev
fields are devm-allocated via letsketch_setup_input_tablet().  On
device unbind (USB unplug or rmmod), the HID core runs its default
teardown and devm cleanup frees both letsketch_data and the input
devices.  Because no .remove callback exists, nothing drains the
timer first: if raw_event armed it within ~100 ms of the unbind,
the pending timer fires on freed memory.  This is a UAF read of
data and of data->input_tablet, followed by input_report_key() /
input_sync() into the freed input_dev.

The same problem can occur on the probe error path: if
hid_hw_start() enabled I/O on an always-poll-quirk device and then
failed, raw_event may have armed the timer before devm releases
data.

Fix by adding a .remove callback that calls hid_hw_stop() first.
hid_hw_stop() synchronously kills the URBs that deliver raw_event(),
so once it returns no path can re-arm the timer.  timer_shutdown_sync()
then drains any in-flight callback and permanently disables further
mod_timer() calls.  Apply the same timer_shutdown_sync() in the probe
error path so the timer is guaranteed not to outlive data.

Fixes: 33a5c2793451 ("HID: Add new Letsketch tablet driver")
Cc: stable@vger.kernel.org
Signed-off-by: Manish Khadka <maskmemanish@gmail.com>
---

v1 -> v2:
  - Drop the spinlock_t and bool removing fields and their uses
    (and the <linux/cleanup.h> include): hid_hw_stop() synchronously
    kills URBs, so once it returns no raw_event can re-arm the timer
    and an explicit gate is unnecessary.
  - Use timer_shutdown_sync() in place of timer_delete_sync(), which
    also makes the probe error path safe against a stray mod_timer()
    that an always-poll-quirk hid_hw_start() failure could leave
    behind.
  - Drain the timer in the probe error path via a unified
    err_shutdown_timer label.
  Thanks to Sashiko AI review for both points.
 drivers/hid/hid-letsketch.c | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-letsketch.c b/drivers/hid/hid-letsketch.c
index 11e21f988723..b52e93a91ae5 100644
--- a/drivers/hid/hid-letsketch.c
+++ b/drivers/hid/hid-letsketch.c
@@ -296,13 +296,42 @@ static int letsketch_probe(struct hid_device *hdev, const struct hid_device_id *
 
 	ret = letsketch_setup_input_tablet(data);
 	if (ret)
-		return ret;
+		goto err_shutdown_timer;
 
 	ret = letsketch_setup_input_tablet_pad(data);
 	if (ret)
-		return ret;
+		goto err_shutdown_timer;
+
+	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+	if (ret)
+		goto err_shutdown_timer;
 
-	return hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+	return 0;
+
+err_shutdown_timer:
+	/*
+	 * Drain any pending callback and permanently disable the timer
+	 * before devm releases data: if hid_hw_start() enabled I/O on an
+	 * always-poll-quirk device and then failed, raw_event may have
+	 * armed the timer already.
+	 */
+	timer_shutdown_sync(&data->inrange_timer);
+	return ret;
+}
+
+static void letsketch_remove(struct hid_device *hdev)
+{
+	struct letsketch_data *data = hid_get_drvdata(hdev);
+
+	/*
+	 * hid_hw_stop() synchronously kills the URBs that deliver
+	 * raw_event(), so once it returns no path can re-arm
+	 * inrange_timer.  timer_shutdown_sync() then drains any
+	 * in-flight callback and permanently disables further
+	 * mod_timer() calls before devm releases data.
+	 */
+	hid_hw_stop(hdev);
+	timer_shutdown_sync(&data->inrange_timer);
 }
 
 static const struct hid_device_id letsketch_devices[] = {
@@ -315,6 +344,7 @@ static struct hid_driver letsketch_driver = {
 	.name = "letsketch",
 	.id_table = letsketch_devices,
 	.probe = letsketch_probe,
+	.remove = letsketch_remove,
 	.raw_event = letsketch_raw_event,
 };
 module_hid_driver(letsketch_driver);
-- 
2.43.0