Re: [PATCH v6 01/12] Input: xbox_gip - Add new driver for Xbox GIP

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [High] Incorrect teardown ordering in `gip_disconnect()` leaves pending `receive_message` work, causing a UAF.
- [High] Missing synchronous cancellation of `out_fragments` and `security` works during driver teardown.
- [High] Input devices are forcefully destroyed and recreated across system suspend/resume.
- [High] Use-After-Free in LED and Power Supply sysfs handlers due to devres lifetime mismatch.
- [High] URB input processing does not loop over packed messages and passes incorrect payload lengths.
- [High] Missing `cancel_delayed_work_sync` in `gip_probe` error path leads to use-after-free of attachment memory.
- [High] Functions returning negative error codes are declared with a `bool` return type, causing silent failures.
- [Medium] Unreachable cleanup code in `gip_init_output` error path leaks URBs and DMA buffers.
- [Medium] Devm-managed `input_dev` is leaked on dynamic device reset.
- [Medium] Concurrent modification of `attachment->seq_vendor` without holding `attachment->lock`.
- [Low] Mixing `guard()` scope-based cleanup with `goto`-based error handling in `gip_send_raw_message`.
--

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

Input: xbox_gip - Add new driver for Xbox GIP

This patch introduces a new driver for the Xbox One/Series controller protocol, 
known as the Gaming Input Protocol. It implements proper support for message 
length parsing, fragmented messages, and metadata auto-detection. The driver 
is designed to allow for future expansion to specialized device types and 
additional features.

> diff --git a/drivers/input/joystick/gip/gip-core.c b/drivers/input/joystick/gip/gip-core.c
> index 0e23811a6ecc1..0000000000000 100644
> --- a/drivers/input/joystick/gip/gip-core.c
> +++ b/drivers/input/joystick/gip/gip-core.c

[ ... ]

> @@ -391,5 +391,5 @@ static uint8_t gip_sequence_next(struct gip_attachment *attachment,
>  		}
>  	} else {
>  		seq = attachment->seq_vendor++;

[Severity: Medium]
Can this modification to the shared sequence state race with other callers?

Calling gip_send_vendor_message() from softirq context, like in gip_play_effect(),
increments attachment->seq_vendor without holding attachment->lock. This might
race with other message senders executing in process context and cause corrupted
sequence numbers.

[ ... ]

> @@ -533,5 +533,5 @@ static void gip_free_fragment(struct gip_out_fragment *fragment)
>  {
>  	cancel_delayed_work(&fragment->timeout);

[ ... ]

> @@ -3142,5 +3142,5 @@ static void gip_shutdown(struct gip_device *device)
>  
>  		guard(mutex)(&attachment->lock);
>  		gip_reset_attachment(attachment);

[Severity: High]
Does this non-synchronous cancellation allow the fragment timeout work to survive
driver teardown?

Because gip_reset_attachment() invokes gip_free_fragment() while holding
attachment->lock, a concurrently executing gip_out_fragment_timeout() will block
trying to acquire the same lock. 

Since cancel_delayed_work() does not wait for the blocked work to finish, the 
work item could resume after gip_disconnect() frees the attachment structure, 
leading to a use-after-free.

[ ... ]

> @@ -677,5 +677,5 @@ static int gip_send_raw_message(struct gip_attachment *attachment,
>  
>  	guard(spinlock_irqsave)(&attachment->device->message_lock);

[ ... ]

>  	return rc;
>  
>  err_free_fragment:
>  	if (fragment)
>  		gip_free_fragment(fragment);
>  	return rc;
>  }

[Severity: Low]
Does this mix guard scope-based cleanup with goto error handling?

The cleanup subsystem guidelines recommend avoiding goto-based cleanup labels
in the same function that uses guard declarations, as it creates confusing
ownership and resource lifecycle semantics.

[ ... ]

> @@ -1151,5 +1151,5 @@ static int gip_parse_message_metadata(struct device *dev,
>  
>  static bool gip_parse_metadata(struct device *dev,
>  	struct gip_metadata *metadata, const uint8_t *bytes, int num_bytes)
>  {

[ ... ]

>  	if (offset >= num_bytes)
>  		goto parse_err;

[ ... ]

>  parse_err:
>  	gip_metadata_free(dev, metadata);
>  	return rc;
>  }

[Severity: High]
Can this function return a false success on error conditions?

The function is declared with a bool return type but uses and returns negative
error codes like -EINVAL and -ENOMEM. If the offset bounds check is hit, rc is
exactly 0, causing the function to return false (0). 

Upstream callers like gip_handle_command_hello_device will implicitly cast this
to success, proceeding to use the freed metadata structure and risking 
out-of-bounds accesses.

[ ... ]

> @@ -1388,5 +1388,5 @@ static int gip_guide_led_probe(struct gip_attachment *attachment, struct device
>  		if (!rc)
>  			rc = devm_led_classdev_multicolor_register(dev,
>  				mc_cdev);

[ ... ]

> @@ -1473,5 +1473,5 @@ static int gip_battery_create(struct gip_attachment *attachment, struct device *
>  
>  	attachment->battery = devm_power_supply_register(dev,
>  		&attachment->battery_desc, &supply_config);

[ ... ]

> @@ -3183,5 +3183,5 @@ static void gip_disconnect(struct usb_interface *intf)
>  
>  		devm_kfree(to_gip_device(attachment), attachment->uniq);
>  		devm_kfree(to_gip_device(attachment), attachment);
>  	}

[Severity: High]
Is there a lifetime mismatch between the devres-managed sysfs interfaces and
the attachment structure?

The battery and LED class devices are registered using the input device
dev pointer as the devres owner. However, the attachment structure, which
is used as the driver data for these callbacks, is explicitly freed via
devm_kfree() in gip_disconnect(). 

If a userspace process holds an open file descriptor on the input event node, 
the input device memory and devres cleanup callbacks are deferred, leaving 
dangling pointers that can be dereferenced by userspace sysfs reads or writes.

[ ... ]

> @@ -1602,5 +1602,5 @@ int gip_setup_input_device(struct gip_attachment *attachment)
>  		return 0;
>  
>  	input = devm_input_allocate_device(to_gip_device(attachment));
>  	if (!input)

[ ... ]

> @@ -1861,5 +1861,5 @@ static void gip_free_devices(struct gip_attachment *attachment)
>  
>  	if (input)
>  		input_unregister_device(input);

[Severity: Medium]
Will this leak the devm-managed input device during a dynamic reset?

The input device is allocated with devm_input_allocate_device(), tying its
memory to the USB interface. When the device is reset dynamically, such as upon
receiving a new hello message, gip_reset_attachment() calls gip_free_devices()
and unregisters the device. 

Because the memory is devm-managed, it is not freed until the parent USB device
unbinds, resulting in a memory leak on every internal reset.

[Severity: High]
Also, does this forcefully destroy and recreate input devices across system 
suspend?

When the system suspends, gip_suspend() calls gip_shutdown(), which invokes
gip_reset_attachment() and gip_free_devices(). This unregisters the input device,
completely destroying the node. 

Applications will receive an EPOLLHUP and lose the controller permanently until 
restarted. Standard input drivers typically persist the input device across 
suspend and only pause I/O.

[ ... ]

> @@ -1902,5 +1902,5 @@ static void gip_reset_attachment(struct gip_attachment *attachment)
>  	cancel_delayed_work(&attachment->metadata_next);
>  	cancel_delayed_work(&attachment->in_fragment_timeout);

[ ... ]

> @@ -3108,5 +3108,5 @@ static int gip_probe(struct usb_interface *intf, const struct usb_device_id *id)
>  	if (rc) {
>  		gip_reset_attachment(attachment);
>  		gip_reset_metadata(attachment);
>  
>  		devm_kfree(to_gip_device(attachment), attachment->uniq);
>  		devm_kfree(to_gip_device(attachment), attachment);

[Severity: High]
Does this non-synchronous cancellation in the probe error path allow the
metadata work item to access freed memory?

If gip_interface_init() fails, gip_reset_attachment() uses cancel_delayed_work()
for metadata_next, and then the attachment memory is immediately freed by
devm_kfree(). 

If the delayed work was scheduled, such as for devices with the GIP_QUIRK_NO_HELLO 
quirk, it could execute concurrently and access the freed attachment structure.

[ ... ]

> @@ -2801,5 +2801,5 @@ static int gip_receive_message(struct gip_device *device, uint8_t *bytes,
>  		guard(mutex)(&attachment->lock);
>  		num_bytes -= offset;
>  		bytes += offset;
>  		fragment_offset = header.length;
>  		if (header.flags & GIP_FLAG_ACME)
>  			gip_acknowledge(attachment, &header, fragment_offset, bytes_remaining);
>  		rc = gip_handle_message(attachment, &header, bytes, num_bytes);

[Severity: High]
Does this code fail to handle multiple packed messages and pass incorrect 
payload lengths?

For non-fragmented messages, the function passes the entire remaining buffer size
(num_bytes) to gip_handle_message() instead of the actual payload size defined by
header.length. It also lacks a loop to process subsequent messages in the transfer.

Because Xbox controllers often pack multiple messages or include trailing padding,
strict length validation checks in downstream handlers will fail, causing valid
messages to be dropped.

[ ... ]

> @@ -2977,6 +2977,5 @@ static int gip_init_output(struct gip_interface *intf,
>  		intf->out_queue[i].data = usb_alloc_coherent(udev, intf->mtu, GFP_KERNEL,
>  			&intf->out_queue[i].urb->transfer_dma);
>  
>  		if (!intf->out_queue[i].data) {
>  			return -ENOMEM;
>  			goto err_free_urbs;
>  		}

[Severity: Medium]
Will this unreachable cleanup code leak URBs and DMA buffers on failure?

The return statement is placed immediately before the goto err_free_urbs label,
making the cleanup path completely unreachable if the coherent allocation fails.

[ ... ]

> @@ -3157,5 +3157,5 @@ static void gip_disconnect(struct usb_interface *intf)
>  		return;
>  
>  	gip_shutdown(gip);
>  
>  	usb_kill_urb(gip->data.urb_in);

[Severity: High]
Does the teardown ordering here leave a window for a use-after-free?

Calling gip_shutdown() invokes cancel_work_sync(&device->receive_message). 
However, usb_kill_urb() is not called until after gip_shutdown() returns. 

In that window, the hardware can complete a pending URB, triggering gip_urb_in(), 
which will successfully reschedule the receive_message work item. After 
gip_disconnect() completes and frees the gip_device structure, the pending 
work will execute and dereference the freed memory.

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