[PATCH 2/2] HID: corsair: cancel worker before unregistering LED to fix use-after-free

Chen Changcheng <[email protected]>
Newsgroups org.kernel.vger.linux-input,org.kernel.vger.linux-kernel
Message-ID <[email protected]>
The cleanup functions k90_cleanup_backlight() and
k90_cleanup_macro_functions() call led_classdev_unregister() before
cancel_work_sync():

    led_classdev_unregister()     <-- may free led->cdev.dev
    cancel_work_sync()            <-- wait for worker

If the LED worker (k90_backlight_work / k90_record_led_work) is
already running on another CPU, the following race can occur:

    CPU 1 (worker)               CPU 2 (remove)
    ---------------------         --------------------
    if (led->removed) -> false
    (passed the guard, about to read led->cdev.dev)
    * preempted
                                   removed = true
                                   led_classdev_unregister()
                                     -> led->cdev.dev freed
                                   cancel_work_sync()
                                     -> waits for worker
    * resumes
    dev = led->cdev.dev->parent   <-- UAF!

Fix by swapping the order so that the worker is cancelled first:

    cancel_work_sync()            <-- wait for worker first
    led_classdev_unregister()     <-- then safe to unregister

The removed flag is set before cancel_work_sync() so that if
led_classdev_unregister() internally triggers another brightness
update (which re-schedules the work), the worker will see the flag
and return immediately.

Signed-off-by: Chen Changcheng <[email protected]>
Reported-by: sashiko-bot <[email protected]>
---
 drivers/hid/hid-corsair.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-corsair.c b/drivers/hid/hid-corsair.c
index ac12877a6db7..278c6efb565d 100644
--- a/drivers/hid/hid-corsair.c
+++ b/drivers/hid/hid-corsair.c
@@ -524,8 +524,8 @@ static void k90_cleanup_backlight(struct hid_device *dev)
 
 	if (drvdata->backlight) {
 		drvdata->backlight->removed = true;
-		led_classdev_unregister(&drvdata->backlight->cdev);
 		cancel_work_sync(&drvdata->backlight->work);
+		led_classdev_unregister(&drvdata->backlight->cdev);
 		kfree(drvdata->backlight->cdev.name);
 		kfree(drvdata->backlight);
 	}
@@ -540,8 +540,8 @@ static void k90_cleanup_macro_functions(struct hid_device *dev)
 		sysfs_remove_group(&dev->dev.kobj, &k90_attr_group);
 
 		k90->record_led.removed = true;
-		led_classdev_unregister(&k90->record_led.cdev);
 		cancel_work_sync(&k90->record_led.work);
+		led_classdev_unregister(&k90->record_led.cdev);
 		kfree(k90->record_led.cdev.name);
 
 		kfree(k90);
-- 
2.25.1
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.