I noticed a recurring pattern is present in multiple hid devices in the
Linux tree, where the LED controller of a device schedules a work_struct
to interact with the hardware.
The work_struct is embedded in the device structure and thus, is freed
at device removal.
The issue is that a LED worker may be scheduled by a timer concurrently
with device removal, causing the work_struct to be accessed after having
been freed.
I was able to trigger the issue in hid-bigbenff.c and hid-asus.c
where the work_structs may be scheduled by the LED controller
while the device is disconnecting, triggering use-after-frees.
I can attach the reproducer, but it's very simple USB configuration,
using the /dev/raw-gadget interface with some more USB interactions
to manage LEDs configuration and pass checks in asus_kbd_init()
and asus_kbd_get_functions() in case of hid-asus.c.
I triggered the issue by connecting a device and immediately
disconnecting it, so that the remove function runs before the LED one
which remains pending.
I am attaching multiple patches for asus and bigben drivers.
The proposed patches introduce safe wrappers to schedule the workers
safely with several spinlocks checks.
I attach the (partial for brevity) ODEBUG dumps:
```hid-bigbenff.c
[ 37.803135][ T1170] usb 1-1: USB disconnect, device number 2
[ 37.827979][ T1170] ODEBUG: free active (active state 0) object
type: work_struct hint: bigben_worker+0x0/0x860
[ 37.829634][ T1170] WARNING: CPU: 0 PID: 1170 at
lib/debugobjects.c:505 debug_check_no_obj_freed+0x43a/0x630
[ 37.830904][ T1170] Modules linked in:
[ 37.831413][ T1170] CPU: 0 PID: 1170 Comm: kworker/0:3 Not tainted
6.1.0-rc4-dirty #43
[ 37.832465][ T1170] Hardware name: QEMU Standard PC (i440FX + PIIX,
1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
[ 37.833751][ T1170] Workqueue: usb_hub_wq hub_event
[ 37.834409][ T1170] RIP: 0010:debug_check_no_obj_freed+0x43a/0x630
[ 37.835218][ T1170] Code: 48 89 ef e8 28 82 58 ff 49 8b 14 24 4c 8b
45 00 48 c7 c7 40 5f 09 87 48 c7 c6 60 5b 09 87 89 d9 4d 89 f9 31 c0
e8 46 25 ef fe <0f> 0b 4c 8b 64 24 20 48 ba 00 00 00 00 00 fc ff df ff
05 4f 7c 17
[ 37.837667][ T1170] RSP: 0018:ffffc900006fee60 EFLAGS: 00010246
[ 37.838503][ T1170] RAX: 0d2d19ffcded3d00 RBX: 0000000000000000
RCX: ffff888117fc9b00
[ 37.839519][ T1170] RDX: 0000000000000000 RSI: 0000000000000000
RDI: 0000000000000000
[ 37.840570][ T1170] RBP: ffffffff86e88380 R08: ffffffff8130793b
R09: fffff520000dfd85
[ 37.841618][ T1170] R10: fffff520000dfd85 R11: 0000000000000000
R12: ffffffff87095fb8
[ 37.842649][ T1170] R13: ffff888117770ad8 R14: ffff888117770acc
R15: ffffffff852b7420
[ 37.843728][ T1170] FS: 0000000000000000(0000)
GS:ffff8881f6600000(0000) knlGS:0000000000000000
[ 37.844877][ T1170] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 37.845749][ T1170] CR2: 00007f992eaab380 CR3: 000000011834b000
CR4: 00000000001006f0
[ 37.846794][ T1170] Call Trace:
[ 37.847245][ T1170] <TASK>
[ 37.847643][ T1170] slab_free_freelist_hook+0x89/0x160
[ 37.848409][ T1170] ? devres_release_all+0x262/0x350
[ 37.849156][ T1170] __kmem_cache_free+0x71/0x110
[ 37.849829][ T1170] devres_release_all+0x262/0x350
[ 37.850478][ T1170] ? devres_release+0x90/0x90
[ 37.851118][ T1170] device_release_driver_internal+0x5e5/0x8a0
[ 37.851944][ T1170] bus_remove_device+0x2ea/0x400
[ 37.852611][ T1170] device_del+0x64f/0xb40
[ 37.853212][ T1170] ? kill_device+0x150/0x150
[ 37.853831][ T1170] ? print_irqtrace_events+0x1f0/0x1f0
[ 37.854564][ T1170] hid_destroy_device+0x66/0x100
[ 37.855226][ T1170] usbhid_disconnect+0x9a/0xc0
[ 37.855887][ T1170] usb_unbind_interface+0x1e1/0x890
```
``` hid-asus.c
[ 77.409878][ T1169] usb 1-1: USB disconnect, device number 2
[ 77.423606][ T1169] ODEBUG: free active (active state 0) object
type: work_struct hint: asus_kbd_backlight_work+0x0/0x2c0
[ 77.425222][ T1169] WARNING: CPU: 0 PID: 1169 at
lib/debugobjects.c:505 debug_check_no_obj_freed+0x43a/0x630
[ 77.426599][ T1169] Modules linked in:
[ 77.427322][ T1169] CPU: 0 PID: 1169 Comm: kworker/0:3 Not tainted
6.1.0-rc4-dirty #43
[ 77.428404][ T1169] Hardware name: QEMU Standard PC (i440FX + PIIX,
1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
[ 77.429644][ T1169] Workqueue: usb_hub_wq hub_event
[ 77.430296][ T1169] RIP: 0010:debug_check_no_obj_freed+0x43a/0x630
[ 77.431142][ T1169] Code: 48 89 ef e8 28 82 58 ff 49 8b 14 24 4c 8b
45 00 48 c7 c7 40 5f 09 87 48 c7 c6 60 5b 09 87 89 d9 4d 89 f9 31 c0
e8 46 25 ef fe <0f> 0b 4c 8b 64 24 20 48 ba 00 00 00 00 00 fc ff df ff
05 4f 7c 17
[ 77.433691][ T1169] RSP: 0018:ffffc9000069ee60 EFLAGS: 00010246
[ 77.434470][ T1169] RAX: b85d2b40c12d7600 RBX: 0000000000000000
RCX: ffff888117a78000
[ 77.435507][ T1169] RDX: 0000000000000000 RSI: 0000000080000000
RDI: 0000000000000000
[ 77.436521][ T1169] RBP: ffffffff86e88380 R08: ffffffff8130793b
R09: ffffed103ecc4ed6
[ 77.437582][ T1169] R10: ffffed103ecc4ed6 R11: 0000000000000000
R12: ffffffff87095fb8
[ 77.438593][ T1169] R13: ffff88810e348fe0 R14: ffff88810e348fd4
R15: ffffffff852b5780
[ 77.439667][ T1169] FS: 0000000000000000(0000)
GS:ffff8881f6600000(0000) knlGS:0000000000000000
[ 77.440842][ T1169] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 77.441688][ T1169] CR2: 00007ffc05495ff0 CR3: 000000010cdf0000
CR4: 00000000001006f0
[ 77.442720][ T1169] Call Trace:
[ 77.443167][ T1169] <TASK>
[ 77.443555][ T1169] slab_free_freelist_hook+0x89/0x160
[ 77.444302][ T1169] ? devres_release_all+0x262/0x350
[ 77.444990][ T1169] __kmem_cache_free+0x71/0x110
[ 77.445638][ T1169] devres_release_all+0x262/0x350
[ 77.446309][ T1169] ? devres_release+0x90/0x90
[ 77.446978][ T1169] device_release_driver_internal+0x5e5/0x8a0
[ 77.447748][ T1169] bus_remove_device+0x2ea/0x400
[ 77.448421][ T1169] device_del+0x64f/0xb40
[ 77.448976][ T1169] ? kill_device+0x150/0x150
[ 77.449577][ T1169] ? print_irqtrace_events+0x1f0/0x1f0
[ 77.450307][ T1169] hid_destroy_device+0x66/0x100
[ 77.450938][ T1169] usbhid_disconnect+0x9a/0xc0
```
Signed-off-by: Pietro Borrello <[email protected]>
---
Changes in v3:
- use spinlocks to prevent workers scheduling
- drop patches on sony & playstation hid drivers
- Link to v2: https://lore.kernel.org/r/[email protected]
Changes in v2:
- dualshock4: Clarify UAF
- dualsense: Clarify UAF
- dualsense: Unregister multicolor led controller
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Pietro Borrello (2):
HID: bigben: use spinlock to safely schedule workers
HID: asus: use spinlock to safely schedule workers
drivers/hid/hid-asus.c | 24 +++++++++++++++++++++---
drivers/hid/hid-bigbenff.c | 34 +++++++++++++++++++++++++++++-----
2 files changed, 50 insertions(+), 8 deletions(-)
---
base-commit: 2241ab53cbb5cdb08a6b2d4688feb13971058f65
change-id: 20230125-hid-unregister-leds-4cbf67099e1d
Best regards,
--
Pietro Borrello <[email protected]>
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 <[email protected]>
---
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);
}
/* 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
Use spinlocks to deal with workers introducing a wrapper
bigben_schedule_work(), and several spinlock checks.
Otherwise, bigben_set_led() may schedule bigben->worker after the
structure has been freed, causing a use-after-free.
Fixes: 4eb1b01de5b9 ("HID: hid-bigbenff: fix race condition for scheduled work during removal")
Signed-off-by: Pietro Borrello <[email protected]>
---
drivers/hid/hid-bigbenff.c | 34 +++++++++++++++++++++++++++++-----
1 file changed, 29 insertions(+), 5 deletions(-)
diff --git a/drivers/hid/hid-bigbenff.c b/drivers/hid/hid-bigbenff.c
index e8b16665860d..28769aa7fed6 100644
--- a/drivers/hid/hid-bigbenff.c
+++ b/drivers/hid/hid-bigbenff.c
@@ -174,6 +174,7 @@ static __u8 pid0902_rdesc_fixed[] = {
struct bigben_device {
struct hid_device *hid;
struct hid_report *report;
+ spinlock_t lock;
bool removed;
u8 led_state; /* LED1 = 1 .. LED4 = 8 */
u8 right_motor_on; /* right motor off/on 0/1 */
@@ -184,15 +185,24 @@ struct bigben_device {
struct work_struct worker;
};
+static inline void bigben_schedule_work(struct bigben_device *bigben)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&bigben->lock, flags);
+ if (!bigben->removed)
+ schedule_work(&bigben->worker);
+ spin_unlock_irqrestore(&bigben->lock, flags);
+}
static void bigben_worker(struct work_struct *work)
{
struct bigben_device *bigben = container_of(work,
struct bigben_device, worker);
struct hid_field *report_field = bigben->report->field[0];
+ unsigned long flags;
- if (bigben->removed || !report_field)
- return;
+ spin_lock_irqsave(&bigben->lock, flags);
if (bigben->work_led) {
bigben->work_led = false;
@@ -219,6 +229,8 @@ static void bigben_worker(struct work_struct *work)
report_field->value[7] = 0x00; /* padding */
hid_hw_request(bigben->hid, bigben->report, HID_REQ_SET_REPORT);
}
+
+ spin_unlock_irqrestore(&bigben->lock, flags);
}
static int hid_bigben_play_effect(struct input_dev *dev, void *data,
@@ -228,6 +240,7 @@ static int hid_bigben_play_effect(struct input_dev *dev, void *data,
struct bigben_device *bigben = hid_get_drvdata(hid);
u8 right_motor_on;
u8 left_motor_force;
+ unsigned long flags;
if (!bigben) {
hid_err(hid, "no device data\n");
@@ -242,10 +255,13 @@ static int hid_bigben_play_effect(struct input_dev *dev, void *data,
if (right_motor_on != bigben->right_motor_on ||
left_motor_force != bigben->left_motor_force) {
+ spin_lock_irqsave(&bigben->lock, flags);
bigben->right_motor_on = right_motor_on;
bigben->left_motor_force = left_motor_force;
bigben->work_ff = true;
- schedule_work(&bigben->worker);
+ spin_unlock_irqrestore(&bigben->lock, flags);
+
+ bigben_schedule_work(bigben);
}
return 0;
@@ -259,6 +275,7 @@ static void bigben_set_led(struct led_classdev *led,
struct bigben_device *bigben = hid_get_drvdata(hid);
int n;
bool work;
+ unsigned long flags;
if (!bigben) {
hid_err(hid, "no device data\n");
@@ -267,6 +284,7 @@ static void bigben_set_led(struct led_classdev *led,
for (n = 0; n < NUM_LEDS; n++) {
if (led == bigben->leds[n]) {
+ spin_lock_irqsave(&bigben->lock, flags);
if (value == LED_OFF) {
work = (bigben->led_state & BIT(n));
bigben->led_state &= ~BIT(n);
@@ -274,10 +292,11 @@ static void bigben_set_led(struct led_classdev *led,
work = !(bigben->led_state & BIT(n));
bigben->led_state |= BIT(n);
}
+ spin_unlock_irqrestore(&bigben->lock, flags);
if (work) {
bigben->work_led = true;
- schedule_work(&bigben->worker);
+ bigben_schedule_work(bigben);
}
return;
}
@@ -307,8 +326,12 @@ static enum led_brightness bigben_get_led(struct led_classdev *led)
static void bigben_remove(struct hid_device *hid)
{
struct bigben_device *bigben = hid_get_drvdata(hid);
+ unsigned long flags;
+ spin_lock_irqsave(&bigben->lock, flags);
bigben->removed = true;
+ spin_unlock_irqrestore(&bigben->lock, flags);
+
cancel_work_sync(&bigben->worker);
hid_hw_stop(hid);
}
@@ -362,6 +385,7 @@ static int bigben_probe(struct hid_device *hid,
set_bit(FF_RUMBLE, hidinput->input->ffbit);
INIT_WORK(&bigben->worker, bigben_worker);
+ spin_lock_init(&bigben->lock);
error = input_ff_create_memless(hidinput->input, NULL,
hid_bigben_play_effect);
@@ -402,7 +426,7 @@ static int bigben_probe(struct hid_device *hid,
bigben->left_motor_force = 0;
bigben->work_led = true;
bigben->work_ff = true;
- schedule_work(&bigben->worker);
+ bigben_schedule_work(bigben);
hid_info(hid, "LED and force feedback support for BigBen gamepad\n");
--
2.25.1
On Fri, Feb 10, 2023 at 2:24 PM Hillf Danton <[email protected]> wrote:
>
> On Thu, 09 Feb 2023 23:58:55 +0000 Pietro Borrello <[email protected]>
> > Use spinlocks to deal with workers introducing a wrapper
> > bigben_schedule_work(), and several spinlock checks.
> > Otherwise, bigben_set_led() may schedule bigben->worker after the
> > structure has been freed, causing a use-after-free.
> >
> > Fixes: 4eb1b01de5b9 ("HID: hid-bigbenff: fix race condition for scheduled work during removal")
>
> Given the flag added in 4eb1b01de5b9 and the spinlock added in this
> patchset, devm_led_classdev_register() looks to not work for you.
Actually, looking at the code now, it is clear that we need that lock.
The current code is happily changing the struct bigben_device from
multiple contexts, and pulls that without any barrier in the work
struct which should produce some interesting results :)
And we can probably abuse that lock to prevent scheduling a new work
as it is done in hid-playstation.c
I'll comment in the patch which parts need to be changed, because it
is true that this patch is definitely not mergeable as such and will
need another revision.
>
> How about replacing the advanced devm_ method with the traditional plain
> pair of led_classdev_un/register(), with the flag mentioned cut off but
> without bothering to add another lock?
>
As mentioned above, the lock is needed anyway, and will probably need
to be added in a separate patch.
Reverting to a non devm version of the led class would complexify the
driver for the error paths, and is probably not the best move IMO.
Cheers,
Benjamin
On Feb 09 2023, Pietro Borrello wrote:
> Use spinlocks to deal with workers introducing a wrapper
> bigben_schedule_work(), and several spinlock checks.
> Otherwise, bigben_set_led() may schedule bigben->worker after the
> structure has been freed, causing a use-after-free.
>
> Fixes: 4eb1b01de5b9 ("HID: hid-bigbenff: fix race condition for scheduled work during removal")
> Signed-off-by: Pietro Borrello <[email protected]>
> ---
> drivers/hid/hid-bigbenff.c | 34 +++++++++++++++++++++++++++++-----
> 1 file changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hid/hid-bigbenff.c b/drivers/hid/hid-bigbenff.c
> index e8b16665860d..28769aa7fed6 100644
> --- a/drivers/hid/hid-bigbenff.c
> +++ b/drivers/hid/hid-bigbenff.c
> @@ -174,6 +174,7 @@ static __u8 pid0902_rdesc_fixed[] = {
> struct bigben_device {
> struct hid_device *hid;
> struct hid_report *report;
> + spinlock_t lock;
> bool removed;
> u8 led_state; /* LED1 = 1 .. LED4 = 8 */
> u8 right_motor_on; /* right motor off/on 0/1 */
> @@ -184,15 +185,24 @@ struct bigben_device {
> struct work_struct worker;
> };
>
> +static inline void bigben_schedule_work(struct bigben_device *bigben)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&bigben->lock, flags);
> + if (!bigben->removed)
> + schedule_work(&bigben->worker);
> + spin_unlock_irqrestore(&bigben->lock, flags);
> +}
>
> static void bigben_worker(struct work_struct *work)
> {
> struct bigben_device *bigben = container_of(work,
> struct bigben_device, worker);
> struct hid_field *report_field = bigben->report->field[0];
> + unsigned long flags;
>
> - if (bigben->removed || !report_field)
You are removing an important test here: if (!report_field), please keep
it.
> - return;
> + spin_lock_irqsave(&bigben->lock, flags);
>
> if (bigben->work_led) {
> bigben->work_led = false;
> @@ -219,6 +229,8 @@ static void bigben_worker(struct work_struct *work)
> report_field->value[7] = 0x00; /* padding */
> hid_hw_request(bigben->hid, bigben->report, HID_REQ_SET_REPORT);
> }
> +
> + spin_unlock_irqrestore(&bigben->lock, flags);
Ouch, having hid_hw_request() called whithin a spinlock is definitely not
something that should be done.
However, the spinlock should be protecting 2 kinds of things:
- any access to any value of struct bigben_device, but in an atomic way
(i.e. copy everything you need locally in a spinlock, then release it
and never read that struct again in that function).
- the access to bigben->removed, which should be checked only in
bigben_schedule_work() and in the .remove() function.
Please note that this is what the playstation driver does: it prepares
the report under the spinlock (which is really fast) before sending the
report to the device which can be slow and be interrupted.
With that being said, it is clear that we need 2 patches for this one:
- the first one introduces the spinlock and protects the concurrent
accesses to struct bigben_device (which is roughly everything below
with the changes I just said)
- the second one introduces bigben_schedule_work() and piggy backs on
top of that new lock.
Cheers,
Benjamin
> }
>
> static int hid_bigben_play_effect(struct input_dev *dev, void *data,
> @@ -228,6 +240,7 @@ static int hid_bigben_play_effect(struct input_dev *dev, void *data,
> struct bigben_device *bigben = hid_get_drvdata(hid);
> u8 right_motor_on;
> u8 left_motor_force;
> + unsigned long flags;
>
> if (!bigben) {
> hid_err(hid, "no device data\n");
> @@ -242,10 +255,13 @@ static int hid_bigben_play_effect(struct input_dev *dev, void *data,
>
> if (right_motor_on != bigben->right_motor_on ||
> left_motor_force != bigben->left_motor_force) {
> + spin_lock_irqsave(&bigben->lock, flags);
> bigben->right_motor_on = right_motor_on;
> bigben->left_motor_force = left_motor_force;
> bigben->work_ff = true;
> - schedule_work(&bigben->worker);
> + spin_unlock_irqrestore(&bigben->lock, flags);
> +
> + bigben_schedule_work(bigben);
> }
>
> return 0;
> @@ -259,6 +275,7 @@ static void bigben_set_led(struct led_classdev *led,
> struct bigben_device *bigben = hid_get_drvdata(hid);
> int n;
> bool work;
> + unsigned long flags;
>
> if (!bigben) {
> hid_err(hid, "no device data\n");
> @@ -267,6 +284,7 @@ static void bigben_set_led(struct led_classdev *led,
>
> for (n = 0; n < NUM_LEDS; n++) {
> if (led == bigben->leds[n]) {
> + spin_lock_irqsave(&bigben->lock, flags);
> if (value == LED_OFF) {
> work = (bigben->led_state & BIT(n));
> bigben->led_state &= ~BIT(n);
> @@ -274,10 +292,11 @@ static void bigben_set_led(struct led_classdev *led,
> work = !(bigben->led_state & BIT(n));
> bigben->led_state |= BIT(n);
> }
> + spin_unlock_irqrestore(&bigben->lock, flags);
>
> if (work) {
> bigben->work_led = true;
> - schedule_work(&bigben->worker);
> + bigben_schedule_work(bigben);
> }
> return;
> }
> @@ -307,8 +326,12 @@ static enum led_brightness bigben_get_led(struct led_classdev *led)
> static void bigben_remove(struct hid_device *hid)
> {
> struct bigben_device *bigben = hid_get_drvdata(hid);
> + unsigned long flags;
>
> + spin_lock_irqsave(&bigben->lock, flags);
> bigben->removed = true;
> + spin_unlock_irqrestore(&bigben->lock, flags);
> +
> cancel_work_sync(&bigben->worker);
> hid_hw_stop(hid);
> }
> @@ -362,6 +385,7 @@ static int bigben_probe(struct hid_device *hid,
> set_bit(FF_RUMBLE, hidinput->input->ffbit);
>
> INIT_WORK(&bigben->worker, bigben_worker);
> + spin_lock_init(&bigben->lock);
>
> error = input_ff_create_memless(hidinput->input, NULL,
> hid_bigben_play_effect);
> @@ -402,7 +426,7 @@ static int bigben_probe(struct hid_device *hid,
> bigben->left_motor_force = 0;
> bigben->work_led = true;
> bigben->work_ff = true;
> - schedule_work(&bigben->worker);
> + bigben_schedule_work(bigben);
>
> hid_info(hid, "LED and force feedback support for BigBen gamepad\n");
>
>
> --
> 2.25.1
>
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 <[email protected]>
> ---
> 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
>
On Fri, 10 Feb 2023 at 15:26, Benjamin Tissoires
<[email protected]> wrote:
>
> [...]
> >
> > - if (bigben->removed || !report_field)
>
> You are removing an important test here: if (!report_field), please keep
> it.
To my understanding, that check was added in commit
918aa1ef104d ("HID: bigbenff: prevent null pointer dereference")
to prevent the NULL pointer crash, only fixing the crash point.
However, the true root cause was a missing check for output
reports patched in commit
c7bf714f8755 ("HID: check empty report_list in bigben_probe()"),
where the type-confused report list_entry was overlapping with
a NULL pointer, which was then causing the crash.
Let me know if there is any other path that may result in having a
report with no fields. In that case, it would make sense to keep the
check.
>
> > - return;
> > + spin_lock_irqsave(&bigben->lock, flags);
> >
> > if (bigben->work_led) {
> > bigben->work_led = false;
> > @@ -219,6 +229,8 @@ static void bigben_worker(struct work_struct *work)
> > report_field->value[7] = 0x00; /* padding */
> > hid_hw_request(bigben->hid, bigben->report, HID_REQ_SET_REPORT);
> > }
> > +
> > + spin_unlock_irqrestore(&bigben->lock, flags);
>
> Ouch, having hid_hw_request() called whithin a spinlock is definitely not
> something that should be done.
>
> However, the spinlock should be protecting 2 kinds of things:
> - any access to any value of struct bigben_device, but in an atomic way
> (i.e. copy everything you need locally in a spinlock, then release it
> and never read that struct again in that function).
> - the access to bigben->removed, which should be checked only in
> bigben_schedule_work() and in the .remove() function.
>
> Please note that this is what the playstation driver does: it prepares
> the report under the spinlock (which is really fast) before sending the
> report to the device which can be slow and be interrupted.
>
> With that being said, it is clear that we need 2 patches for this one:
> - the first one introduces the spinlock and protects the concurrent
> accesses to struct bigben_device (which is roughly everything below
> with the changes I just said)
> - the second one introduces bigben_schedule_work() and piggy backs on
> top of that new lock.
Thanks for clarifying. I will work on a v4 patch.
Best regards,
Pietro
On Sat, Feb 11, 2023 at 3:01 AM Hillf Danton <[email protected]> wrote:
>
> On Fri, 10 Feb 2023 15:11:26 +0100 Benjamin Tissoires <[email protected]>
> > On Fri, Feb 10, 2023 at 2:24 PM Hillf Danton <[email protected]> wrote:
> > > On Thu, 09 Feb 2023 23:58:55 +0000 Pietro Borrello <[email protected]>
> > > > Use spinlocks to deal with workers introducing a wrapper
> > > > bigben_schedule_work(), and several spinlock checks.
> > > > Otherwise, bigben_set_led() may schedule bigben->worker after the
> > > > structure has been freed, causing a use-after-free.
> > > >
> > > > Fixes: 4eb1b01de5b9 ("HID: hid-bigbenff: fix race condition for scheduled work during removal")
> > >
> > > Given the flag added in 4eb1b01de5b9 and the spinlock added in this
> > > patchset, devm_led_classdev_register() looks to not work for you.
> >
> > Actually, looking at the code now, it is clear that we need that lock.
> > The current code is happily changing the struct bigben_device from
> > multiple contexts, and pulls that without any barrier in the work
> > struct which should produce some interesting results :)
> >
> > And we can probably abuse that lock to prevent scheduling a new work
> > as it is done in hid-playstation.c
> >
> > I'll comment in the patch which parts need to be changed, because it
> > is true that this patch is definitely not mergeable as such and will
> > need another revision.
> >
> > >
> > > How about replacing the advanced devm_ method with the traditional plain
> > > pair of led_classdev_un/register(), with the flag mentioned cut off but
> > > without bothering to add another lock?
> > >
> >
> > As mentioned above, the lock is needed anyway, and will probably need
> > to be added in a separate patch.
> > Reverting to a non devm version of the led class would complexify the
> > driver for the error paths, and is probably not the best move IMO.
>
> After this patch,
>
> cpu 0 cpu 2
> --- ---
> bigben_remove()
> spin_lock_irqsave(&bigben->lock, flags);
> bigben->removed = true;
> spin_unlock_irqrestore(&bigben->lock, flags);
>
> spin_lock_irqsave(&bigben->lock, flags);
>
> what makes it safe for cpu2 to acquire lock after the removed flag is true?
The remove flag is just a way to prevent any other workqueue from
starting. It doesn't mean that the struct bigben has been freed, so
acquiring a lock at that point is fine.
We then rely on 2 things:
- devm_class_led to be released before struct bigben, because it was
devm-allocated *after* the struct bigben was devm-allocated
- we prevent any new workqueue to start and we guarantee that any
running workqueue is terminated before leaving the .remove function.
Given that the ledclass is gracefully shutting down all of its
potential queues, we don't have any other possibility to have an
unsafe call AFAIU.
Cheers,
Benjamin
On Mon, Feb 13, 2023 at 11:37 AM Hillf Danton <[email protected]> wrote:
>
> On Mon, 13 Feb 2023 09:25:37 +0100 Benjamin Tissoires <[email protected]>
> >
> > The remove flag is just a way to prevent any other workqueue from
> > starting. It doesn't mean that the struct bigben has been freed, so
> > acquiring a lock at that point is fine.
> > We then rely on 2 things:
> > - devm_class_led to be released before struct bigben, because it was
> > devm-allocated *after* the struct bigben was devm-allocated
>
> This is the current behavior and it is intact after this patch.
>
> > - we prevent any new workqueue to start and we guarantee that any
> > running workqueue is terminated before leaving the .remove function.
>
> If spinlock is added for not scheduling new workqueue then it is not
> needed, because the removed flag is set before running workqueue is
> terminated. Checking the flag is enough upon queuing new work.
>
I tend to disagree (based on Pietro's v4:
- no worker is running
- a led sysfs call is made
- the line "if (!bigben->removed)" is true
- this gets interrupted/or another CPU kicks in for the next one
-> .remove gets called
- bigben->removed is set to false
- cancel_work_sync() called
the led call continues, and schedules the work
.removes terminates, and devm kicks in, killing led_class and struct
bigben while the workqueue is running.
So having a simple spinlocks ensures the atomicity between checking
for bigben->removed and scheduling a workqueue.
Cheers,
Benjamin
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/[email protected]/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 <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
| Link: https://lore.kernel.org/r/[email protected]/
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
On Tue, 14 Feb 2023 at 08:58, Dan Carpenter <[email protected]> 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/[email protected]/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 <[email protected]>
> | Reported-by: Dan Carpenter <[email protected]>
> | Link: https://lore.kernel.org/r/[email protected]/
>
> 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/[email protected]/T/#m8983ca3d0fbf656a40011a77c8988e3d16cac671
Best regards,
Pietro