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

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] LED class devices remain registered after the driver's private data (`struct gip_attachment`) is freed, leading to a Use-After-Free via sysfs attributes.
- [High] Double registration of the same embedded `led_classdev` during device reset causes list corruption.

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but a race condition in driver teardown leaves a window where `gip_rumble_work` can be scheduled and executed after `attachment` is freed, causing a Use-After-Free.
--

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

Input: xbox_gip - Add controllable LED support
    
Xbox One controllers have two different types of controllable LED support:
    
- Monochrome white, which most controllers have.
- RGBW addressible, which the Elite 2 controllers have.
    
This exposes both types as led cdevs.

> diff --git a/drivers/input/joystick/gip/gip-core.c b/drivers/input/joystick/gip/gip-core.c
> index b67b38c880cd3..9371d860a59b8 100644
> --- a/drivers/input/joystick/gip/gip-core.c
> +++ b/drivers/input/joystick/gip/gip-core.c
[ ... ]
> @@ -1056,9 +1056,120 @@ static int gip_send_guide_button_led(struct gip_attachment *attachment,
[ ... ]
> +static int gip_guide_led_probe(struct gip_attachment *attachment, struct device *dev)
> +{
[ ... ]
> +		if (!rc)
> +			rc = devm_led_classdev_multicolor_register(dev,
> +				mc_cdev);

[Severity: High]
Can this cause a double registration of the same embedded led_classdev
during a device reset?

If the controller unexpectedly resets (e.g., receiving a GIP_CMD_HELLO_DEVICE
message), the driver calls gip_reset_attachment(), which unregisters the
input_dev. It then re-initializes and allocates a new input_dev, calling
gip_guide_led_probe() again.

Because the previous input_dev's devres hasn't executed (if userspace kept it
open), the original LED registration is never undone. The driver then calls
devm_led_classdev_multicolor_register() on the exact same statically embedded
mc_cdev (which is still actively linked in leds_list). Double-registering a
node into a linked list corrupts its pointers.

> +
> +		if (rc)
> +			devm_kfree(dev, mc_led_info);
> +	} 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]
Similarly, does registering the standard embedded cdev here expose it to
the same double-registration vulnerability during a device reset?

> +
> +		if (rc)
> +			devm_kfree(dev, cdev->name);
> +	}
> +
> +	return rc;
> +}
[ ... ]
> @@ -1169,6 +1280,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: Critical]
Can tying the devres lifecycle of the LED to &input->dev lead to a
use-after-free?

During a physical disconnect, gip_disconnect() explicitly frees the
attachment structure. However, if userspace holds an open file descriptor
on the input device (e.g., via evdev), the input_dev reference count
remains positive, delaying its devres execution.

This leaves the embedded led_classdev registered and pointing to freed
memory. A subsequent write to the LED's sysfs attribute calls
gip_guide_led_set(), which dereferences the freed attachment structure.

[Severity: Critical]
This is a pre-existing issue, but does a race condition in driver teardown
leave a window where gip_rumble_work can be scheduled and executed after
attachment is freed, causing a use-after-free?

In gip_shutdown(), work is canceled synchronously:

    cancel_work_sync(&attachment->rumble_work);

However, while the input_dev is still registered, a concurrent thread in
userspace can invoke the EVIOCSFF ioctl, calling gip_play_effect() to
re-schedule the work.

Later in the teardown, gip_reset_attachment() uses the non-blocking
cancel_work():

    cancel_work(&attachment->rumble_work);

If the worker thread has already started executing, cancel_work() does not
wait for it. The worker blocks on guard(mutex)(&attachment->lock) held by
gip_shutdown(). gip_shutdown() then unregisters the input device and
returns. gip_disconnect() then immediately frees the attachment structure.
Finally, the worker thread unblocks, acquiring the lock and accessing the
freed attachment.

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

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