[PATCH v2 0/5] HID: manually unregister leds on device removal to prevent UAFs

[PATCH v2 0/5] HID: manually unregister leds on device removal to prevent UAFs

[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: 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>

---
Changes in v2:
- dualshock4: Clarify UAF
- dualsense:  Clarify UAF
- dualsense:  Unregister multicolor led controller
- Link to v1: https://lore.kernel.org/r/20230125-hid-unregister-leds-v1-0-9a5192dcef16@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 | 10 ++++++++++
 drivers/hid/hid-sony.c        |  8 ++++++++
 4 files changed, 24 insertions(+)
---
base-commit: 2241ab53cbb5cdb08a6b2d4688feb13971058f65
change-id: 20230125-hid-unregister-leds-4cbf67099e1d

Best regards,
-- 
Pietro Borrello <borrello@diag.uniroma1.it>

[PATCH v2 1/5] HID: bigben_remove: manually unregister leds

[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

[PATCH v2 2/5] HID: asus_remove: manually unregister led

[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

[PATCH v2 3/5] HID: dualsense_remove: manually unregister leds

[Pietro Borrello]

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

[PATCH v2 4/5] HID: dualshock4_remove: manually unregister leds

[Pietro Borrello]

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

[PATCH v2 5/5] HID: sony_remove: manually unregister leds

[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>
Reviewed-by: Sven Eckelmann <sven@narfation.org>
---
 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

Re: [PATCH v2 0/5] HID: manually unregister leds on device removal to prevent UAFs

[Lee Jones]

> To: Jiri Kosina <jikos@kernel.org>
> To: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> To: Hanno Zulla <kontakt@hanno.de>
> To: Pavel Machek <pavel@ucw.cz>
> To: Lee Jones <lee@kernel.org>

Not entirely sure why I'm receiving these?

> 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>
> 
> ---
> Changes in v2:
> - dualshock4: Clarify UAF
> - dualsense:  Clarify UAF
> - dualsense:  Unregister multicolor led controller
> - Link to v1: https://lore.kernel.org/r/20230125-hid-unregister-leds-v1-0-9a5192dcef16@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 | 10 ++++++++++
>  drivers/hid/hid-sony.c        |  8 ++++++++
>  4 files changed, 24 insertions(+)
> ---
> base-commit: 2241ab53cbb5cdb08a6b2d4688feb13971058f65
> change-id: 20230125-hid-unregister-leds-4cbf67099e1d
> 
> Best regards,
> -- 
> Pietro Borrello <borrello@diag.uniroma1.it>

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 1/5] HID: bigben_remove: manually unregister leds

[Benjamin Tissoires]

Hi Pietro,

On Jan 31 2023, Pietro Borrello wrote:
> 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);

I don't think this is the correct fix. It would seem that we are
suddenly making the assumption that the devm mechanism would do things
in the wrong order, when the devm_led_classdev_unregister() should be
called *before* the devm_free() of the struct bigben_device.

However, you can trigger a bug, and thus we can analyse a little bit
further what is happening:

* user calls a function on the LED
* bigben_set_led() is called
* .remove() is being called at roughly the same time:
  - bigben->removed is set to true
  - cancel_work_sync() is called
* at that point, bigben_set_led() can not crash because
  led_classdev_unregister() flushes all of its workers, and thus
  prevents the call for dev_kfree(struct bigben_device)
* but now bigben_set_led() calls schedule_work()
* led_classdev_unregister() is now done and devm_kfree() is called for
  struct bigben_device
* now the led worker kicks in, and tries to access struct bigben_device
  and derefences it to get the value of bigben->removed (and
  bigben->report), which crashes.

So without your patch, the problem seems to be that we call a
schedule_work *after* we set bigben->removed to true and we call
cancel_work_sync().

And if you look at the hid-playstation driver, you'll see that the
schedule_work() call is encapsulated in a spinlock and a check to
ds->output_worker_initialized.

And this is why you can not reproduce on the hid-playstation driver,
because it is guarded against scheduling a worker when the driver is
being removed.

I think I prefer a lot more the playstation solution: having to manually
call a devm_release_free always feels wrong in a normal path. And also
by doing so, you might paper another problem that might happen on an
error path in probe for instance. Also, this means that the pattern you
saw is specific to some drivers, not all depending on how they make use
of workers.

Would you mind respinning that series with those comments?

Cheers,
Benjamin


>  	hid_hw_stop(hid);
>  }
> 
> -- 
> 2.25.1

Re: [PATCH v2 3/5] HID: dualsense_remove: manually unregister leds

[Benjamin Tissoires]

On Jan 31 2023, Pietro Borrello wrote:
> 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.

And so we don't need that patch (nor for hid-sony.c) because we have a
guard against scheduling a worker job when the device is being removed.

So please drop 3,4,5 from this series, they are just making the code
worse.

Cheers,
Benjamin

> 
> 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

Re: [PATCH v2 1/5] HID: bigben_remove: manually unregister leds

[Pietro Borrello]

On Thu, 9 Feb 2023 at 09:55, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> Hi Pietro,
>
> On Jan 31 2023, Pietro Borrello wrote:
> > 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);
>
> I don't think this is the correct fix. It would seem that we are
> suddenly making the assumption that the devm mechanism would do things
> in the wrong order, when the devm_led_classdev_unregister() should be
> called *before* the devm_free() of the struct bigben_device.
>
> However, you can trigger a bug, and thus we can analyse a little bit
> further what is happening:
>
> * user calls a function on the LED
> * bigben_set_led() is called
> * .remove() is being called at roughly the same time:
>   - bigben->removed is set to true
>   - cancel_work_sync() is called
> * at that point, bigben_set_led() can not crash because
>   led_classdev_unregister() flushes all of its workers, and thus
>   prevents the call for dev_kfree(struct bigben_device)
> * but now bigben_set_led() calls schedule_work()
> * led_classdev_unregister() is now done and devm_kfree() is called for
>   struct bigben_device
> * now the led worker kicks in, and tries to access struct bigben_device
>   and derefences it to get the value of bigben->removed (and
>   bigben->report), which crashes.
>
> So without your patch, the problem seems to be that we call a
> schedule_work *after* we set bigben->removed to true and we call
> cancel_work_sync().

Yes, this matches my intuition of what is happening here.
Thank you for the extensive description.

>
> And if you look at the hid-playstation driver, you'll see that the
> schedule_work() call is encapsulated in a spinlock and a check to
> ds->output_worker_initialized.
>
> And this is why you can not reproduce on the hid-playstation driver,
> because it is guarded against scheduling a worker when the driver is
> being removed.
>
> I think I prefer a lot more the playstation solution: having to manually
> call a devm_release_free always feels wrong in a normal path. And also
> by doing so, you might paper another problem that might happen on an
> error path in probe for instance. Also, this means that the pattern you
> saw is specific to some drivers, not all depending on how they make use
> of workers.
>

Yes, I agree this would be much cleaner.

> Would you mind respinning that series with those comments?

Sure, I'll work on that!

Best regards,
Pietro

Re: [PATCH v2 3/5] HID: dualsense_remove: manually unregister leds

[Pietro Borrello]

On Thu, 9 Feb 2023 at 09:59, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Jan 31 2023, Pietro Borrello wrote:
> > 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.
>
> And so we don't need that patch (nor for hid-sony.c) because we have a
> guard against scheduling a worker job when the device is being removed.
>
> So please drop 3,4,5 from this series, they are just making the code
> worse.

Sure.
I kept them only due to the Roderick Colenbrander's comment, but I'm happy
to remove them.
For reference:
> [...] I don't mind the change as it
> prevents the work scheduling functions to get called unnecessarily.
Link: https://lore.kernel.org/lkml/CAEc3jaCEKfqEJSV4=6GRj1Ry97xH+HwVSeEOZReNwkt=rLNvNQ@mail.gmail.com/

Thanks,
Pietro