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 <[email protected]>
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.
> diff --git a/drivers/input/joystick/gip/gip-core.c b/drivers/input/joystick/gip/gip-core.c
> --- /dev/null
> +++ b/drivers/input/joystick/gip/gip-core.c
> @@ -0,0 +1,2569 @@
> +static int gip_decode_length(uint64_t *length, const uint8_t *bytes, int num_bytes)
> +{
> + *length = 0;
> + int offset;
> +
> + for (offset = 0; offset < num_bytes; offset++) {
> + uint8_t byte = bytes[offset];
> +
> + *length |= (byte & 0x7full) << (offset * 7);
[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/[email protected]?part=1
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.