| Message ID | 20230125-hid-unregister-leds-v3-2-0a52ac225e00@diag.uniroma1.it |
|---|---|
| State | New |
| Headers | show |
| Series | HID: use spinlocks to safely schedule led workers | expand |
On Feb 09 2023, Pietro Borrello wrote: > Use spinlocks to deal with workers introducing a wrapper > asus_schedule_work(), and several spinlock checks. > Otherwise, asus_kbd_backlight_set() may schedule led->work after the > structure has been freed, causing a use-after-free. > > Fixes: af22a610bc38 ("HID: asus: support backlight on USB keyboards") > Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it> > --- > drivers/hid/hid-asus.c | 24 +++++++++++++++++++++--- > 1 file changed, 21 insertions(+), 3 deletions(-) > > diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c > index f99752b998f3..30e194803bd7 100644 > --- a/drivers/hid/hid-asus.c > +++ b/drivers/hid/hid-asus.c > @@ -98,6 +98,7 @@ struct asus_kbd_leds { > struct hid_device *hdev; > struct work_struct work; > unsigned int brightness; > + spinlock_t lock; > bool removed; > }; > > @@ -490,13 +491,23 @@ static int rog_nkey_led_init(struct hid_device *hdev) > return ret; > } > > +static void asus_schedule_work(struct asus_kbd_leds *led) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&led->lock, flags); > + if (!led->removed) > + schedule_work(&led->work); > + spin_unlock_irqrestore(&led->lock, flags); > +} > + > static void asus_kbd_backlight_set(struct led_classdev *led_cdev, > enum led_brightness brightness) > { > struct asus_kbd_leds *led = container_of(led_cdev, struct asus_kbd_leds, > cdev); > led->brightness = brightness; > - schedule_work(&led->work); > + asus_schedule_work(led); > } > > static enum led_brightness asus_kbd_backlight_get(struct led_classdev *led_cdev) > @@ -512,15 +523,17 @@ static void asus_kbd_backlight_work(struct work_struct *work) > struct asus_kbd_leds *led = container_of(work, struct asus_kbd_leds, work); > u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, 0x00 }; > int ret; > + unsigned long flags; > > - if (led->removed) > - return; > + spin_lock_irqsave(&led->lock, flags); > > buf[4] = led->brightness; > > ret = asus_kbd_set_report(led->hdev, buf, sizeof(buf)); > if (ret < 0) > hid_err(led->hdev, "Asus failed to set keyboard backlight: %d\n", ret); > + > + spin_unlock_irqrestore(&led->lock, flags); Same as in 1/2, please only keep "buf[4] = led->brightness;" under spinlock. Which also raises the question on why the other accesses of led->brightness are not protected by the spinlock :) Note that we could use an atomic to not use the spinlock, but we need the spinlock anyway... Cheers, Benjamin > } > > /* WMI-based keyboard backlight LED control (via asus-wmi driver) takes > @@ -584,6 +597,7 @@ static int asus_kbd_register_leds(struct hid_device *hdev) > drvdata->kbd_backlight->cdev.brightness_set = asus_kbd_backlight_set; > drvdata->kbd_backlight->cdev.brightness_get = asus_kbd_backlight_get; > INIT_WORK(&drvdata->kbd_backlight->work, asus_kbd_backlight_work); > + spin_lock_init(&drvdata->kbd_backlight->lock); > > ret = devm_led_classdev_register(&hdev->dev, &drvdata->kbd_backlight->cdev); > if (ret < 0) { > @@ -1119,9 +1133,13 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id) > static void asus_remove(struct hid_device *hdev) > { > struct asus_drvdata *drvdata = hid_get_drvdata(hdev); > + unsigned long flags; > > if (drvdata->kbd_backlight) { > + spin_lock_irqsave(&drvdata->kbd_backlight->lock, flags); > drvdata->kbd_backlight->removed = true; > + spin_unlock_irqrestore(&drvdata->kbd_backlight->lock, flags); > + > cancel_work_sync(&drvdata->kbd_backlight->work); > } > > > -- > 2.25.1 >
Hi Pietro,
url: https://github.com/intel-lab-lkp/linux/commits/Pietro-Borrello/HID-bigben-use-spinlock-to-safely-schedule-workers/20230210-080058
base: 2241ab53cbb5cdb08a6b2d4688feb13971058f65
patch link: https://lore.kernel.org/r/20230125-hid-unregister-leds-v3-2-0a52ac225e00%40diag.uniroma1.it
patch subject: [PATCH v3 2/2] HID: asus: use spinlock to safely schedule workers
config: riscv-randconfig-m031-20230212 (https://download.01.org/0day-ci/archive/20230214/202302141039.anZLT940-lkp@intel.com/config)
compiler: riscv32-linux-gcc (GCC) 12.1.0
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Link: https://lore.kernel.org/r/202302141039.anZLT940-lkp@intel.com/
smatch warnings:
drivers/hid/hid-asus.c:532 asus_kbd_backlight_work() warn: sleeping in atomic context
vim +532 drivers/hid/hid-asus.c
af22a610bc3850 Carlo Caione 2017-04-06 521 static void asus_kbd_backlight_work(struct work_struct *work)
af22a610bc3850 Carlo Caione 2017-04-06 522 {
af22a610bc3850 Carlo Caione 2017-04-06 523 struct asus_kbd_leds *led = container_of(work, struct asus_kbd_leds, work);
af22a610bc3850 Carlo Caione 2017-04-06 524 u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, 0x00 };
af22a610bc3850 Carlo Caione 2017-04-06 525 int ret;
31e578b641b3b3 Pietro Borrello 2023-02-09 526 unsigned long flags;
af22a610bc3850 Carlo Caione 2017-04-06 527
31e578b641b3b3 Pietro Borrello 2023-02-09 528 spin_lock_irqsave(&led->lock, flags);
^^^^^^^^^^^^^^^^^
Holding a spinlock.
af22a610bc3850 Carlo Caione 2017-04-06 529
af22a610bc3850 Carlo Caione 2017-04-06 530 buf[4] = led->brightness;
af22a610bc3850 Carlo Caione 2017-04-06 531
af22a610bc3850 Carlo Caione 2017-04-06 @532 ret = asus_kbd_set_report(led->hdev, buf, sizeof(buf));
^^^^^^^^^^^^^^^^^^^
asus_kbd_set_report() does a GFP_KERNEL allocation.
af22a610bc3850 Carlo Caione 2017-04-06 533 if (ret < 0)
af22a610bc3850 Carlo Caione 2017-04-06 534 hid_err(led->hdev, "Asus failed to set keyboard backlight: %d\n", ret);
31e578b641b3b3 Pietro Borrello 2023-02-09 535
31e578b641b3b3 Pietro Borrello 2023-02-09 536 spin_unlock_irqrestore(&led->lock, flags);
af22a610bc3850 Carlo Caione 2017-04-06 537 }
On Tue, 14 Feb 2023 at 08:58, Dan Carpenter <error27@gmail.com> wrote: > > Hi Pietro, > > url: https://github.com/intel-lab-lkp/linux/commits/Pietro-Borrello/HID-bigben-use-spinlock-to-safely-schedule-workers/20230210-080058 > base: 2241ab53cbb5cdb08a6b2d4688feb13971058f65 > patch link: https://lore.kernel.org/r/20230125-hid-unregister-leds-v3-2-0a52ac225e00%40diag.uniroma1.it > patch subject: [PATCH v3 2/2] HID: asus: use spinlock to safely schedule workers > config: riscv-randconfig-m031-20230212 (https://download.01.org/0day-ci/archive/20230214/202302141039.anZLT940-lkp@intel.com/config) > compiler: riscv32-linux-gcc (GCC) 12.1.0 > > If you fix the issue, kindly add following tag where applicable > | Reported-by: kernel test robot <lkp@intel.com> > | Reported-by: Dan Carpenter <error27@gmail.com> > | Link: https://lore.kernel.org/r/202302141039.anZLT940-lkp@intel.com/ > > smatch warnings: > drivers/hid/hid-asus.c:532 asus_kbd_backlight_work() warn: sleeping in atomic context > > vim +532 drivers/hid/hid-asus.c > > af22a610bc3850 Carlo Caione 2017-04-06 521 static void asus_kbd_backlight_work(struct work_struct *work) > af22a610bc3850 Carlo Caione 2017-04-06 522 { > af22a610bc3850 Carlo Caione 2017-04-06 523 struct asus_kbd_leds *led = container_of(work, struct asus_kbd_leds, work); > af22a610bc3850 Carlo Caione 2017-04-06 524 u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, 0x00 }; > af22a610bc3850 Carlo Caione 2017-04-06 525 int ret; > 31e578b641b3b3 Pietro Borrello 2023-02-09 526 unsigned long flags; > af22a610bc3850 Carlo Caione 2017-04-06 527 > 31e578b641b3b3 Pietro Borrello 2023-02-09 528 spin_lock_irqsave(&led->lock, flags); > ^^^^^^^^^^^^^^^^^ > Holding a spinlock. > > af22a610bc3850 Carlo Caione 2017-04-06 529 > af22a610bc3850 Carlo Caione 2017-04-06 530 buf[4] = led->brightness; > af22a610bc3850 Carlo Caione 2017-04-06 531 > af22a610bc3850 Carlo Caione 2017-04-06 @532 ret = asus_kbd_set_report(led->hdev, buf, sizeof(buf)); > ^^^^^^^^^^^^^^^^^^^ > asus_kbd_set_report() does a GFP_KERNEL allocation. > > > af22a610bc3850 Carlo Caione 2017-04-06 533 if (ret < 0) > af22a610bc3850 Carlo Caione 2017-04-06 534 hid_err(led->hdev, "Asus failed to set keyboard backlight: %d\n", ret); > 31e578b641b3b3 Pietro Borrello 2023-02-09 535 > 31e578b641b3b3 Pietro Borrello 2023-02-09 536 spin_unlock_irqrestore(&led->lock, flags); > af22a610bc3850 Carlo Caione 2017-04-06 537 } > > -- > 0-DAY CI Kernel Test Service > https://github.com/intel/lkp-tests > Hello, Thank you, but the issue has already been fixed in v4. Link: https://lore.kernel.org/lkml/20230125-hid-unregister-leds-v4-3-7860c5763c38@diag.uniroma1.it/T/#m8983ca3d0fbf656a40011a77c8988e3d16cac671 Best regards, Pietro
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c index f99752b998f3..30e194803bd7 100644 --- a/drivers/hid/hid-asus.c +++ b/drivers/hid/hid-asus.c @@ -98,6 +98,7 @@ struct asus_kbd_leds { struct hid_device *hdev; struct work_struct work; unsigned int brightness; + spinlock_t lock; bool removed; }; @@ -490,13 +491,23 @@ static int rog_nkey_led_init(struct hid_device *hdev) return ret; } +static void asus_schedule_work(struct asus_kbd_leds *led) +{ + unsigned long flags; + + spin_lock_irqsave(&led->lock, flags); + if (!led->removed) + schedule_work(&led->work); + spin_unlock_irqrestore(&led->lock, flags); +} + static void asus_kbd_backlight_set(struct led_classdev *led_cdev, enum led_brightness brightness) { struct asus_kbd_leds *led = container_of(led_cdev, struct asus_kbd_leds, cdev); led->brightness = brightness; - schedule_work(&led->work); + asus_schedule_work(led); } static enum led_brightness asus_kbd_backlight_get(struct led_classdev *led_cdev) @@ -512,15 +523,17 @@ static void asus_kbd_backlight_work(struct work_struct *work) struct asus_kbd_leds *led = container_of(work, struct asus_kbd_leds, work); u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, 0x00 }; int ret; + unsigned long flags; - if (led->removed) - return; + spin_lock_irqsave(&led->lock, flags); buf[4] = led->brightness; ret = asus_kbd_set_report(led->hdev, buf, sizeof(buf)); if (ret < 0) hid_err(led->hdev, "Asus failed to set keyboard backlight: %d\n", ret); + + spin_unlock_irqrestore(&led->lock, flags); } /* WMI-based keyboard backlight LED control (via asus-wmi driver) takes @@ -584,6 +597,7 @@ static int asus_kbd_register_leds(struct hid_device *hdev) drvdata->kbd_backlight->cdev.brightness_set = asus_kbd_backlight_set; drvdata->kbd_backlight->cdev.brightness_get = asus_kbd_backlight_get; INIT_WORK(&drvdata->kbd_backlight->work, asus_kbd_backlight_work); + spin_lock_init(&drvdata->kbd_backlight->lock); ret = devm_led_classdev_register(&hdev->dev, &drvdata->kbd_backlight->cdev); if (ret < 0) { @@ -1119,9 +1133,13 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id) static void asus_remove(struct hid_device *hdev) { struct asus_drvdata *drvdata = hid_get_drvdata(hdev); + unsigned long flags; if (drvdata->kbd_backlight) { + spin_lock_irqsave(&drvdata->kbd_backlight->lock, flags); drvdata->kbd_backlight->removed = true; + spin_unlock_irqrestore(&drvdata->kbd_backlight->lock, flags); + cancel_work_sync(&drvdata->kbd_backlight->work); }
Use spinlocks to deal with workers introducing a wrapper asus_schedule_work(), and several spinlock checks. Otherwise, asus_kbd_backlight_set() may schedule led->work after the structure has been freed, causing a use-after-free. Fixes: af22a610bc38 ("HID: asus: support backlight on USB keyboards") Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it> --- drivers/hid/hid-asus.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-)