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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Devres actions on `input->dev` are delayed until userspace drops all references, causing the power supply to outlive the `attachment` structure and resulting in a Use-After-Free.
- [High] Manually unregistering a devm-managed power supply and freeing its name on an error path leads to a double-unregister and UAF when devres cleanup executes.
--

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

Input: xbox_gip - Add battery support

This commit adds battery support to the xbox_gip driver by exposing the
controller's battery information as a power_supply device.

> diff --git a/drivers/input/joystick/gip/gip-core.c b/drivers/input/joystick/gip/gip-core.c
> --- a/drivers/input/joystick/gip/gip-core.c
> +++ b/drivers/input/joystick/gip/gip-core.c
[ ... ]
> @@ -1199,6 +1206,106 @@ static int gip_guide_led_probe(struct gip_attachment *attachment, struct device
[ ... ]
> +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 = devm_power_supply_register(dev,
> +		&attachment->battery_desc, &supply_config);
> +	if (IS_ERR(attachment->battery)) {
> +		rc = PTR_ERR(attachment->battery);
> +		goto err_out;
> +	}
> +
> +	rc = power_supply_powers(attachment->battery, dev);
> +	if (rc < 0) {
> +		power_supply_unregister(attachment->battery);
> +		goto err_out;
> +	}

[Severity: High]
Because attachment->battery was allocated using devm_power_supply_register(),
a devres action is already queued to unregister it. If power_supply_powers()
fails and we manually call power_supply_unregister() here, will devres
attempt to unregister it again later when the input device is cleaned up,
leading to a double-free?

[ ... ]
> @@ -1494,6 +1604,8 @@ static void gip_free_devices(struct gip_attachment *attachment)
>  
>  	rcu_assign_pointer(attachment->input, NULL);
>  	rcu_assign_pointer(attachment->hdev, NULL);
> +	/* The following are freed by devres */
> +	attachment->battery = NULL;
>  	synchronize_rcu();

[Severity: High]
The battery was registered with devm using the input device (input->dev).
Since input device devres actions are delayed until userspace drops all
evdev file descriptor references, does setting attachment->battery to NULL
here leave the power supply active after the driver disconnects?

If a user reads the sysfs attributes for the power supply after the device
is unplugged, could gip_battery_get_property() attempt to access the freed
attachment structure?

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