1. Patchew
  2. linux
  3. [PATCH 0/5] HID: manually unregister leds on device removal to prevent UAFs
  4. View patch

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

Pietro Borrello posted 5 patches 2 years, 10 months ago
There is a newer version of this series
[PATCH 3/5] HID: dualsense_remove: manually unregister leds
Posted by Pietro Borrello 2 years, 10 months ago 
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
Re: [PATCH 3/5] HID: dualsense_remove: manually unregister leds
Posted by Roderick Colenbrander 2 years, 10 months ago 
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
Re: [PATCH 3/5] HID: dualsense_remove: manually unregister leds
Posted by Pietro Borrello 2 years, 10 months ago 
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
Re: [PATCH 3/5] HID: dualsense_remove: manually unregister leds
Posted by Roderick Colenbrander 2 years, 10 months ago 
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
[PATCH v2 3/5] HID: dualsense_remove: manually unregister leds
Posted by Pietro Borrello 2 years, 10 months ago 
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
Posted by Pietro Borrello 2 years, 10 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.