Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
New issues:
- [Critical] Missing cancellation of `send_fragment` work and OUT URBs causes Use-After-Free during driver teardown.
- [High] Missing NULL check for `devm_kmalloc` allocation in `gip_send_raw_message`.
- [High] Unsynchronized allocation of `out_fragments` slots in `gip_send_raw_message` leads to a TOCTOU race condition.
- [High] Fragment slots and memory are permanently leaked on output queue full or URB submission failures.
- [High] Unprotected read-modify-write of the `intf->has_pending_out` bitmask causes data races.
- [High] Broken search logic in `gip_find_fragment` completely ignores exact sequence numbers when `strict` is true.
- [High] Fragment slots for ACME messages permanently leak if the device drops the packet.
- [Medium] Improper use of managed resources (`devm_kmalloc`) for high-frequency, dynamic per-message buffers.
Pre-existing issues:
- [High] AB-BA deadlock in `gip_shutdown` when canceling delayed works.
- [High] gip_receive_fragment() blindly trusts header->length when processing subsequent fragments, leading to an out-of-bounds read from the kernel heap.
- [High] gip_receive_fragment() can dereference a NULL pointer if devm_kmalloc fails and the fragmented message sequence continues.
--
--- Patch [6]: [PATCH v4 06/12] Input: xbox_gip - Allow sending fragmented and ACME messages ---
commit 945e1fae94cb61e149c842cf10c67d06095eaf41
Author: Vicki Pfau <[email protected]>
Subject: Input: xbox_gip - Allow sending fragmented and ACME messages
This patch adds support for sending messages that exceed the length of the MTU
using a fragmented message flow. It also supports sending messages requiring
ACME acknowledgments, which will be necessary for future security flows.
> diff --git a/drivers/input/joystick/gip/gip-core.c b/drivers/input/joystick/gip/gip-core.c
> index d694ac5516395..a75fbcb1c02dc 100644
> --- a/drivers/input/joystick/gip/gip-core.c
> +++ b/drivers/input/joystick/gip/gip-core.c
[ ... ]
> @@ -479,6 +486,48 @@ static void gip_handle_quirks(struct gip_attachment *attachment)
> +static struct gip_out_fragment *gip_find_fragment(struct gip_attachment *attachment,
> + uint8_t message_type, uint8_t flags, uint8_t seq, bool strict)
> +{
> + int i;
> +
> + for (i = 0; i < MAX_OUT_FRAGMENTS; i++) {
[ ... ]
> + }
> +
> + return &attachment->out_fragments[i];
> + }
> +
> + for (i = 0; i < MAX_OUT_FRAGMENTS; i++) {
> + if (!attachment->out_fragments[i].active)
> + continue;
> + if (attachment->out_fragments[i].message != message_type)
> + continue;
> + if ((attachment->out_fragments[i].flags ^ flags) & GIP_FLAG_SYSTEM)
> + continue;
> +
> +
> + return &attachment->out_fragments[i];
> + }
[Severity: High]
Does this second loop bypass the sequence number check entirely when strict is
true? If a strict search fails in the first loop, falling through to this one
might unconditionally return a fragment that matches the type but has the wrong
sequence number, potentially misrouting packets.
[ ... ]
> @@ -486,20 +535,25 @@ static int gip_send_raw_message(struct gip_attachment *attachment,
[ ... ]
> + if (num_bytes > reduced_mtu ||
> + (flags & (GIP_FLAG_ACME | GIP_FLAG_FRAGMENT)) == GIP_FLAG_ACME) {
> + for (i = 0; i < MAX_OUT_FRAGMENTS; i++) {
[Severity: High]
Are fragment slots permanently consumed if an ACME message is sent but the
device drops the packet and never ACKs it? Since there doesn't appear to be a
timeout or cleanup mechanism for pending fragments, a couple of dropped packets
might permanently exhaust the queue.
> + if (attachment->out_fragments[i].active)
> + continue;
> +
> + fragment = &attachment->out_fragments[i];
> + break;
> + }
> +
> + if (!fragment) {
> + gip_err(attachment, "Fragmented message queue is full; dropping message\n");
> + return -EALREADY;
> + }
> +
> + fragment->active = true;
[Severity: High]
Can multiple threads race to claim the same fragment slot here? This loop and
the subsequent assignment to fragment->active appear to occur without holding
attachment->lock or device->message_lock.
> + fragment->message = message_type;
[ ... ]
> + 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]
Could this memcpy dereference a NULL pointer if the devm_kmalloc allocation
fails? The GFP_ATOMIC flag means the allocation can fail under memory
pressure, but there is no check before accessing the returned pointer.
[Severity: Medium]
Is devm_kmalloc the right choice for dynamic, per-message buffers? Since
devm_kfree requires a linear scan of the device's devres linked list under a
spinlock, frequent allocations and frees here might cause noticeable overhead
and list fragmentation. Would standard kmalloc and kfree be better suited?
[ ... ]
> @@ -522,15 +659,10 @@ static int gip_send_raw_message(struct gip_attachment *attachment,
[ ... ]
> for (i = 0; i < MAX_OUT_MESSAGES && !urb; i++) {
> if (!intf->out_queue[i].urb)
> continue;
> if (!intf->out_queue[i].urb->anchor)
> urb = &intf->out_queue[i];
> }
> if (!urb) {
> gip_err(attachment, "Output queue is full; dropping message\n");
> return -ENOSPC;
> }
[ ... ]
> + rc = usb_submit_urb(urb->urb, GFP_ATOMIC);
> + if (rc) {
> + dev_err(&intf->intf->dev,
> + "%s - usb_submit_urb failed with result %d\n",
> + __func__, rc);
> + usb_unanchor_urb(urb->urb);
> + rc = -EIO;
> + }
[Severity: High]
If the function returns early due to a full output queue (-ENOSPC) or a failure
in usb_submit_urb() (-EIO), does the fragment slot remain permanently active?
It looks like the allocated memory and the active slot aren't freed on these
error paths.
[ ... ]
> +static int gip_send_next_fragment(struct gip_attachment *attachment,
> + struct gip_out_fragment *fragment)
> +{
[ ... ]
> + if (fragment->fragment_offset == fragment->total_length) {
> + if ((fragment->message & GIP_DATA_CLASS_MASK) == GIP_DATA_CLASS_AUDIO)
> + intf = &attachment->device->audio;
> + else
> + intf = &attachment->device->data;
> +
> + next_bytes = 0;
> + intf->has_pending_out &= ~gip_fragment_id(attachment, fragment);
[Severity: High]
Is it safe to perform an unprotected read-modify-write on intf->has_pending_out
here? In gip_send_raw_message(), this same bitmask is modified while holding
the message_lock spinlock. Concurrent modifications could lead to a data race
and cause the driver to lose track of pending output.
[ ... ]
> @@ -2476,6 +2771,7 @@ static void gip_urb_out(struct urb *urb)
> switch (status) {
> case 0:
> /* success */
> + schedule_work(&intf->send_fragment);
> break;
[Severity: High]
Could this result in a use-after-free during driver teardown?
If gip_disconnect() frees the out_queue URBs using usb_free_urb() instead of
usb_kill_urb(), in-flight URBs might complete after the device structures are
freed. The completion handler here would then schedule the send_fragment work
using a freed intf context.
[ ... ]
A few additional observations in the surrounding codebase:
> static void gip_shutdown(struct gip_device *gip)
> {
[ ... ]
> scoped_guard (mutex, &attachment->lock) {
> cancel_delayed_work_sync(&attachment->metadata_next);
> cancel_delayed_work_sync(&attachment->in_fragment_timeout);
[Severity: High]
This isn't a bug introduced by this patch, but could this cause an AB-BA
deadlock? gip_shutdown() acquires attachment->lock before waiting synchronously
on these works, but the callbacks for both works (gip_retry_metadata() and
gip_fragment_timeout()) begin by acquiring the same attachment->lock.
> static int gip_receive_fragment(...)
> {
[ ... ]
> if (header->length != 0)
> memcpy(&attachment->in_fragment_data[*fragment_offset],
> &bytes[offset], header->length);
[Severity: High]
This isn't a bug introduced by this patch, but is it safe to blindly trust
header->length here? There doesn't appear to be a check ensuring that
offset + header->length <= num_bytes (the actual size of the received USB
packet). A malicious device could send a short packet with a large length field,
potentially causing an out-of-bounds read from the kernel heap.
> static int gip_receive_fragment(...)
> {
[ ... ]
> attachment->in_fragment_message = header->message_type;
> ...
> 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]
This isn't a bug introduced by this patch, but does an allocation failure leave
the state inconsistent? By updating in_fragment_message before a failing
devm_kmalloc(), a subsequent fragment from the device might pass the message
type check and blindly memcpy into a NULL in_fragment_data pointer.
--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
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.