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

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The devm-managed power supply is tied to the `input_dev`'s lifecycle instead of the physical device, leading to a Use-After-Free of `attachment` when sysfs properties are read after unplug.
- [High] Memory and device leak of the power supply across metadata resets due to a devres mismatch when unregistering the input device.
- [High] Data race and potential Use-After-Free on `attachment->battery` during concurrent device status URB processing and device teardown.

Pre-existing issues:
- [Critical] Use-after-free of `gip_device` and `gip_attachment` due to incorrect teardown order of the URB and workqueue in `gip_disconnect`.
--

commit e93493b8a8fc4d66dfb37174e70166cba8bd408c
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 411b67399e018..92bfdd6d76826 100644
> --- a/drivers/input/joystick/gip/gip-core.c
> +++ b/drivers/input/joystick/gip/gip-core.c
[ ... ]
> @@ -1218,6 +1225,97 @@ 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_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: Critical]
Does this tie the power supply's devres lifecycle to the child input_dev 
rather than the parent attachment?

If dev is &input->dev, the power supply will outlive the attachment 
structure if a user unplugs the device while keeping the input device open. 

Since attachment is explicitly freed on disconnect, couldn't this lead to 
a use-after-free when the user subsequently reads sysfs properties (via 
gip_battery_get_property) which dereferences 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;
> +}
[ ... ]
> @@ -1514,6 +1615,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();
>  
>  	if (input)

[Severity: High]
Does this code assume devres will immediately free the battery upon
calling input_unregister_device()?

Because the devres lifecycle of input_dev depends on its parent (the USB
interface), its refcount doesn't drop to zero until the USB interface 
unbinds. 

If a device sends repeated reset requests that invoke gip_free_devices(),
could this leak power_supply devices and their sysfs entries indefinitely
across metadata resets?

> @@ -1647,10 +1750,14 @@ static int gip_handle_command_status_device(struct gip_attachment *attachment,
>  	if (num_bytes < 1)
>  		return -EINVAL;
>  
> -	attachment->status.base.battery_level = bytes[0] & 3;
> -	attachment->status.base.battery_type = (bytes[0] >> 2) & 3;
> -	attachment->status.base.charge = (bytes[0] >> 4) & 3;
> -	attachment->status.base.power_level = (bytes[0] >> 6) & 3;
> +	scoped_guard(spinlock_irqsave, &attachment->battery_lock) {
> +		attachment->status.base.battery_level = bytes[0] & 3;
> +		attachment->status.base.battery_type = (bytes[0] >> 2) & 3;
> +		attachment->status.base.charge = (bytes[0] >> 4) & 3;
> +		attachment->status.base.power_level = (bytes[0] >> 6) & 3;
> +	}
> +	if (attachment->battery)
> +		power_supply_changed(attachment->battery);

[Severity: High]
Is it safe to access attachment->battery here without RCU read-side 
protection or READ_ONCE()?

Since gip_handle_command_status_device() executes concurrently with 
device teardown, gip_free_devices() might concurrently clear the pointer 
(attachment->battery = NULL) and call synchronize_rcu(). 

Without rcu_read_lock() held here, synchronize_rcu() will not wait,
potentially allowing the battery pointer to be freed underneath this reader,
leading to a use-after-free.

[Severity: Critical]
This is a pre-existing issue, but can a use-after-free of gip_device and
gip_attachment happen due to an incorrect teardown order in gip_disconnect()?

Looking at gip_disconnect(), it calls gip_shutdown() which cancels the
receive_message workqueue. However, the URB that re-queues this work is 
not killed until gip_deinit_input() runs later.

Could this open a race window where an interrupt packet arrives just after 
cancel_work_sync(&gip->receive_message), allowing gip_urb_in() to 
reschedule the work? 

The device structures would then be freed by devm_kfree() at the end of
disconnect, and the newly scheduled work would dereference freed memory
when it executes.

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