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

Thank you for your contribution! Sashiko AI review found 17 potential issue(s) to consider:
- [High] Deadlock caused by calling `cancel_delayed_work_sync()` while holding a mutex required by the work function.
- [High] Lifecycle mismatch causing devres memory leak due to allocating against the parent USB device instead of the USB interface.
- [High] Active OUT URBs are freed without being killed, causing use-after-free and DMA corruption.
- [High] Lock inversion during devres cleanup triggers an AB-BA deadlock between `attachment->lock` and `cancel_work_sync`.
- [High] Buffer over-read in `gip_receive_fragment` for non-initial fragments due to missing bounds check.
- [High] Probe error path leaves an active IN URB running, leading to a Use-After-Free.
- [High] Dangling pointer left in `attachment->input` upon `input_register_device` failure.
- [High] Blind dereference of potential `ERR_PTR` returned from `gip_ensure_attachment`.
- [High] Undefined behavior from left shifting by 64 or more bits in `gip_decode_length`.
- [High] NULL pointer dereference when `in_fragment_data` allocation fails.
- [High] Missing NULL check after `devm_kmalloc` in `gip_send_raw_message`.
- [High] Use-After-Free due to LED/Battery callbacks operating on explicitly freed `attachment` structure.
- [High] Sleeping in atomic context by calling `usb_free_coherent` while holding a spinlock.
- [High] Memory leak of URBs and DMA buffers due to `return` combined with dead `goto` error handling.
- [High] Heap buffer overflow in `gip_receive_fragment` due to bypassed bounds check on exact offset match.
- [Medium] Bitwise OR logic typo in `gip_parse_preferred_types_metadata` ignores the high byte of the string length.
- [Medium] Memory leak of `attachment->uniq` caused by repeated `HELLO_DEVICE` messages.
--

--- Patch [1]: [PATCH v4 01/12] Input: xbox_gip - Add new driver for Xbox GIP ---

commit 8f3bdd937c628e28382af94b64282af26f6cbd22
Author: Vicki Pfau [off-list ref]
Subject: Input: xbox_gip - Add new driver for Xbox GIP

This commit introduces a new Gaming Input Protocol (GIP) driver for Xbox
One and Series controllers. It supports parsing message lengths, metadata,
and handles basic gamepad input functionality along with controller quirks.

[Severity: High]
Does this loop need a bounds check to prevent shifting by 64 or more bits?

If a device sends 10 or more consecutive bytes with the 0x80 bit set,
offset will reach 10, causing the shift to become 70. This results in
undefined behavior in C.

[ ... ]

+static int gip_send_raw_message(struct gip_attachment *attachment,
+	uint8_t message_type, uint8_t flags, uint8_t seq, const uint8_t *bytes,
+	int num_bytes)
+{

[ ... ]

+		fragment->total_length = num_bytes;
+		fragment->data = devm_kmalloc(to_gip_device(attachment),
+			fragment->total_length, GFP_ATOMIC);
+		fragment->fragment_offset = 0;
+		memcpy(fragment->data, bytes, num_bytes);

[Severity: High]
Is there a missing NULL check for the devm_kmalloc() return value?

If the atomic memory allocation fails under memory pressure, the subsequent
memcpy() will result in a NULL pointer dereference.

[ ... ]

+static int gip_parse_preferred_types_metadata(struct device *dev,
+	struct gip_device_metadata *dev_metadata, const uint8_t *bytes,
+	int length, int buffer_offset)
+{

[ ... ]

+	buffer_offset++;
+	for (i = 0; i < dev_metadata->num_preferred_types; i++) {
+		if (buffer_offset + 2 >= length)
+			return -EINVAL;
+
+		count = bytes[buffer_offset];
+		count |= bytes[buffer_offset];

[Severity: Medium]
Is there a typo in the bitwise OR assignment for the count?

By OR-ing the exact same byte against itself, the high byte of the 16-bit
length is ignored (it looks like it should read buffer_offset + 1). This
could cause strings larger than 255 bytes to parse incorrectly as 0,
desynchronizing the buffer offset.

[ ... ]

+static bool gip_handle_command_hello_device(struct gip_attachment *attachment,
+	const struct gip_header *header, const uint8_t *bytes, int num_bytes)
+{

[ ... ]

+	attachment->firmware_major_version = le16_to_cpu(message->firmware_major_version);
+	attachment->firmware_minor_version = le16_to_cpu(message->firmware_minor_version);
+	attachment->vendor_id = le16_to_cpu(message->vendor_id);
+	attachment->product_id = le16_to_cpu(message->product_id);
+	attachment->uniq = devm_kasprintf(to_gip_device(attachment),
+		GFP_KERNEL, "%llx", le64_to_cpu(message->device_id));

[Severity: Medium]
Could this cause a memory leak if the device repeatedly sends HELLO_DEVICE
messages?

The attachment->uniq pointer is assigned a newly allocated string each time
this command is handled, but any previously allocated string is not freed
prior to being overwritten.

[ ... ]

+int gip_setup_input_device(struct gip_attachment *attachment)
+{

[ ... ]

+	rcu_assign_pointer(attachment->input, input);
+	rc = input_register_device(input);
+	if (rc)
+		goto err_free_device;
+
+	rc = gip_guide_led_probe(attachment, &input->dev);

[ ... ]

+err_free_device:
+	input_free_device(input);
+	return rc;
+}

[Severity: High]
Should attachment->input be cleared to NULL in the error path?

If input_register_device() fails, input_free_device() is called, but
attachment->input remains set. If an interrupt arrives afterwards,
gip_handle_ll_input_report() could dereference the freed dangling pointer.

[ ... ]

+static void gip_reset_metadata(struct gip_attachment *attachment)
+{
+	struct input_dev *input;
+
+	if (attachment->got_metadata != GIP_METADATA_GOT)
+		return;
+
+	gip_metadata_free(to_gip_device(attachment), &attachment->metadata);
+	rcu_read_lock();
+	input = rcu_dereference(attachment->input);
+	rcu_read_unlock();
+	if (input) {
+		rcu_assign_pointer(attachment->input, NULL);
+		synchronize_rcu();
+		input_unregister_device(input);
+	}

[Severity: High]
Could this lead to an AB-BA deadlock between attachment->lock and
the power supply's changed_work cancelation?

When a new message fragment is processed, gip_receive_message() holds
attachment->lock. It might call into gip_reset_metadata(), which triggers
input_unregister_device(). This synchronously releases devres resources,
unregistering the battery power supply and flushing its work queue via
cancel_work_sync().

If the power supply worker thread is simultaneously executing
gip_battery_get_property(), it will attempt to acquire attachment->lock,
creating a deadlock.

[ ... ]

+static int gip_receive_fragment(struct gip_attachment *attachment,
+	const struct gip_header *header, int offset,
+	uint64_t *fragment_offset, uint16_t *bytes_remaining, uint8_t *bytes,
+	int num_bytes)
+{

[ ... ]

+		attachment->in_fragment_data = devm_kmalloc(to_gip_device(attachment),
+			attachment->in_total_length, GFP_KERNEL);
+		if (!attachment->in_fragment_data)
+			return -ENOMEM;

[Severity: High]
Does this error path leave the attachment in an inconsistent state?

If the allocation fails, the function returns -ENOMEM but leaves
attachment->in_fragment_message set to a valid message type. A subsequent
non-initial fragment matching the type would pass validation and call
memcpy() into the NULL in_fragment_data pointer.

[ ... ]

+		if (*fragment_offset != attachment->in_fragment_offset) {

[ ... ]

+			if (header->length && *fragment_offset != attachment->in_total_length) {
+				gip_warn_ratelimited(attachment,
+					"Received out of sequence fragment: claimed %llu, expected %d\n",
+					*fragment_offset, attachment->in_fragment_offset);
+				gip_acknowledge(attachment, header,
+					attachment->in_fragment_offset,
+					attachment->in_total_length - attachment->in_fragment_offset);
+				return -EPROTO;
+			}
+			gip_dbg_ratelimited(attachment,
+				"Received erroneous final fragment offset, proceeding anyway\n");
+			*bytes_remaining = 0;
+		} else if (*fragment_offset + header->length > attachment->in_total_length) {
+			gip_warn(attachment,
+				"Received too long fragment, %llu exceeds %d\n",
+				*fragment_offset + header->length, attachment->in_total_length);
+			gip_fragment_failed(attachment, header);
+			return -EINVAL;
+		} else {
+			*bytes_remaining = attachment->in_total_length -
+				(*fragment_offset + header->length);
+		}
+		if (header->length != 0)
+			memcpy(&attachment->in_fragment_data[*fragment_offset],
+				&bytes[offset], header->length);

[Severity: High]
Can this logic result in a heap buffer overflow?

If a device provides *fragment_offset == attachment->in_total_length and
header->length > 0, the first if block is evaluated. Since it exits the
block without returning, it skips the else if bounds check entirely and
proceeds to memcpy(), writing header->length bytes past the end of the
heap allocation.

[Severity: High]
Is there a missing bounds check for non-initial fragments?

For initial fragments (GIP_FLAG_INIT_FRAG), the driver checks if
header->length > num_bytes - offset. However, for non-initial fragments,
this validation appears to be missing before calling memcpy(). This allows
the device to specify a header->length larger than the actual received
packet, reading adjacent kernel heap memory into in_fragment_data.

[ ... ]

+	cancel_delayed_work_sync(&attachment->in_fragment_timeout);
+	schedule_delayed_work(&attachment->in_fragment_timeout, HZ);
+
+	return rc;
+}

[Severity: High]
Can this lead to an AB-BA deadlock with the work function?

gip_receive_fragment() is called from gip_receive_message() while holding
attachment->lock. It then calls cancel_delayed_work_sync(). If the delayed
work gip_fragment_timeout() is currently executing, it will attempt to
acquire attachment->lock, deadlocking the system.

[ ... ]

+static int gip_receive_message(struct gip_device *device, uint8_t *bytes,
+	int num_bytes)
+{

[ ... ]

+	is_fragment = header.flags & GIP_FLAG_FRAGMENT;
+	attachment_index = header.flags & GIP_FLAG_ATTACHMENT_MASK;
+	attachment = gip_ensure_attachment(device, attachment_index);
+
+	print_hex_dump_debug(KBUILD_MODNAME ": Received message: ",
+		DUMP_PREFIX_OFFSET, 16, 1, bytes, num_bytes, false);
+
+	guard(mutex)(&attachment->lock);

[Severity: High]
Does gip_ensure_attachment() need to be checked for an ERR_PTR return?

If memory allocation within gip_ensure_attachment() fails, it returns
ERR_PTR(-ENOMEM). Blindly calling guard(mutex)(&attachment->lock) with
an ERR_PTR will trigger a kernel panic.

[ ... ]

+static int gip_init_output(struct gip_interface *intf,
+	struct usb_endpoint_descriptor *ep_out)
+{

[ ... ]

+		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: High]
Does the early return cause a memory leak on the error path?

The code executes return -ENOMEM; immediately, bypassing the goto statement.
This bypasses the err_free_urbs cleanup logic, leaking all URBs and buffers
allocated in previous loop iterations.

[ ... ]

+static int gip_probe(struct usb_interface *intf, const struct usb_device_id *id)
+{

[ ... ]

+	gip = devm_kzalloc(&udev->dev, sizeof(*gip), GFP_KERNEL);
+	if (!gip)
+		return -ENOMEM;
+
+	gip->udev = udev;
+	gip->data.device = gip;

[Severity: High]
Are devres allocations incorrectly tied to the parent USB device?

The structure is allocated using devm_kzalloc(&udev->dev, ...). Since a
usb_driver is bound to the usb_interface, its devres allocations must be
tied to &intf->dev. Because they are tied to the parent device, unbinding
the driver via sysfs does not trigger devres cleanup, permanently leaking
memory until the physical device is unplugged.

[ ... ]

+	rc = gip_interface_init(&gip->data);
+	if (rc) {
+		devm_kfree(to_gip_device(gip), gip);
+		return rc;
+	}
+	/* Don't init audio interface -- we aren't using it yet */
+
+	usb_set_intfdata(intf, gip);
+
+	/* Pre-create the first attachment, as it should always exist */
+	attachment = gip_ensure_attachment(gip, 0);
+	if (IS_ERR(attachment))
+		return PTR_ERR(attachment);
+
+	return 0;
+}

[Severity: High]
Are we leaking the running URB if gip_ensure_attachment() fails?

The call to gip_interface_init() submits urb_in. If ensuring the first
attachment subsequently fails, the function returns without killing the
active URB. When the hardware interrupt fires, the USB core will attempt
to access the freed gip structure.

[ ... ]

+static int gip_shutdown(struct gip_device *device)
+{

[ ... ]

+	for (i = 0; i < MAX_ATTACHMENTS; i++) {
+		struct gip_attachment *attachment = device->attachments[i];
+		struct input_dev *input;
+
+		if (!attachment)
+			continue;
+
+		scoped_guard (mutex, &attachment->lock) {
+			cancel_delayed_work_sync(&attachment->metadata_next);
+			cancel_delayed_work_sync(&attachment->in_fragment_timeout);

[Severity: High]
Does this scoped_guard introduce a deadlock?

The scoped_guard acquires attachment->lock, and then calls
cancel_delayed_work_sync(). If the work functions (gip_retry_metadata or
gip_fragment_timeout) are running concurrently, they attempt to acquire
this same lock, causing an AB-BA deadlock during driver shutdown.

[ ... ]

+static void gip_disconnect(struct usb_interface *intf)
+{

[ ... ]

+	usb_kill_urb(gip->data.urb_in);
+	if (gip->audio.intf)
+		usb_kill_urb(gip->audio.urb_in);
+
+	gip_shutdown(gip);
+
+	spin_lock_irqsave(&gip->message_lock, flags);
+	gip_deinit_input(&gip->data);
+	gip_deinit_output(&gip->data);

[Severity: High]
Should the active OUT URBs be killed before de-initializing output?

Only the IN URBs are killed via usb_kill_urb() prior to cleanup.
gip_deinit_output() frees the URBs and their DMA buffers via
usb_free_coherent(). If the host controller is actively transmitting
these buffers, it will cause DMA corruption and crash the USB core.

[Severity: High]
Is it safe to call usb_free_coherent() while holding a spinlock?

gip_deinit_input() and gip_deinit_output() invoke usb_free_coherent()
to free DMA buffers. This function can involve TLB flushes and IOMMU
operations that may sleep. Calling it while holding gip->message_lock
and with interrupts disabled violates atomic context rules.

[ ... ]

+	for (i = 0; i < MAX_ATTACHMENTS; i++) {
+		struct gip_attachment *attachment = gip->attachments[i];
+
+		if (!attachment)
+			continue;
+
+		gip_reset_attachment(attachment);
+
+		devm_kfree(to_gip_device(attachment), attachment->uniq);
+		devm_kfree(to_gip_device(attachment), attachment);
+	}
+
+	devm_kfree(to_gip_device(gip), gip);
+}

[Severity: High]
Can explicit freeing of the attachment cause a Use-After-Free?

The attachment memory is explicitly freed here via devm_kfree(). However,
the LED and Battery devices were registered with &input->dev as their
devres parent. If userspace holds the input device open, devres cleanup
is delayed. A subsequent sysfs modification to the LED state will invoke
gip_guide_led_set(), which dereferences the freed attachment structure.

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