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

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

New issues:
- [High] Use-After-Free of `attachment` via battery sysfs properties due to devres lifecycle mismatch.
- [Medium] Missing `power_supply_changed()` call prevents userspace from receiving battery state updates.
- [Low] Data Race on battery status fields causes torn reads in sysfs.

Pre-existing issues:
- [High] Use-After-Free of `attachment` via LED sysfs properties due to devres lifecycle mismatch.
- [High] Dangling `attachment->input` pointer on `input_register_device` failure causes Use-After-Free.
- [High] AB-BA Deadlock between `gip_shutdown` and `gip_fragment_timeout`.
- [High] Out-of-Bounds Read in `gip_receive_fragment` due to missing bounds check.
--

--- Patch [5]: [PATCH v4 05/12] Input: xbox_gip - Add battery support ---
commit 467fca5f8d0f00e2e7affd3b50a79e158dfc1b7d
Author: Vicki Pfau <[email protected]>

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.
    
Signed-off-by: Vicki Pfau <[email protected]>

> diff --git a/drivers/input/joystick/gip/gip-core.c b/drivers/input/joystick/gip/gip-core.c
> index c833b10259ab8..d694ac5516395 100644
> --- a/drivers/input/joystick/gip/gip-core.c
> +++ b/drivers/input/joystick/gip/gip-core.c
[ ... ]
> @@ -1196,6 +1203,92 @@ static int gip_guide_led_probe(struct gip_attachment *attachment, struct device
>  	return rc;
>  }
>  
> +static int gip_battery_get_property(struct power_supply *psy,
> +	enum power_supply_property psp, union power_supply_propval *val)
> +{
> +	struct gip_attachment *attachment = power_supply_get_drvdata(psy);
> +
> +	guard(mutex)(&attachment->lock);

[Severity: Low]
Does taking this mutex fully protect the battery status fields?

Looking at gip_handle_command_status_device(), the fields are updated
directly without acquiring attachment->lock:

gip_handle_command_status_device()
	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;

Could a concurrent sysfs read observe torn or partially updated state?

[Severity: Medium]
Also, when gip_handle_command_status_device() updates the battery state,
is a call to power_supply_changed(attachment->battery) needed?

Without it, will userspace applications be notified of the updated battery
state, or will they only see changes if they manually poll sysfs?

[ ... ]
> +static int gip_battery_create(struct gip_attachment *attachment, struct device *dev)
> +{
> +	struct power_supply_config supply_config = { .drv_data = attachment, };
> +
> +	attachment->battery_desc.properties = gip_battery_props;
[ ... ]
> +	attachment->battery = devm_power_supply_register(dev,
> +		&attachment->battery_desc, &supply_config);

[Severity: High]
Could this lead to a use-after-free when the device is disconnected?

The power supply is registered using devm with the input device as its
parent, so its release is deferred until the input device's reference
count drops to zero.

However, gip_disconnect() immediately frees the attachment structure:

gip_disconnect()
	devm_kfree(to_gip_device(attachment), attachment);

If a userspace process holds the input evdev node open, the power supply
sysfs node remains registered. Could a subsequent sysfs read trigger
gip_battery_get_property() and dereference the freed attachment structure?

> +	if (IS_ERR(attachment->battery))
> +		return PTR_ERR(attachment->battery);
> +
> +	return power_supply_powers(attachment->battery, dev);
> +}
[ ... ]
> @@ -1308,6 +1401,9 @@ static int gip_setup_input_device(struct gip_attachment *attachment)
>  	rc = gip_guide_led_probe(attachment, &input->dev);

[Severity: High]
This isn't a bug introduced by this patch, but does gip_guide_led_probe()
have the same devm lifecycle mismatch as the battery creation?

The LEDs are registered using devm_led_classdev_multicolor_register()
with the input device as their parent.

Since gip_disconnect() frees the attachment structure manually, could writing
to the LED brightness sysfs node trigger callbacks that dereference the
freed attachment if userspace still holds the input device open?

[Severity: High]
This is also a pre-existing issue, but if gip_setup_input_device() fails,
is there a dangling pointer left behind?

The pointer is published before registration:

gip_setup_input_device()
	rcu_assign_pointer(attachment->input, input);
	rc = input_register_device(input);
	if (rc)
		goto err_free_device;

But the error path does not clear it:

err_free_device:
	input_free_device(input);
	return rc;

Could an incoming hardware event trigger gip_handle_ll_input_report()
and read the freed attachment->input pointer?

>  	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;
[ ... ]
> @@ -2623,6 +2719,7 @@ static int gip_shutdown(struct gip_device *device)
>  			hdev = rcu_dereference(attachment->hdev);
>  			rcu_read_unlock();
>  
> +			attachment->battery = NULL;

[Severity: High]
This isn't a bug introduced by this patch, but can gip_shutdown() deadlock
with gip_fragment_timeout()?

In gip_shutdown(), the lock is acquired before cancelling the work:

gip_shutdown()
	scoped_guard (mutex, &attachment->lock) {
		cancel_delayed_work_sync(&attachment->metadata_next);
		cancel_delayed_work_sync(&attachment->in_fragment_timeout);

If gip_fragment_timeout() runs concurrently, it attempts to acquire the
same mutex:

gip_fragment_timeout()
	guard(mutex)(&attachment->lock);

Could cancel_delayed_work_sync() block waiting for the work to finish,
while the work blocks waiting for the mutex?

>  			rcu_assign_pointer(attachment->input, NULL);
>  			rcu_assign_pointer(attachment->hdev, NULL);
>  			synchronize_rcu();

[Severity: High]
This is a pre-existing issue, but does gip_receive_fragment() have an
out-of-bounds read when processing fragments?

When parsing a non-initial message fragment, it uses header->length directly
in memcpy():

gip_receive_fragment()
	if (header->length != 0)
		memcpy(&attachment->in_fragment_data[*fragment_offset],
			&bytes[offset], header->length);

Is there a check to ensure that offset + header->length does not exceed
the actual size of the received USB buffer? Could a malicious USB device
sending a fragmented message with an artificially large length cause an
out-of-bounds read?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5