linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/5] HID: bigben_remove: manually unregister leds
  2023-01-26  0:24 [PATCH 0/5] HID: manually unregister leds on device removal to prevent UAFs Pietro Borrello
@ 2023-01-26  0:24 ` Pietro Borrello
  2023-01-26  0:24 ` [PATCH 2/5] HID: asus_remove: manually unregister led Pietro Borrello
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 15+ messages in thread
From: Pietro Borrello @ 2023-01-26  0:24 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires, Hanno Zulla, Carlo Caione,
	Pavel Machek, Lee Jones, Roderick Colenbrander, Sven Eckelmann
  Cc: linux-leds, Cristiano Giuffrida, Bos, H.J.,
	Jakob Koschel, linux-input, linux-kernel, Jiri Kosina,
	Roderick Colenbrander, Pietro Borrello

Unregister the LED controllers before device removal, as
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 <borrello@diag.uniroma1.it>
---
 drivers/hid/hid-bigbenff.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/hid/hid-bigbenff.c b/drivers/hid/hid-bigbenff.c
index e8b16665860d..d3201b755595 100644
--- a/drivers/hid/hid-bigbenff.c
+++ b/drivers/hid/hid-bigbenff.c
@@ -306,9 +306,14 @@ static enum led_brightness bigben_get_led(struct led_classdev *led)
 
 static void bigben_remove(struct hid_device *hid)
 {
+	int n;
 	struct bigben_device *bigben = hid_get_drvdata(hid);
 
 	bigben->removed = true;
+	for (n = 0; n < NUM_LEDS; n++) {
+		if (bigben->leds[n])
+			devm_led_classdev_unregister(&hid->dev, bigben->leds[n]);
+	}
 	cancel_work_sync(&bigben->worker);
 	hid_hw_stop(hid);
 }

-- 
2.25.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 0/5] HID: manually unregister leds on device removal to prevent UAFs
@ 2023-01-26  0:24 Pietro Borrello
  2023-01-26  0:24 ` [PATCH 1/5] HID: bigben_remove: manually unregister leds Pietro Borrello
                   ` (5 more replies)
  0 siblings, 6 replies; 15+ messages in thread
From: Pietro Borrello @ 2023-01-26  0:24 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires, Hanno Zulla, Carlo Caione,
	Pavel Machek, Lee Jones, Roderick Colenbrander, Sven Eckelmann
  Cc: linux-leds, Cristiano Giuffrida, Bos, H.J.,
	Jakob Koschel, linux-input, linux-kernel, Jiri Kosina,
	Roderick Colenbrander, Pietro Borrello

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 <jikos@kernel.org>
To: Benjamin Tissoires <benjamin.tissoires@redhat.com>
To: Hanno Zulla <kontakt@hanno.de>
To: Carlo Caione <carlo@endlessm.com>
To: Pavel Machek <pavel@ucw.cz>
To: Lee Jones <lee@kernel.org>
To: Roderick Colenbrander <roderick.colenbrander@sony.com>
To: Sven Eckelmann <sven@narfation.org>
Cc: linux-leds@vger.kernel.org
Cc: Cristiano Giuffrida <c.giuffrida@vu.nl>
Cc: "Bos, H.J." <h.j.bos@vu.nl>
Cc: Jakob Koschel <jkl820.git@gmail.com>
Cc: linux-input@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: Roderick Colenbrander <roderick@gaikai.com>
Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>

---
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 <borrello@diag.uniroma1.it>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 2/5] HID: asus_remove: manually unregister led
  2023-01-26  0:24 [PATCH 0/5] HID: manually unregister leds on device removal to prevent UAFs Pietro Borrello
  2023-01-26  0:24 ` [PATCH 1/5] HID: bigben_remove: manually unregister leds Pietro Borrello
@ 2023-01-26  0:24 ` Pietro Borrello
  2023-01-26  0:24 ` [PATCH 3/5] HID: dualsense_remove: manually unregister leds Pietro Borrello
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 15+ messages in thread
From: Pietro Borrello @ 2023-01-26  0:24 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires, Hanno Zulla, Carlo Caione,
	Pavel Machek, Lee Jones, Roderick Colenbrander, Sven Eckelmann
  Cc: linux-leds, Cristiano Giuffrida, Bos, H.J.,
	Jakob Koschel, linux-input, linux-kernel, Jiri Kosina,
	Roderick Colenbrander, Pietro Borrello

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 <borrello@diag.uniroma1.it>
---
 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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/5] HID: dualsense_remove: manually unregister leds
  2023-01-26  0:24 [PATCH 0/5] HID: manually unregister leds on device removal to prevent UAFs Pietro Borrello
  2023-01-26  0:24 ` [PATCH 1/5] HID: bigben_remove: manually unregister leds Pietro Borrello
  2023-01-26  0:24 ` [PATCH 2/5] HID: asus_remove: manually unregister led Pietro Borrello
@ 2023-01-26  0:24 ` Pietro Borrello
  2023-01-26  0:43   ` Roderick Colenbrander
  2023-01-26  0:24 ` [PATCH 4/5] HID: dualshock4_remove: " Pietro Borrello
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 15+ messages in thread
From: Pietro Borrello @ 2023-01-26  0:24 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires, Hanno Zulla, Carlo Caione,
	Pavel Machek, Lee Jones, Roderick Colenbrander, Sven Eckelmann
  Cc: linux-leds, Cristiano Giuffrida, Bos, H.J.,
	Jakob Koschel, linux-input, linux-kernel, Jiri Kosina,
	Roderick Colenbrander, Pietro Borrello

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 <borrello@diag.uniroma1.it>
---
 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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 4/5] HID: dualshock4_remove: manually unregister leds
  2023-01-26  0:24 [PATCH 0/5] HID: manually unregister leds on device removal to prevent UAFs Pietro Borrello
                   ` (2 preceding siblings ...)
  2023-01-26  0:24 ` [PATCH 3/5] HID: dualsense_remove: manually unregister leds Pietro Borrello
@ 2023-01-26  0:24 ` Pietro Borrello
  2023-01-26  0:24 ` [PATCH 5/5] HID: sony_remove: " Pietro Borrello
  2023-05-10 10:12 ` [PATCH 0/5] HID: manually unregister leds on device removal to prevent UAFs Donald Buczek
  5 siblings, 0 replies; 15+ messages in thread
From: Pietro Borrello @ 2023-01-26  0:24 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires, Hanno Zulla, Carlo Caione,
	Pavel Machek, Lee Jones, Roderick Colenbrander, Sven Eckelmann
  Cc: linux-leds, Cristiano Giuffrida, Bos, H.J.,
	Jakob Koschel, linux-input, linux-kernel, Jiri Kosina,
	Roderick Colenbrander, Pietro Borrello

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 <borrello@diag.uniroma1.it>
---
 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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 5/5] HID: sony_remove: manually unregister leds
  2023-01-26  0:24 [PATCH 0/5] HID: manually unregister leds on device removal to prevent UAFs Pietro Borrello
                   ` (3 preceding siblings ...)
  2023-01-26  0:24 ` [PATCH 4/5] HID: dualshock4_remove: " Pietro Borrello
@ 2023-01-26  0:24 ` Pietro Borrello
  2023-01-26  8:32   ` Sven Eckelmann
  2023-05-10 10:12 ` [PATCH 0/5] HID: manually unregister leds on device removal to prevent UAFs Donald Buczek
  5 siblings, 1 reply; 15+ messages in thread
From: Pietro Borrello @ 2023-01-26  0:24 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires, Hanno Zulla, Carlo Caione,
	Pavel Machek, Lee Jones, Roderick Colenbrander, Sven Eckelmann
  Cc: linux-leds, Cristiano Giuffrida, Bos, H.J.,
	Jakob Koschel, linux-input, linux-kernel, Jiri Kosina,
	Roderick Colenbrander, Pietro Borrello

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 <borrello@diag.uniroma1.it>
---
 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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/5] HID: dualsense_remove: manually unregister leds
  2023-01-26  0:24 ` [PATCH 3/5] HID: dualsense_remove: manually unregister leds Pietro Borrello
@ 2023-01-26  0:43   ` Roderick Colenbrander
  2023-01-26  0:47     ` Roderick Colenbrander
  2023-01-26  1:01     ` [PATCH 3/5] HID: dualsense_remove: " Pietro Borrello
  0 siblings, 2 replies; 15+ messages in thread
From: Roderick Colenbrander @ 2023-01-26  0:43 UTC (permalink / raw)
  To: Pietro Borrello
  Cc: Jiri Kosina, Benjamin Tissoires, Hanno Zulla, Carlo Caione,
	Pavel Machek, Lee Jones, Roderick Colenbrander, Sven Eckelmann,
	linux-leds, Cristiano Giuffrida, Bos, H.J.,
	Jakob Koschel, linux-input, linux-kernel, Jiri Kosina,
	Roderick Colenbrander

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
<borrello@diag.uniroma1.it> 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 <borrello@diag.uniroma1.it>
> ---
>  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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/5] HID: dualsense_remove: manually unregister leds
  2023-01-26  0:43   ` Roderick Colenbrander
@ 2023-01-26  0:47     ` Roderick Colenbrander
  2023-01-26 12:09       ` [PATCH v2 " Pietro Borrello
  2023-01-26 12:11       ` [PATCH v2 4/5] HID: dualshock4_remove: " Pietro Borrello
  2023-01-26  1:01     ` [PATCH 3/5] HID: dualsense_remove: " Pietro Borrello
  1 sibling, 2 replies; 15+ messages in thread
From: Roderick Colenbrander @ 2023-01-26  0:47 UTC (permalink / raw)
  To: Pietro Borrello
  Cc: Jiri Kosina, Benjamin Tissoires, Hanno Zulla, Pavel Machek,
	Lee Jones, Roderick Colenbrander, Sven Eckelmann, linux-leds,
	Cristiano Giuffrida, Bos, H.J.,
	Jakob Koschel, linux-input, linux-kernel, Jiri Kosina,
	Roderick Colenbrander

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
<thunderbird2k@gmail.com> 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
> <borrello@diag.uniroma1.it> 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 <borrello@diag.uniroma1.it>
> > ---
> >  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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/5] HID: dualsense_remove: manually unregister leds
  2023-01-26  0:43   ` Roderick Colenbrander
  2023-01-26  0:47     ` Roderick Colenbrander
@ 2023-01-26  1:01     ` Pietro Borrello
  1 sibling, 0 replies; 15+ messages in thread
From: Pietro Borrello @ 2023-01-26  1:01 UTC (permalink / raw)
  To: Roderick Colenbrander
  Cc: Jiri Kosina, Benjamin Tissoires, Hanno Zulla, Carlo Caione,
	Pavel Machek, Lee Jones, Roderick Colenbrander, Sven Eckelmann,
	linux-leds, Cristiano Giuffrida, Bos, H.J.,
	Jakob Koschel, linux-input, linux-kernel, Jiri Kosina,
	Roderick Colenbrander

On Thu, 26 Jan 2023 at 01:44, Roderick Colenbrander
<thunderbird2k@gmail.com> 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/5] HID: sony_remove: manually unregister leds
  2023-01-26  0:24 ` [PATCH 5/5] HID: sony_remove: " Pietro Borrello
@ 2023-01-26  8:32   ` Sven Eckelmann
  0 siblings, 0 replies; 15+ messages in thread
From: Sven Eckelmann @ 2023-01-26  8:32 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires, Hanno Zulla, Carlo Caione,
	Pavel Machek, Lee Jones, Roderick Colenbrander, Pietro Borrello
  Cc: linux-leds, Cristiano Giuffrida, Bos, H.J.,
	Jakob Koschel, linux-input, linux-kernel, Jiri Kosina,
	Roderick Colenbrander, Pietro Borrello

[-- Attachment #1: Type: text/plain, Size: 535 bytes --]

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 <borrello@diag.uniroma1.it>
> ---
>  drivers/hid/hid-sony.c | 8 ++++++++
>  1 file changed, 8 insertions(+)


Reviewed-by: Sven Eckelmann <sven@narfation.org>

Thanks,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 3/5] HID: dualsense_remove: manually unregister leds
  2023-01-26  0:47     ` Roderick Colenbrander
@ 2023-01-26 12:09       ` Pietro Borrello
  2023-01-26 12:11       ` [PATCH v2 4/5] HID: dualshock4_remove: " Pietro Borrello
  1 sibling, 0 replies; 15+ messages in thread
From: Pietro Borrello @ 2023-01-26 12:09 UTC (permalink / raw)
  To: Roderick Colenbrander
  Cc: Pietro Borrello, linux-leds, Cristiano Giuffrida, Bos, H.J.,
	Jakob Koschel, linux-input, linux-kernel, Jiri Kosina,
	Roderick Colenbrander, Jiri Kosina, Benjamin Tissoires,
	Hanno Zulla, Pavel Machek, Lee Jones, Roderick Colenbrander,
	Sven Eckelmann

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 <borrello@diag.uniroma1.it>

---

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/20230125-hid-unregister-leds-v1-3-9a5192dcef16@diag.uniroma1.it/
---
 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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 4/5] HID: dualshock4_remove: manually unregister leds
  2023-01-26  0:47     ` Roderick Colenbrander
  2023-01-26 12:09       ` [PATCH v2 " Pietro Borrello
@ 2023-01-26 12:11       ` Pietro Borrello
  1 sibling, 0 replies; 15+ messages in thread
From: Pietro Borrello @ 2023-01-26 12:11 UTC (permalink / raw)
  To: Roderick Colenbrander
  Cc: Pietro Borrello, linux-leds, Cristiano Giuffrida, Bos, H.J.,
	Jakob Koschel, linux-input, linux-kernel, Jiri Kosina,
	Roderick Colenbrander, Jiri Kosina, Benjamin Tissoires,
	Hanno Zulla, Pavel Machek, Lee Jones, Roderick Colenbrander,
	Sven Eckelmann

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 <borrello@diag.uniroma1.it>

---

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/20230125-hid-unregister-leds-v1-4-9a5192dcef16@diag.uniroma1.it/
---
 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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/5] HID: manually unregister leds on device removal to prevent UAFs
  2023-01-26  0:24 [PATCH 0/5] HID: manually unregister leds on device removal to prevent UAFs Pietro Borrello
                   ` (4 preceding siblings ...)
  2023-01-26  0:24 ` [PATCH 5/5] HID: sony_remove: " Pietro Borrello
@ 2023-05-10 10:12 ` Donald Buczek
  2023-05-10 10:59   ` Benjamin Tissoires
  2023-05-10 11:41   ` Pavel Machek
  5 siblings, 2 replies; 15+ messages in thread
From: Donald Buczek @ 2023-05-10 10:12 UTC (permalink / raw)
  To: Pietro Borrello, Jiri Kosina, Benjamin Tissoires, Hanno Zulla,
	Carlo Caione, Pavel Machek, Lee Jones, Roderick Colenbrander,
	Sven Eckelmann
  Cc: linux-leds, Cristiano Giuffrida, Bos, H.J.,
	Jakob Koschel, linux-input, linux-kernel, Jiri Kosina,
	Roderick Colenbrander

Is this series in a queue somewhere? Seems rather important and I don't find progress.

Also, should be cc: stable@vger.kernel.org , 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 <jikos@kernel.org>
> To: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> To: Hanno Zulla <kontakt@hanno.de>
> To: Carlo Caione <carlo@endlessm.com>
> To: Pavel Machek <pavel@ucw.cz>
> To: Lee Jones <lee@kernel.org>
> To: Roderick Colenbrander <roderick.colenbrander@sony.com>
> To: Sven Eckelmann <sven@narfation.org>
> Cc: linux-leds@vger.kernel.org
> Cc: Cristiano Giuffrida <c.giuffrida@vu.nl>
> Cc: "Bos, H.J." <h.j.bos@vu.nl>
> Cc: Jakob Koschel <jkl820.git@gmail.com>
> Cc: linux-input@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Jiri Kosina <jkosina@suse.cz>
> Cc: Roderick Colenbrander <roderick@gaikai.com>
> Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
> 
> ---
> 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
buczek@molgen.mpg.de
Tel: +49 30 8413 1433

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/5] HID: manually unregister leds on device removal to prevent UAFs
  2023-05-10 10:12 ` [PATCH 0/5] HID: manually unregister leds on device removal to prevent UAFs Donald Buczek
@ 2023-05-10 10:59   ` Benjamin Tissoires
  2023-05-10 11:41   ` Pavel Machek
  1 sibling, 0 replies; 15+ messages in thread
From: Benjamin Tissoires @ 2023-05-10 10:59 UTC (permalink / raw)
  To: Donald Buczek
  Cc: Pietro Borrello, Jiri Kosina, Hanno Zulla, Carlo Caione,
	Pavel Machek, Lee Jones, Roderick Colenbrander, Sven Eckelmann,
	linux-leds, Cristiano Giuffrida, Bos, H.J.,
	Jakob Koschel, linux-input, linux-kernel, Jiri Kosina,
	Roderick Colenbrander

On Wed, May 10, 2023 at 12:21 PM Donald Buczek <buczek@molgen.mpg.de> 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: stable@vger.kernel.org , 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 <jikos@kernel.org>
> > To: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > To: Hanno Zulla <kontakt@hanno.de>
> > To: Carlo Caione <carlo@endlessm.com>
> > To: Pavel Machek <pavel@ucw.cz>
> > To: Lee Jones <lee@kernel.org>
> > To: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > To: Sven Eckelmann <sven@narfation.org>
> > Cc: linux-leds@vger.kernel.org
> > Cc: Cristiano Giuffrida <c.giuffrida@vu.nl>
> > Cc: "Bos, H.J." <h.j.bos@vu.nl>
> > Cc: Jakob Koschel <jkl820.git@gmail.com>
> > Cc: linux-input@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Cc: Jiri Kosina <jkosina@suse.cz>
> > Cc: Roderick Colenbrander <roderick@gaikai.com>
> > Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
> >
> > ---
> > 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
> buczek@molgen.mpg.de
> Tel: +49 30 8413 1433
>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/5] HID: manually unregister leds on device removal to prevent UAFs
  2023-05-10 10:12 ` [PATCH 0/5] HID: manually unregister leds on device removal to prevent UAFs Donald Buczek
  2023-05-10 10:59   ` Benjamin Tissoires
@ 2023-05-10 11:41   ` Pavel Machek
  1 sibling, 0 replies; 15+ messages in thread
From: Pavel Machek @ 2023-05-10 11:41 UTC (permalink / raw)
  To: Donald Buczek
  Cc: Pietro Borrello, Jiri Kosina, Benjamin Tissoires, Hanno Zulla,
	Carlo Caione, Lee Jones, Roderick Colenbrander, Sven Eckelmann,
	linux-leds, Cristiano Giuffrida, Bos, H.J.,
	Jakob Koschel, linux-input, linux-kernel, Jiri Kosina,
	Roderick Colenbrander

[-- Attachment #1: Type: text/plain, Size: 437 bytes --]

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: stable@vger.kernel.org , 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2023-05-10 11:42 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-26  0:24 [PATCH 0/5] HID: manually unregister leds on device removal to prevent UAFs Pietro Borrello
2023-01-26  0:24 ` [PATCH 1/5] HID: bigben_remove: manually unregister leds Pietro Borrello
2023-01-26  0:24 ` [PATCH 2/5] HID: asus_remove: manually unregister led Pietro Borrello
2023-01-26  0:24 ` [PATCH 3/5] HID: dualsense_remove: manually unregister leds Pietro Borrello
2023-01-26  0:43   ` Roderick Colenbrander
2023-01-26  0:47     ` Roderick Colenbrander
2023-01-26 12:09       ` [PATCH v2 " Pietro Borrello
2023-01-26 12:11       ` [PATCH v2 4/5] HID: dualshock4_remove: " Pietro Borrello
2023-01-26  1:01     ` [PATCH 3/5] HID: dualsense_remove: " Pietro Borrello
2023-01-26  0:24 ` [PATCH 4/5] HID: dualshock4_remove: " Pietro Borrello
2023-01-26  0:24 ` [PATCH 5/5] HID: sony_remove: " Pietro Borrello
2023-01-26  8:32   ` Sven Eckelmann
2023-05-10 10:12 ` [PATCH 0/5] HID: manually unregister leds on device removal to prevent UAFs Donald Buczek
2023-05-10 10:59   ` Benjamin Tissoires
2023-05-10 11:41   ` Pavel Machek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).