[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