Re: [PATCH v5 12/12] Input: xbox_gip - Add wheel support

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

New issues:
- [Medium] Improper wrap-around calculation for REL_DIAL in TrueForce wheel driver.
- [Medium] Packet length validation happens after partial event reporting, causing input event desync on short packets.

Pre-existing issues:
- [Critical] Workqueues are synchronously cancelled before killing URBs in gip_disconnect, causing a Use-After-Free.
--

commit f290772c9ca66efd1e7d5ec4dd73e7af098deefd
Author: Vicki Pfau [off-list ref]

Input: xbox_gip - Add wheel support

This adds preliminary support for racing wheel support in xbox_gip,
exposing them mapped to the newly added axes.

[ ... ]

+static int gip_handle_wheel_report(struct gip_attachment *attachment,
+	struct input_dev *input, const uint8_t *bytes, int num_bytes)
+{
+	uint8_t connections;
+	struct gip_wheel_info *info = attachment->driver_data;
+	int rc = gip_driver_navigation.handle_input_report(attachment, input, bytes, num_bytes);
+
+	if (rc < 0)
+		return rc;
+
+	if (!info)
+		return -ENODEV;
+
+	if (num_bytes < 17)
+		return -EINVAL;

[Severity: Medium]
Does this cause input events to be desynced on short packets?

If a short packet arrives (between 2 and 16 bytes), the base navigation handler
above successfully processes the first bytes and queues events. The length
check here then fails and returns -EINVAL, which causes an early exit before
the core can call input_sync().

This could leave the device in an inconsistent state with phantom inputs or
stuck keys until the next valid packet arrives.

[ ... ]

+static int gip_handle_trueforce_wheel_report(struct gip_attachment *attachment,
+	struct input_dev *input, const uint8_t *bytes, int num_bytes)
+{
+	int rc = gip_driver_wheel.handle_input_report(attachment, input, bytes, num_bytes);
+	struct gip_trueforce_wheel_state *state = attachment->driver_data;
+	int dial;
+
+	if (rc < 0)
+		return rc;
+
+	if (num_bytes < 18)
+		return -EINVAL;

[Severity: Medium]
Similarly, does this early return skip input synchronization for short packets?

Like the base wheel handler, events are queued by
gip_driver_wheel.handle_input_report() before this check fails.

[ ... ]

+	if (dial == 0 && state->dial == 7)
+		input_report_rel(input, REL_DIAL, -1);
+	else if (dial == 7 && state->dial == 0)
+		input_report_rel(input, REL_DIAL, 1);
+	else
+		input_report_rel(input, REL_DIAL,
+			state->dial - dial);
+	state->dial = dial;
+
+	return 0;
+}

[Severity: Medium]
Can this calculation magnify offsets if the dial is turned quickly?

If the wheel's dial is turned fast enough to jump multiple steps between USB
reports (e.g., from 6 to 0, crossing the boundary), this falls through the
1-step boundary checks.

The fallback calculation of state->dial - dial would then result in a
reported offset of 6 (6 - 0) instead of the actual -2 steps taken.

As a quick aside, I noticed a separate issue in the core code while reviewing:

[Severity: Critical]
This is a pre-existing issue, but does gip_disconnect() have a Use-After-Free
race window during device unbind?

drivers/input/joystick/gip/gip-core.c:gip_disconnect() {
    ...
    gip_shutdown(gip);
    usb_kill_urb(gip->data.urb_in);
    ...
}

Since workqueues are synchronously cancelled in gip_shutdown() before calling
usb_kill_urb(), if an URB completes in that window, its completion handler
can reschedule the work on memory that is actively being torn down.

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