Re: [PATCH] HID: playstation: Support DualSense player LED brightness control

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Multi-variable data race on `ds->player_leds_state` and `ds->player_leds_brightness` in `dualsense_player_led_get_brightness()`.

Pre-existing issues:
- [High] Missing workqueue cancellation in the `dualsense_create()` and `dualshock4_create()` error paths causes a use-after-free if probe fails.
--

--- Patch [1]: [PATCH] HID: playstation: Support DualSense player LED brightness control ---
commit 9dd8a497081a6ac6ae8e211e738b3ef23a0669be
Author: Kateřina Medvědová <k8ie@mcld.eu>

    HID: playstation: Support DualSense player LED brightness control

This patch wires up the controls for the brightness byte, allowing
the player indicator brightness to be adjusted. It increases the
max_brightness of each player's LED classdev from 1 to 3, and correctly
updates the firmware encoding.

> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index f9dc9378cf777..78e43a176248a 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
[ ... ]
> @@ -1219,12 +1229,22 @@ static int dualsense_lightbar_set_brightness(struct led_classdev *cdev,
>  	return 0;
>  }
>  
> +/*
> + * The DualSense's player LEDs only support a single, shared brightness level
> + * for all lit LEDs -- there is no per-LED brightness control. We still expose
> + * per-LED on/off state plus 3 brightness levels through each LED classdev's
> + * max_brightness of 3; the last value written by any player LED classdev sets
> + * the shared level for all of them.
> + */
>  static enum led_brightness dualsense_player_led_get_brightness(struct led_classdev *led)
>  {
>  	struct hid_device *hdev = to_hid_device(led->dev->parent);
>  	struct dualsense *ds = hid_get_drvdata(hdev);
>  
> -	return !!(ds->player_leds_state & BIT(led - ds->player_leds));
> +	if (!(ds->player_leds_state & BIT(led - ds->player_leds)))
> +		return LED_OFF;
> +
> +	return DS_OUTPUT_PLAYER_LED_BRIGHTNESS_LOW + 1 - ds->player_leds_brightness;

[Severity: Medium]
Could this lockless read result in a multi-variable data race?

In dualsense_player_led_set_brightness(), ds->player_leds_state and
ds->player_leds_brightness are updated together under ds->base.lock.
If dualsense_player_led_get_brightness() reads them without the lock,
a concurrent read could observe the updated state but the old
brightness (or vice-versa due to compiler or CPU reordering), returning
an inconsistent value to userspace.

>  }
>  
>  static int dualsense_player_led_set_brightness(struct led_classdev *led, enum led_brightness value)
[ ... ]
> @@ -1742,15 +1774,15 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
>  	int i, ret;
>  
>  	static const struct ps_led_info player_leds_info[] = {
> -		{ LED_FUNCTION_PLAYER1, "white", 1, dualsense_player_led_get_brightness,
> +		{ LED_FUNCTION_PLAYER1, "white", 3, dualsense_player_led_get_brightness,
>  				dualsense_player_led_set_brightness },
[ ... ]
> -		{ LED_FUNCTION_PLAYER5, "white", 1, dualsense_player_led_get_brightness,
> +		{ LED_FUNCTION_PLAYER5, "white", 3, dualsense_player_led_get_brightness,
>  				dualsense_player_led_set_brightness }
>  	};
>  

[Severity: High]
This is a pre-existing issue, but missing workqueue cancellation in the
dualsense_create() and dualshock4_create() error paths could cause a
use-after-free if probe fails.

During probe, dualsense_set_lightbar() is called to set the initial lightbar
color, which implicitly schedules ds->output_worker:

drivers/hid/hid-playstation.c:dualsense_create() {
    ...
	dualsense_set_lightbar(ds, 0, 0, 128); /* blue */

	for (i = 0; i < ARRAY_SIZE(player_leds_info); i++) {
		const struct ps_led_info *led_info = &player_leds_info[i];

		ret = ps_led_register(ps_dev, &ds->player_leds[i], led_info);
		if (ret < 0)
			goto err;
	}
    ...
}

If a subsequent initialization step fails, the error path returns and allows
devres to free the allocated ds struct:

err:
	ps_devices_list_remove(ps_dev);
	return ERR_PTR(ret);
}

Because the worker is not cancelled, it remains in the system workqueue and
will dereference the freed ds pointer when it executes. Does the error path
need an explicit cancel_work_sync(&ds->output_worker) to prevent this? The
exact same vulnerability exists in dualshock4_create() due to
dualshock4_set_bt_poll_interval().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714180424.70662-1-k8ie@mcld.eu?part=1