Re: [PATCH v5 03/12] Input: xbox_gip - Add controllable LED support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] LED classdev remains registered after driver disconnect, leading to a Use-After-Free vulnerability.
--

commit c366b72f3332d21395d97672e39c1566b23f91d5
Author: Vicki Pfau <vi@endrift.com>

Input: xbox_gip - Add controllable LED support

This commit adds controllable LED support for Xbox One controllers. It supports
both monochrome white LEDs and RGBW addressable LEDs for Elite 2 controllers,
exposing them as LED class devices.

> diff --git a/drivers/input/joystick/gip/gip-core.c b/drivers/input/joystick/gip/gip-core.c
> index 11196afdce33f..23ab0768580f5 100644
> --- a/drivers/input/joystick/gip/gip-core.c
> +++ b/drivers/input/joystick/gip/gip-core.c

[ ... ]

> +static int gip_guide_led_probe(struct gip_attachment *attachment, struct device *dev)
> +{
> +	int rc = 0;
> +
> +	if (!gip_supports_system_message(attachment, GIP_CMD_LED, false))
> +		return 0;
> +
> +	if (attachment->features & GIP_FEATURE_GUIDE_COLOR) {
> +		struct mc_subled *mc_led_info;
> +		struct led_classdev_mc *mc_cdev = &attachment->guide_led.color;
> +		struct led_classdev *cdev = &mc_cdev->led_cdev;
> +
> +		mc_led_info = devm_kcalloc(dev, 4,
> +			sizeof(*mc_led_info), GFP_KERNEL);
> +		if (!mc_led_info)
> +			return -ENOMEM;
> +
> +		mc_led_info[0].color_index = LED_COLOR_ID_RED;
> +		mc_led_info[0].intensity = 51;
> +		mc_led_info[1].color_index = LED_COLOR_ID_GREEN;
> +		mc_led_info[1].intensity = 51;
> +		mc_led_info[2].color_index = LED_COLOR_ID_BLUE;
> +		mc_led_info[2].intensity = 51;
> +		mc_led_info[3].color_index = LED_COLOR_ID_WHITE;
> +		mc_led_info[3].intensity = 51;
> +
> +		mc_cdev->subled_info = mc_led_info;
> +		mc_cdev->num_colors = 4;
> +
> +		cdev->brightness = 51;
> +		cdev->max_brightness = 255;
> +		cdev->flags = LED_CORE_SUSPENDRESUME | LED_RETAIN_AT_SHUTDOWN;
> +		cdev->brightness_set_blocking = gip_guide_color_led_set;
> +		cdev->name = devm_kasprintf(dev, GFP_KERNEL,
> +			"%s:rgb:power", dev_name(dev));
> +		if (!cdev->name)
> +			rc = -ENOMEM;
> +
> +		if (!rc)
> +			rc = devm_led_classdev_multicolor_register(dev,
> +				mc_cdev);

[Severity: High]
Does binding this LED to the input device's lifecycle cause a use-after-free
if the controller is disconnected while an input event node is still open?

Since the devres action for unregistering the LED is bound to the input device
(&input->dev), the LED will remain registered as long as userspace holds an
evdev file descriptor open.

However, during driver unbind, gip_disconnect() calls:
devm_kfree(to_gip_device(attachment), attachment);

This immediately frees the attachment structure containing the registered 
led_classdev and led_classdev_mc structures. Subsequent accesses to the LED's 
sysfs attributes (such as changing brightness via gip_guide_color_led_set) 
would then dereference this freed memory.

Should this register against the physical device instead, such as 
to_gip_device(attachment), to ensure the LED is unregistered synchronously 
during driver unbind?

[ ... ]

> +	} else {
> +		struct led_classdev *cdev = &attachment->guide_led.standard;
> +
> +		cdev->max_brightness = GIP_LED_GUIDE_MAX_BRIGHTNESS;
> +		cdev->brightness = GIP_LED_GUIDE_INIT_BRIGHTNESS;
> +		cdev->flags = LED_CORE_SUSPENDRESUME | LED_RETAIN_AT_SHUTDOWN;
> +		cdev->brightness_set_blocking = gip_guide_led_set;
> +		cdev->name = devm_kasprintf(dev, GFP_KERNEL,
> +			"%s:white:power", dev_name(dev));
> +		if (!cdev->name)
> +			return -ENOMEM;
> +
> +		rc = devm_led_classdev_register(dev, cdev);

[Severity: High]
Can this similarly cause a use-after-free since the standard LED is also
registered using the input device as the devres parent?

[ ... ]

> @@ -1150,6 +1261,10 @@ static int gip_setup_input_device(struct gip_attachment *attachment)
>  	if (rc)
>  		goto err_free_device;
>  
> +	rc = gip_guide_led_probe(attachment, &input->dev);

[Severity: High]
By passing &input->dev instead of the physical device to gip_guide_led_probe(),
the LED lifecycle becomes tied to the input device.

Input devices only run their devres actions when the last userspace reference
is released. If the LED memory resides in attachment, shouldn't the LED be
tied to the attachment's device so that unregistration happens before the
attachment is freed?

> +	if (rc)
> +		gip_err(attachment, "Failed to register LEDs: %d\n", rc);
> +
>  	return 0;
>  
>  err_free_device:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902010237.865772-1-vi@endrift.com?part=3