[PATCH] HID: corsair: fix use-after-free by reordering remove sequence

Chen Changcheng posted 1 patch 12 hours ago
drivers/hid/hid-corsair.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
[PATCH] HID: corsair: fix use-after-free by reordering remove sequence
Posted by Chen Changcheng 12 hours ago
On device removal corsair_remove() currently does:
    k90_cleanup_macro_functions()     ─┐
        kfree(k90)                     │  k90 freed, drvdata->k90
                                       │  is now a dangling pointer
    k90_cleanup_backlight()            │
    hid_hw_stop(dev)                  ─┘  HID I/O finally stopped
The problem is that kfree happens before hid_hw_stop.  Between them,
corsair_event() can still be invoked on another CPU:

    CPU 0 (remove path)                  CPU 1 (USB interrupt)
    ──────────────────                   ────────────────────
    kfree(k90)
                                          corsair_event()
    ↓                                      if (!drvdata->k90)
    ↓                                        return 0;   ← non-NULL!
    ↓                                      drvdata->k90->record_led
    ↓                                        .brightness = x;
    ↓                                      ^^^^^^^^^^^^^^^^^^^^^^^^
    ↓                                      UAF write into freed slab
    hid_hw_stop(dev)
      hid_disconnect()
        clear claimed flags
      usbhid_stop()
        kill URBs

The NULL check in corsair_event() is ineffective because
k90_cleanup_macro_functions() never clears drvdata->k90 after kfree.

Fix by reordering so hid_hw_stop() runs first, eliminating the window:
    hid_hw_stop(dev)                  ─┐  HID I/O stopped first,
    k90_cleanup_macro_functions()      │  no more events can arrive
    k90_cleanup_backlight()           ─┘

Set drvdata->k90 = NULL after kfree() as a defensive measure, matching
the existing pattern in k90_init_macro_functions()'s error path.

Signed-off-by: Chen Changcheng <chenchangcheng@kylinos.cn>
---
 drivers/hid/hid-corsair.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-corsair.c b/drivers/hid/hid-corsair.c
index 21cd8b12a757..ac12877a6db7 100644
--- a/drivers/hid/hid-corsair.c
+++ b/drivers/hid/hid-corsair.c
@@ -545,6 +545,7 @@ static void k90_cleanup_macro_functions(struct hid_device *dev)
 		kfree(k90->record_led.cdev.name);
 
 		kfree(k90);
+		drvdata->k90 = NULL;
 	}
 }
 
@@ -596,10 +597,10 @@ static int corsair_probe(struct hid_device *dev, const struct hid_device_id *id)
 
 static void corsair_remove(struct hid_device *dev)
 {
+	hid_hw_stop(dev);
+
 	k90_cleanup_macro_functions(dev);
 	k90_cleanup_backlight(dev);
-
-	hid_hw_stop(dev);
 }
 
 static int corsair_event(struct hid_device *dev, struct hid_field *field,
-- 
2.25.1