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.
More drivers have the same pattern (hid-lg-g15.c, hid-playstation.c,
hid-sony.c) but I wasn't able to properly pass the right descriptors
to trigger the led configurations needed to trigger the workers.
Some other drivers manually unregister at removal (hid-corsair.c,
hid-gt683r.c, hid-lenovo.c) since they do not use the managed
interface, which is safe, to my understanding.
Also, a similar pattern is present with callbacks which schedule
a worker originating from input_ff_create_memless() (e.g.,
in hid-bigbenff.c) but in these cases, I wasn't able to trigger
the race condition with the event handling to schedule the worker
during device removal. However, I have no experience with the USB
protocol and I'm not able to say that they cannot be triggered.
I am currently wondering if this is due to some emulation of the
/dev/raw-gadget interface or if it's effectively an issue with how each
device manages resource removal.
But I wonder why syzkaller didn't find any crash while fuzzing the
interface with upstream-usb.config, as they seem pretty
straightforward to trigger.
Configuring the kernel with CONFIG_DEBUG_OBJECTS, it emits
a warning in debug_check_no_obj_freed, which makes it clear that
device removal is freeing resources in use.
KASAN detects them as use-after-free.
I am attaching multiple patches for all the drivers I suspect the bug
is present.
The proposed patches unregister the LED controllers before removing the
device itself.
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
```
To: Jiri Kosina <[email protected]>
To: Benjamin Tissoires <[email protected]>
To: Hanno Zulla <[email protected]>
To: Carlo Caione <[email protected]>
To: Pavel Machek <[email protected]>
To: Lee Jones <[email protected]>
To: Roderick Colenbrander <[email protected]>
To: Sven Eckelmann <[email protected]>
Cc: [email protected]
Cc: Cristiano Giuffrida <[email protected]>
Cc: "Bos, H.J." <[email protected]>
Cc: Jakob Koschel <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: Jiri Kosina <[email protected]>
Cc: Roderick Colenbrander <[email protected]>
Signed-off-by: Pietro Borrello <[email protected]>
---
Pietro Borrello (5):
HID: bigben_remove: manually unregister leds
HID: asus_remove: manually unregister led
HID: dualsense_remove: manually unregister leds
HID: dualshock4_remove: manually unregister leds
HID: sony_remove: manually unregister leds
drivers/hid/hid-asus.c | 1 +
drivers/hid/hid-bigbenff.c | 5 +++++
drivers/hid/hid-playstation.c | 8 ++++++++
drivers/hid/hid-sony.c | 8 ++++++++
4 files changed, 22 insertions(+)
---
base-commit: 2241ab53cbb5cdb08a6b2d4688feb13971058f65
change-id: 20230125-hid-unregister-leds-4cbf67099e1d
Best regards,
--
Pietro Borrello <[email protected]>
Unregister the LED controller before device removal, as
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 | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index f99752b998f3..0f274c8d1bef 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -1122,6 +1122,7 @@ static void asus_remove(struct hid_device *hdev)
if (drvdata->kbd_backlight) {
drvdata->kbd_backlight->removed = true;
+ devm_led_classdev_unregister(&hdev->dev, &drvdata->kbd_backlight->cdev);
cancel_work_sync(&drvdata->kbd_backlight->work);
}
--
2.25.1
Unregister the LED controller before device removal, as
dualsense_player_led_set_brightness() may schedule output_worker
after the structure has been freed, causing a use-after-free.
Fixes: 8c0ab553b072 ("HID: playstation: expose DualSense player LEDs through LED class.")
Signed-off-by: Pietro Borrello <[email protected]>
---
drivers/hid/hid-playstation.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 27c40894acab..9e23860b7e95 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -1503,11 +1503,15 @@ static void dualsense_remove(struct ps_device *ps_dev)
{
struct dualsense *ds = container_of(ps_dev, struct dualsense, base);
unsigned long flags;
+ int i;
spin_lock_irqsave(&ds->base.lock, flags);
ds->output_worker_initialized = false;
spin_unlock_irqrestore(&ds->base.lock, flags);
+ for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++)
+ devm_led_classdev_unregister(&ps_dev->hdev->dev, &ds->player_leds[i]);
+
cancel_work_sync(&ds->output_worker);
}
--
2.25.1
Unregister the LED controller before device removal, as
sony_led_set_brightness() may schedule sc->state_worker
after the structure has been freed, causing a use-after-free.
Fixes: 0a286ef27852 ("HID: sony: Add LED support for Sixaxis/Dualshock3 USB")
Signed-off-by: Pietro Borrello <[email protected]>
---
drivers/hid/hid-sony.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 13125997ab5e..146677c8319c 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -3083,6 +3083,7 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
static void sony_remove(struct hid_device *hdev)
{
struct sony_sc *sc = hid_get_drvdata(hdev);
+ int n;
if (sc->quirks & (GHL_GUITAR_PS3WIIU | GHL_GUITAR_PS4)) {
del_timer_sync(&sc->ghl_poke_timer);
@@ -3100,6 +3101,13 @@ static void sony_remove(struct hid_device *hdev)
if (sc->hw_version_created)
device_remove_file(&sc->hdev->dev, &dev_attr_hardware_version);
+ if (sc->quirks & SONY_LED_SUPPORT) {
+ for (n = 0; n < sc->led_count; n++) {
+ if (sc->leds[n])
+ devm_led_classdev_unregister(&hdev->dev, sc->leds[n]);
+ }
+ }
+
sony_cancel_work_sync(sc);
sony_remove_dev_list(sc);
--
2.25.1
Unregister the LED controller before device removal, as
dualshock4_led_set_brightness() may schedule output_worker
after the structure has been freed, causing a use-after-free.
Fixes: 4521109a8f40 ("HID: playstation: support DualShock4 lightbar.")
Signed-off-by: Pietro Borrello <[email protected]>
---
drivers/hid/hid-playstation.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 9e23860b7e95..6fae5a24eaad 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -2432,11 +2432,15 @@ static void dualshock4_remove(struct ps_device *ps_dev)
{
struct dualshock4 *ds4 = container_of(ps_dev, struct dualshock4, base);
unsigned long flags;
+ int i;
spin_lock_irqsave(&ds4->base.lock, flags);
ds4->output_worker_initialized = false;
spin_unlock_irqrestore(&ds4->base.lock, flags);
+ for (i = 0; i < ARRAY_SIZE(ds4->lightbar_leds); i++)
+ devm_led_classdev_unregister(&ps_dev->hdev->dev, &ds4->lightbar_leds[i]);
+
cancel_work_sync(&ds4->output_worker);
if (ps_dev->hdev->product == USB_DEVICE_ID_SONY_PS4_CONTROLLER_DONGLE)
--
2.25.1
Hi Pietro,
Thanks for your patch. For sure for ds4/dualsense there have been edge
cases around rumble removal and others. Those were prevented by this
'output_worker_initialized' variable, which is checked during the
centralized work scheduling function (dualshock4_schedule_work /
dualsense_schedule_work). That said I don't mind the change as it
prevents the work scheduling functions to get called unnecessarily.
Thanks,
Roderick Colenbrander
On Wed, Jan 25, 2023 at 4:26 PM Pietro Borrello
<[email protected]> wrote:
>
> Unregister the LED controller before device removal, as
> dualsense_player_led_set_brightness() may schedule output_worker
> after the structure has been freed, causing a use-after-free.
>
> Fixes: 8c0ab553b072 ("HID: playstation: expose DualSense player LEDs through LED class.")
> Signed-off-by: Pietro Borrello <[email protected]>
> ---
> drivers/hid/hid-playstation.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index 27c40894acab..9e23860b7e95 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> @@ -1503,11 +1503,15 @@ static void dualsense_remove(struct ps_device *ps_dev)
> {
> struct dualsense *ds = container_of(ps_dev, struct dualsense, base);
> unsigned long flags;
> + int i;
>
> spin_lock_irqsave(&ds->base.lock, flags);
> ds->output_worker_initialized = false;
> spin_unlock_irqrestore(&ds->base.lock, flags);
>
> + for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++)
> + devm_led_classdev_unregister(&ps_dev->hdev->dev, &ds->player_leds[i]);
> +
> cancel_work_sync(&ds->output_worker);
> }
>
>
> --
> 2.25.1
One other little note. For completeness I guess we need a similar
patch for the multicolor led class.
On Wed, Jan 25, 2023 at 4:43 PM Roderick Colenbrander
<[email protected]> wrote:
>
> Hi Pietro,
>
> Thanks for your patch. For sure for ds4/dualsense there have been edge
> cases around rumble removal and others. Those were prevented by this
> 'output_worker_initialized' variable, which is checked during the
> centralized work scheduling function (dualshock4_schedule_work /
> dualsense_schedule_work). That said I don't mind the change as it
> prevents the work scheduling functions to get called unnecessarily.
>
> Thanks,
> Roderick Colenbrander
>
> On Wed, Jan 25, 2023 at 4:26 PM Pietro Borrello
> <[email protected]> wrote:
> >
> > Unregister the LED controller before device removal, as
> > dualsense_player_led_set_brightness() may schedule output_worker
> > after the structure has been freed, causing a use-after-free.
> >
> > Fixes: 8c0ab553b072 ("HID: playstation: expose DualSense player LEDs through LED class.")
> > Signed-off-by: Pietro Borrello <[email protected]>
> > ---
> > drivers/hid/hid-playstation.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> > index 27c40894acab..9e23860b7e95 100644
> > --- a/drivers/hid/hid-playstation.c
> > +++ b/drivers/hid/hid-playstation.c
> > @@ -1503,11 +1503,15 @@ static void dualsense_remove(struct ps_device *ps_dev)
> > {
> > struct dualsense *ds = container_of(ps_dev, struct dualsense, base);
> > unsigned long flags;
> > + int i;
> >
> > spin_lock_irqsave(&ds->base.lock, flags);
> > ds->output_worker_initialized = false;
> > spin_unlock_irqrestore(&ds->base.lock, flags);
> >
> > + for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++)
> > + devm_led_classdev_unregister(&ps_dev->hdev->dev, &ds->player_leds[i]);
> > +
> > cancel_work_sync(&ds->output_worker);
> > }
> >
> >
> > --
> > 2.25.1
On Thu, 26 Jan 2023 at 01:44, Roderick Colenbrander
<[email protected]> wrote:
>
> Hi Pietro,
>
> Thanks for your patch. For sure for ds4/dualsense there have been edge
> cases around rumble removal and others. Those were prevented by this
> 'output_worker_initialized' variable, which is checked during the
> centralized work scheduling function (dualshock4_schedule_work /
> dualsense_schedule_work). That said I don't mind the change as it
> prevents the work scheduling functions to get called unnecessarily.
Hi Roderick,
Thank you for your fast response. You are right, the combination
of the `output_worker_initialized` variable and the spinlock prevents
the work to be scheduled during device removal for ds4/dualsense.
Thank you for the clarification!
Thanks,
Pietro
On Thursday, 26 January 2023 01:24:57 CET Pietro Borrello wrote:
> Unregister the LED controller before device removal, as
> sony_led_set_brightness() may schedule sc->state_worker
> after the structure has been freed, causing a use-after-free.
>
> Fixes: 0a286ef27852 ("HID: sony: Add LED support for Sixaxis/Dualshock3 USB")
> Signed-off-by: Pietro Borrello <[email protected]>
> ---
> drivers/hid/hid-sony.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
Reviewed-by: Sven Eckelmann <[email protected]>
Thanks,
Sven
Unregister the LED controllers before device removal, to prevent
unnecessary runs of dualsense_player_led_set_brightness().
Fixes: 8c0ab553b072 ("HID: playstation: expose DualSense player LEDs through LED class.")
Signed-off-by: Pietro Borrello <[email protected]>
---
Contrary to the other patches in this series, failing to unregister
the led controller does not results into a use-after-free thanks
to the output_worker_initialized variable and the spinlock checks.
Changes in v2:
- Unregister multicolor led controller
- Clarify UAF
- Link to v1: https://lore.kernel.org/all/[email protected]/
---
drivers/hid/hid-playstation.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 27c40894acab..f23186ca2d76 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -1503,11 +1503,17 @@ static void dualsense_remove(struct ps_device *ps_dev)
{
struct dualsense *ds = container_of(ps_dev, struct dualsense, base);
unsigned long flags;
+ int i;
spin_lock_irqsave(&ds->base.lock, flags);
ds->output_worker_initialized = false;
spin_unlock_irqrestore(&ds->base.lock, flags);
+ for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++)
+ devm_led_classdev_unregister(&ps_dev->hdev->dev, &ds->player_leds[i]);
+
+ devm_led_classdev_multicolor_unregister(&ps_dev->hdev->dev, &ds->lightbar);
+
cancel_work_sync(&ds->output_worker);
}
--
2.25.1
Unregister the LED controllers before device removal, to prevent
unnecessary runs of dualshock4_led_set_brightness().
Fixes: 4521109a8f40 ("HID: playstation: support DualShock4 lightbar.")
Signed-off-by: Pietro Borrello <[email protected]>
---
Contrary to the other patches in this series, failing to unregister
the led controller does not results into a use-after-free thanks
to the output_worker_initialized variable and the spinlock checks.
Changes in v2:
- Clarify UAF
- Link to v1: https://lore.kernel.org/all/[email protected]/
---
drivers/hid/hid-playstation.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index f23186ca2d76..b41657842e26 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -2434,11 +2434,15 @@ static void dualshock4_remove(struct ps_device *ps_dev)
{
struct dualshock4 *ds4 = container_of(ps_dev, struct dualshock4, base);
unsigned long flags;
+ int i;
spin_lock_irqsave(&ds4->base.lock, flags);
ds4->output_worker_initialized = false;
spin_unlock_irqrestore(&ds4->base.lock, flags);
+ for (i = 0; i < ARRAY_SIZE(ds4->lightbar_leds); i++)
+ devm_led_classdev_unregister(&ps_dev->hdev->dev, &ds4->lightbar_leds[i]);
+
cancel_work_sync(&ds4->output_worker);
if (ps_dev->hdev->product == USB_DEVICE_ID_SONY_PS4_CONTROLLER_DONGLE)
--
2.25.1
Is this series in a queue somewhere? Seems rather important and I don't find progress.
Also, should be cc: [email protected] , right?
CVE-2023-25012
Best
Donald
On 1/26/23 1:24 AM, Pietro Borrello wrote:
> 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.
>
> More drivers have the same pattern (hid-lg-g15.c, hid-playstation.c,
> hid-sony.c) but I wasn't able to properly pass the right descriptors
> to trigger the led configurations needed to trigger the workers.
> Some other drivers manually unregister at removal (hid-corsair.c,
> hid-gt683r.c, hid-lenovo.c) since they do not use the managed
> interface, which is safe, to my understanding.
> Also, a similar pattern is present with callbacks which schedule
> a worker originating from input_ff_create_memless() (e.g.,
> in hid-bigbenff.c) but in these cases, I wasn't able to trigger
> the race condition with the event handling to schedule the worker
> during device removal. However, I have no experience with the USB
> protocol and I'm not able to say that they cannot be triggered.
>
> I am currently wondering if this is due to some emulation of the
> /dev/raw-gadget interface or if it's effectively an issue with how each
> device manages resource removal.
> But I wonder why syzkaller didn't find any crash while fuzzing the
> interface with upstream-usb.config, as they seem pretty
> straightforward to trigger.
> Configuring the kernel with CONFIG_DEBUG_OBJECTS, it emits
> a warning in debug_check_no_obj_freed, which makes it clear that
> device removal is freeing resources in use.
> KASAN detects them as use-after-free.
>
> I am attaching multiple patches for all the drivers I suspect the bug
> is present.
> The proposed patches unregister the LED controllers before removing the
> device itself.
>
> 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
> ```
>
> To: Jiri Kosina <[email protected]>
> To: Benjamin Tissoires <[email protected]>
> To: Hanno Zulla <[email protected]>
> To: Carlo Caione <[email protected]>
> To: Pavel Machek <[email protected]>
> To: Lee Jones <[email protected]>
> To: Roderick Colenbrander <[email protected]>
> To: Sven Eckelmann <[email protected]>
> Cc: [email protected]
> Cc: Cristiano Giuffrida <[email protected]>
> Cc: "Bos, H.J." <[email protected]>
> Cc: Jakob Koschel <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: Jiri Kosina <[email protected]>
> Cc: Roderick Colenbrander <[email protected]>
> Signed-off-by: Pietro Borrello <[email protected]>
>
> ---
> Pietro Borrello (5):
> HID: bigben_remove: manually unregister leds
> HID: asus_remove: manually unregister led
> HID: dualsense_remove: manually unregister leds
> HID: dualshock4_remove: manually unregister leds
> HID: sony_remove: manually unregister leds
>
> drivers/hid/hid-asus.c | 1 +
> drivers/hid/hid-bigbenff.c | 5 +++++
> drivers/hid/hid-playstation.c | 8 ++++++++
> drivers/hid/hid-sony.c | 8 ++++++++
> 4 files changed, 22 insertions(+)
> ---
> base-commit: 2241ab53cbb5cdb08a6b2d4688feb13971058f65
> change-id: 20230125-hid-unregister-leds-4cbf67099e1d
>
> Best regards,
>
--
Donald Buczek
[email protected]
Tel: +49 30 8413 1433
On Wed, May 10, 2023 at 12:21 PM Donald Buczek <[email protected]> wrote:
>
> Is this series in a queue somewhere?
Yes, but not in that form.
>
>
> Seems rather important and I don't find progress.
Well, there has been a v4 that has been merged already.
>
> Also, should be cc: [email protected] , right?
The fixes are already backported to stable, because Pietro added the
"fixes" tags which is a trigger for them to backport stuff.
>
> CVE-2023-25012
You have no idea how much I hate CVEs and how they are used as a way
to force an answer to an email or to force code inclusion...
Cheers,
Benjamin
>
> Best
>
> Donald
>
> On 1/26/23 1:24 AM, Pietro Borrello wrote:
> > 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.
> >
> > More drivers have the same pattern (hid-lg-g15.c, hid-playstation.c,
> > hid-sony.c) but I wasn't able to properly pass the right descriptors
> > to trigger the led configurations needed to trigger the workers.
> > Some other drivers manually unregister at removal (hid-corsair.c,
> > hid-gt683r.c, hid-lenovo.c) since they do not use the managed
> > interface, which is safe, to my understanding.
> > Also, a similar pattern is present with callbacks which schedule
> > a worker originating from input_ff_create_memless() (e.g.,
> > in hid-bigbenff.c) but in these cases, I wasn't able to trigger
> > the race condition with the event handling to schedule the worker
> > during device removal. However, I have no experience with the USB
> > protocol and I'm not able to say that they cannot be triggered.
> >
> > I am currently wondering if this is due to some emulation of the
> > /dev/raw-gadget interface or if it's effectively an issue with how each
> > device manages resource removal.
> > But I wonder why syzkaller didn't find any crash while fuzzing the
> > interface with upstream-usb.config, as they seem pretty
> > straightforward to trigger.
> > Configuring the kernel with CONFIG_DEBUG_OBJECTS, it emits
> > a warning in debug_check_no_obj_freed, which makes it clear that
> > device removal is freeing resources in use.
> > KASAN detects them as use-after-free.
> >
> > I am attaching multiple patches for all the drivers I suspect the bug
> > is present.
> > The proposed patches unregister the LED controllers before removing the
> > device itself.
> >
> > 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
> > ```
> >
> > To: Jiri Kosina <[email protected]>
> > To: Benjamin Tissoires <[email protected]>
> > To: Hanno Zulla <[email protected]>
> > To: Carlo Caione <[email protected]>
> > To: Pavel Machek <[email protected]>
> > To: Lee Jones <[email protected]>
> > To: Roderick Colenbrander <[email protected]>
> > To: Sven Eckelmann <[email protected]>
> > Cc: [email protected]
> > Cc: Cristiano Giuffrida <[email protected]>
> > Cc: "Bos, H.J." <[email protected]>
> > Cc: Jakob Koschel <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: Jiri Kosina <[email protected]>
> > Cc: Roderick Colenbrander <[email protected]>
> > Signed-off-by: Pietro Borrello <[email protected]>
> >
> > ---
> > Pietro Borrello (5):
> > HID: bigben_remove: manually unregister leds
> > HID: asus_remove: manually unregister led
> > HID: dualsense_remove: manually unregister leds
> > HID: dualshock4_remove: manually unregister leds
> > HID: sony_remove: manually unregister leds
> >
> > drivers/hid/hid-asus.c | 1 +
> > drivers/hid/hid-bigbenff.c | 5 +++++
> > drivers/hid/hid-playstation.c | 8 ++++++++
> > drivers/hid/hid-sony.c | 8 ++++++++
> > 4 files changed, 22 insertions(+)
> > ---
> > base-commit: 2241ab53cbb5cdb08a6b2d4688feb13971058f65
> > change-id: 20230125-hid-unregister-leds-4cbf67099e1d
> >
> > Best regards,
> >
>
>
> --
> Donald Buczek
> [email protected]
> Tel: +49 30 8413 1433
>
On Wed 2023-05-10 12:12:50, Donald Buczek wrote:
> Is this series in a queue somewhere? Seems rather important and I don't find progress.
>
> Also, should be cc: [email protected] , right?
>
> CVE-2023-25012
It has Fixes, stable will pick it up.
And yes, "crafted USB device" can do a bad stuff. Severity is not too
high.
BR,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.