Re: [PATCH v6 06/12] Input: xbox_gip - Allow sending fragmented and ACME messages

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Missing synchronous cancellation of `out_fragments[i].timeout` during device shutdown leads to use-after-free.
- [High] Unprotected lockless access to `out_fragments` state causes data races, corruption, and potential Use-After-Free.
- [High] Infinite USB transmission loop if the first fragment of a message times out.
- [High] The `acked` field of `gip_out_fragment` is not cleared on slot reuse, breaking reliable retries.
- [Medium] Stale `has_pending_out` bit in error path causes concurrent chunk transmissions.
- [Low] Mixing `goto` error handling with `guard()` cleanup violates subsystem guidelines.
--

commit a080933dee9387ce07bf54626ee39a087371ba35
Author: Vicki Pfau [off-list ref]
Subject: Input: xbox_gip - Allow sending fragmented and ACME messages

This patch implements support for sending fragmented messages and handling the
reliable transmission flow (ACME messages). It adds dynamic message allocation
slots and uses background delayed work to manage ACKs and retry timeouts.

Link: https://lkml.kernel.org/r/target-patch-link

[ ... ]

+static void gip_free_fragment(struct gip_out_fragment *fragment)
+{
+	cancel_delayed_work(&fragment->timeout);
+	kfree(fragment->data);
+	fragment->data = NULL;
+	fragment->active = false;
+	fragment->message = 0;
+	fragment->flags = 0;
+	fragment->seq = 0;
+	fragment->retries = 0;
+	fragment->total_length = 0;
+	fragment->fragment_offset = 0;
+}

[Severity: High]
Does this code leak the acked state to future messages?

When fragment slots are released, fragment->acked is not cleared here. If a
slot is later reused, it could inherit a stale true value, causing the
timeout retry logic to immediately skip resending lost packets.

[ ... ]

+		for (i = 0; i < MAX_OUT_FRAGMENTS; i++) {
+			if (attachment->out_fragments[i].active)
+				continue;
+
+			fragment = &attachment->out_fragments[i];
+			break;
+		}

[Severity: High]
Is it safe to access out_fragments locklessly here?

gip_handle_command_protocol_control() also modifies these fields locklessly.
Scanning and claiming slots concurrently without acquiring attachment->lock
can lead to data races and duplicate slot allocations.

+
+		if (!fragment) {
+			gip_err(attachment, "Fragmented message queue is full; dropping message\n");
+			return -EALREADY;
+		}
+
+		fragment->data = kmalloc(num_bytes, GFP_KERNEL);
+		if (!fragment->data)
+			return -ENOMEM;
+		fragment->active = true;
+		fragment->message = message_type;
+		fragment->flags = flags & GIP_FLAG_SYSTEM;
+		fragment->seq = seq;
+		fragment->total_length = num_bytes;
+		fragment->fragment_offset = 0;

[Severity: High]
Should fragment->acked be explicitly set to false here?

Since gip_free_fragment() does not clear the acked flag, explicitly setting
it to false here would ensure the new message does not skip retry handling
if the previous message finished successfully.

[ ... ]

 	guard(spinlock_irqsave)(&attachment->device->message_lock);
 	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 = -ENOSPC;
+		goto err_free_fragment;
 	}

[Severity: Low]
Is it permitted to mix goto error handling with guard() in this function?

The cleanup subsystem guidelines strictly mandate that functions must not mix
goto-based cleanup labels with guard() declarations, as it causes confusing
ownership semantics and resource leaks.

[ ... ]

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

[Severity: Medium]
If usb_submit_urb() fails and we jump to err_free_fragment, does this leave a
stale bit in intf->has_pending_out?

If the has_pending_out bit was previously set from an intermediate chunk
submission, failing to clear it here leaves it active. When a new message
claims this slot, gip_send_fragment_work() could see the stale bit and
prematurely send the new message's second chunk.

[ ... ]

+static int gip_send_next_fragment(struct gip_attachment *attachment,
+	struct gip_out_fragment *fragment)
+{

[ ... ]

+	} else if (fragment->fragment_offset + mtu >= fragment->total_length) {
+		next_bytes = fragment->total_length - fragment->fragment_offset;
+		flags |= GIP_FLAG_ACME;
+	} else {
+		next_bytes = mtu;
+	}

[Severity: High]
How does this sequence handle retries for the first chunk?

If the first chunk of a fragmented message times out and this function
retries it, GIP_FLAG_ACME is not appended because it is not the final
fragment. This bypasses the reliable ACME timeout logic, which causes
the worker thread to immediately and repeatedly resend offset 0 without
waiting for ACKs, resulting in an infinite USB transmission loop.

[ ... ]

 static void gip_reset_attachment(struct gip_attachment *attachment)
 {
+	int i;
+	int pending_out_bits =
+		BIT((attachment->attachment_index + 1) * MAX_OUT_FRAGMENTS) -
+		BIT(attachment->attachment_index * MAX_OUT_FRAGMENTS);
+	unsigned long flags;
+
 	cancel_delayed_work(&attachment->metadata_next);
 	cancel_delayed_work(&attachment->in_fragment_timeout);
 
 	if (attachment->in_fragment_data)
 		gip_free_in_fragment(attachment);
 
+	spin_lock_irqsave(&attachment->device->message_lock, flags);
+	attachment->device->data.has_pending_out &= ~pending_out_bits;
+	attachment->device->audio.has_pending_out &= ~pending_out_bits;
+	spin_unlock_irqrestore(&attachment->device->message_lock, flags);
+
+	for (i = 0; i < MAX_OUT_FRAGMENTS; i++) {
+		if (attachment->out_fragments[i].active)
+			gip_free_fragment(&attachment->out_fragments[i]);
+	}
+
 	gip_free_devices(attachment);
+
 }

[Severity: High]
Could the loop in gip_reset_attachment() introduce a use-after-free during
teardown?

During gip_shutdown(), the device teardown path correctly uses
cancel_delayed_work_sync() for other background workers, but relies on this
loop which uses the non-synchronous cancel_delayed_work() via
gip_free_fragment().

If gip_out_fragment_timeout() is executing and waiting on attachment->lock
during shutdown, it could resume after devres frees the attachment
structure, resulting in a use-after-free.

[ ... ]

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

[ ... ]

+	fragment->acked = true;
+	fragment->retries = 0;
+	cancel_delayed_work(&fragment->timeout);
+	if (fragment->flags & GIP_FLAG_FRAGMENT) {
+		fragment->fragment_offset = fragment_offset;
+		rc = gip_send_next_fragment(attachment, fragment);
+		if (rc < 0)
+			return rc;
+	} else {
+		gip_free_fragment(fragment);
+	}
+
+	return 0;

[Severity: High]
Are modifications to the fragment state thread-safe here without
attachment->lock?

This path modifies fragment->acked and fragment->fragment_offset, and calls
gip_free_fragment() entirely locklessly. Doing so can race concurrently
with gip_out_fragment_timeout() and gip_send_raw_message(), leading to
corrupted fragment offsets or duplicated USB submissions.

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