Re: [PATCH v6 05/12] Input: xbox_gip - Add battery support

[sashiko-bot@kernel.org]

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

New issues:
- [High] Attaching power_supply devres to the input_dev creates a Use-After-Free due to lifecycle mismatch.

Pre-existing issues:
- [High] Preexisting Use-After-Free in LED registration due to lifecycle mismatch with input_dev.
- [High] Improper cleanup ordering in gip_disconnect allows receive_message work to run after structures are freed.
--

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

Input: xbox_gip - Add battery support

Controllers are required to give information about the battery in the
controller, if present. This patch exposes that information as a
power_supply device.

> diff --git a/drivers/input/joystick/gip/gip-core.c b/drivers/input/joystick/gip/gip-core.c
> index dc174560ee49f..568877a4ce4b4 100644
> --- a/drivers/input/joystick/gip/gip-core.c
> +++ b/drivers/input/joystick/gip/gip-core.c
[ ... ]
> @@ -1206,6 +1213,97 @@ static int gip_guide_led_probe(struct gip_attachment *attachment, struct device
>  	return rc;
>  }
>  
> +static int gip_battery_create(struct gip_attachment *attachment, struct device *dev)
> +{
> +	struct power_supply_config supply_config = { .drv_data = attachment, };
> +	int rc;
> +
> +	attachment->battery_desc.properties = gip_battery_props;
> +	attachment->battery_desc.num_properties = ARRAY_SIZE(gip_battery_props);
> +	attachment->battery_desc.get_property = gip_battery_get_property;
> +	attachment->battery_desc.type = POWER_SUPPLY_TYPE_BATTERY;
> +	attachment->battery_desc.name = devm_kasprintf(dev, GFP_KERNEL, "gip-%s", dev_name(dev));
> +
> +	if (!attachment->battery_desc.name)
> +		return -ENOMEM;
> +
> +	attachment->battery = devm_power_supply_register(dev,
> +		&attachment->battery_desc, &supply_config);

[Severity: High]
Does registering the power supply against the refcounted input_dev create a
use-after-free risk?

Because input_dev is refcounted (e.g. via evdev), its destruction can be
delayed by a user holding an open file descriptor. However, the attachment
structure (passed as driver data) is freed synchronously upon device
disconnect.

If the device is unplugged, attachment is freed, but the power supply remains
registered until input_dev is closed. Reading the battery sysfs files during
this window will dereference the freed attachment.

> +	if (IS_ERR(attachment->battery)) {
> +		rc = PTR_ERR(attachment->battery);
> +		attachment->battery = NULL;
> +		devm_kfree(dev, attachment->battery_desc.name);
> +		return rc;
> +	}
> +
> +	power_supply_powers(attachment->battery, dev);
> +	return 0;
> +}
[ ... ]
> @@ -1319,6 +1417,9 @@ static int gip_setup_input_device(struct gip_attachment *attachment)
>  	rc = gip_guide_led_probe(attachment, &input->dev);

[Severity: High]
This is a pre-existing issue, but does registering the LED devices against
input->dev create a similar use-after-free risk?

In gip_guide_led_probe() in drivers/input/joystick/gip/gip-core.c,
devm_led_classdev_register() (and the multicolor variant) use input->dev as
the devres parent. Like the battery, the LED class devices will remain
registered as long as the input_dev exists.

Since the underlying attachment structure is freed upon unplug, this results
in a use-after-free when sysfs LED attributes are read or written after
disconnection.

>  	if (rc)
>  		gip_err(attachment, "Failed to register LEDs: %d\n", rc);
> +	rc = gip_battery_create(attachment, &input->dev);
> +	if (rc)
> +		gip_err(attachment, "Failed to register battery: %d\n", rc);
>  
>  	return 0;

[Severity: High]
This isn't a bug introduced by this patch, but could the cleanup ordering in
gip_disconnect() allow the receive_message work to run after the gip structure
is freed?

gip_disconnect() in drivers/input/joystick/gip/gip-core.c calls gip_shutdown()
(which does cancel_work_sync(&device->receive_message)) before it calls
usb_kill_urb(gip->data.urb_in):

drivers/input/joystick/gip/gip-core.c:gip_disconnect() {
    ...
    gip_shutdown(gip);
    ...
    usb_kill_urb(gip->data.urb_in);
    ...
}

If a USB completion interrupt arrives between cancel_work_sync() and
usb_kill_urb(), gip_urb_in() will execute with a success status and put the
work back on the queue:

drivers/input/joystick/gip/gip-core.c:gip_urb_in() {
    ...
    schedule_work(&gip->receive_message);
    ...
}

gip_disconnect() then frees the gip structure. When the workqueue runs, it
will dereference the freed gip pointer. Can we reverse the order to cancel the
work after killing the URBs?

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