| Message ID | 20230125-hid-unregister-leds-v1-3-9a5192dcef16@diag.uniroma1.it (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: manually unregister leds on device removal to prevent UAFs | expand |
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
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
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
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); }
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(+)