Re: [PATCH v5 04/12] Input: xbox_gip - Add HID relaying

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

New issues:
- [Critical] gip_reset_metadata() frees hid_descriptor before destroying the HID device, allowing a concurrent UAF during HID probe.
- [High] Fragmented HID reports are processed with a payload size of 0, causing the entire assembled report to be truncated and silently dropped.
- [Medium] Useless synchronize_rcu() call delays device initialization.
- [Medium] The hid_device fields `version`, `country`, and `uniq` are not initialized before calling `hid_add_device()`.

Pre-existing issues:
- [Critical] Use-after-free in receive worker due to incorrect shutdown ordering.
--

commit e6352eb8b59489fd2119b2f3d053cfef65ac2c92
Author: Vicki Pfau [off-list ref]
Subject: Input: xbox_gip - Add HID relaying

This patch allows tunneling of HID packets by creating a hid_device for
the HID descriptor embedded in the GIP metadata, and relays the packets.

[Severity: Critical]
Can this lead to a use-after-free during a concurrent HID probe?

If a device reset or hotplug occurs, gip_reset_metadata() frees the
metadata before destroying the child devices:

gip_reset_metadata()
    gip_metadata_free(to_gip_device(attachment), &attachment->metadata);
    gip_free_devices(attachment);

Since gip_metadata_free() synchronously frees hid_descriptor, if the HID
core concurrently probes the device and calls gip_hid_ll_parse() before
gip_free_devices() unregisters the HID device, it would dereference the
freed descriptor memory.

[ ... ]

[Severity: Medium]
Are we missing the initialization of some standard hid_device fields here?

It appears that the version, country, and uniq fields are not populated
before calling hid_add_device(). This might break userspace expectations
that rely on these standard HID properties for device matching or
configuration.

+	if (rc) {
+		gip_err(attachment, "HID device add failed: %d\n", rc);
+		hid_destroy_device(hdev);
+	} else {
+		rcu_assign_pointer(attachment->hdev, hdev);
+		synchronize_rcu();
+	}

[Severity: Medium]
Is this synchronize_rcu() call necessary on the successful allocation path?

Since we are publishing a new pointer where the previous value was NULL,
there are no existing readers holding an old pointer that needs to be
reclaimed. This seems to delay device initialization by a full RCU grace
period unnecessarily.

[ ... ]

[Severity: High]
Could this cause large fragmented HID reports to be silently dropped?

When gip_receive_fragment() reassembles a fragmented message, it completes
upon receiving a terminator fragment where header->length is 0. It then
calls gip_handle_message() using this final header. 

If umin(header->length, num_bytes) is evaluated with header->length == 0,
the payload size passed to hid_safe_input_report() will be 0, causing the
entire reassembled HID report to be truncated.

+
+	gip_warn(attachment, "Got HID report with no HID descriptor\n");
+
+	return -EPROTO;
 }

[ ... ]

[Severity: Critical]
This is a pre-existing issue, but is there a use-after-free risk in the
receive worker due to incorrect shutdown ordering in gip_disconnect()?

Looking at gip_disconnect():

	gip_shutdown(gip);
	...
	usb_kill_urb(gip->data.urb_in);

The gip_shutdown() function synchronously cancels the receive worker.
However, this happens before usb_kill_urb() is called. If an incoming URB
completes right after cancel_work_sync() returns, gip_urb_in() could
re-queue the worker.

When gip_disconnect() then proceeds to free the gip structure, the
re-queued worker will execute gip_receive_work() and access the freed
memory. Should the URBs be killed before cancelling the workers?

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