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

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [Critical] Loop index typo in `gip_disconnect` causes OOB array access and failure to cancel delayed work.
- [High] Retrying reliable (ACME) messages corrupts their headers due to incorrect flag and length field encoding.
- [High] Teardown sequence race condition causing Use-After-Free of device structures due to URBs re-scheduling cancelled works.

Pre-existing issues:
- [High] AB-BA deadlock during device reset due to unregistering input device under lock.
- [High] Teardown ordering race condition causing UAF of rumble_work and attachment memory.
--

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

This patch adds support for sending fragmented and ACME (reliable) messages
in the xbox_gip driver. It sits on top of a reliable message transmission
flow for messages that exceed the MTU and handles the required sequence and
acknowledgment tracking.

[ ... ]

[Severity: High]
Does this unconditionally force GIP_FLAG_FRAGMENT onto all retries? If an 
unfragmented reliable message times out, retrying it with this flag will
cause gip_send_raw_message() to process it as a fragment. When processed as a
fragment, it encodes the fragment_offset (which is 0) instead of the
total_length, resulting in a corrupted header that the hardware may reject.

+	int mtu;
+	int rc;

[ ... ]

[ ... ]

 	for (i = 0; i < MAX_ATTACHMENTS; i++) {
 		struct gip_attachment *attachment;
 
 		mutex_lock(&gip->attachment_lock);
 		attachment = gip->attachments[i];
 		mutex_unlock(&gip->attachment_lock);
 
 		if (!attachment)
 			continue;
 
 		cancel_delayed_work_sync(&attachment->metadata_next);
 		cancel_delayed_work_sync(&attachment->in_fragment_timeout);
 		cancel_work_sync(&attachment->rumble_work);
 
 		guard(mutex)(&attachment->lock);
 		gip_reset_attachment(attachment);

[Severity: High]
This is a pre-existing issue, but does this teardown sequence leave a race 
window open for rumble_work to be rescheduled? The rumble_work is cancelled
before gip_reset_attachment() unregisters the input device. A concurrent 
ioctl or ff-memless timer can call gip_play_effect() and reschedule the work 
while the device is still registered, leading to a Use-After-Free when the 
attachment memory is freed.

[Severity: High]
This isn't a bug introduced by this patch, but is there a risk of an AB-BA
deadlock here during device reset?

When a hardware reconnect triggers gip_receive_message() and eventually
gip_reset_attachment(), it acquires attachment->lock and calls
input_unregister_device() via gip_free_devices(). The unregistration process
synchronously removes sysfs groups, which blocks waiting for all active
kernfs references to drain.

If a local user concurrently writes to the LED sysfs brightness attribute,
gip_guide_led_set() acquires an active kernfs reference and then blocks
attempting to acquire attachment->lock, completing the deadlock.

[Severity: High]
Is this the correct teardown sequence? If gip_shutdown() cancels the
send_fragment work before usb_kill_anchored_urbs() is called, active URBs
might complete in the window between them. If an URB completes and triggers
gip_urb_out(), it will reschedule the send_fragment work. After
gip_disconnect() frees the device structures, the rescheduled work could
execute and access freed memory. Should usb_kill_anchored_urbs() be called 
before gip_shutdown()?

[ ... ]

[Severity: Critical]
Does this inner loop mistakenly use the outer index i instead of j?
Iterating up to MAX_OUT_FRAGMENTS using the outer attachment index i could
lead to an out-of-bounds access when i is larger than the fragment array 
bounds. Additionally, it fails to cancel the actual delayed work items at 
indexes j, which could lead to a Use-After-Free if they execute after the 
attachment is freed.

 		devm_kfree(to_gip_device(attachment), attachment->uniq);
 		devm_kfree(to_gip_device(attachment), attachment);

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