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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Late packet length validation causes input state desynchronization.
- [Medium] Incorrect wraparound delta calculation for the REL_DIAL axis when multiple steps occur.
- [Medium] Phantom REL_DIAL scroll event upon device initialization.

Pre-existing issues:
- [High] Out-of-order teardown in `gip_disconnect` allows URB completion handler to trigger use-after-free of gip_device.
--

commit 4a4f1a60a6109caec93af3211950c865f6e16e58
Author: Vicki Pfau <vi@endrift.com>

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.

> diff --git a/drivers/input/joystick/gip/gip-wheel.c b/drivers/input/joystick/gip/gip-wheel.c
> new file mode 100644
> index 0000000000000..5de455edff3c1
> --- /dev/null
> +++ b/drivers/input/joystick/gip/gip-wheel.c
[ ... ]
> +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]
Will returning -EINVAL here trap events that were already queued?

If a device sends a fragmented or short report (e.g. 16 bytes),
gip_driver_navigation.handle_input_report() will process the first bytes and
queue events.

By returning -EINVAL after this, the error propagates and bypasses the
input_sync() call in the core loop, leaving the queued events
unsynchronized in the input subsystem buffer and potentially causing
stuck keys.

[ ... ]
> +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]
Can this similarly trap events reported by the base wheel handler?

Events are reported by gip_driver_wheel.handle_input_report() before the
TrueForce-specific size validation rejects the packet. This will also bypass
input_sync().

[ ... ]
> +	input_report_key(input, KEY_KPENTER, bytes[17] & BIT(4));
> +	/* Dial states only change one at a time, so we can easily check for wraparound */
> +	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);

[Severity: Medium]
Is it possible for this calculation to compute an incorrect wraparound delta
if multiple steps occur between reports?

If a user quickly scrolls the TrueForce dial, the step might jump by 2 or more
across the 0/7 boundary (e.g. from 7 to 1).

The explicit edge case checks would fail, and the fallback logic would compute
state->dial - dial (7 - 1 = 6), incorrectly reporting a delta of +6 instead of
the correct logical step of -2.

[Severity: Medium]
Could this logic emit a phantom REL_DIAL scroll event when the device is
initialized?

When the driver first allocates gip_trueforce_wheel_state, kzalloc zeroes
state->dial.

If the physical wheel dial is at a non-zero position (e.g. 4) when normal
reporting begins, the handler computes 0 - 4 = -4 and immediately emits a
scroll event before the user even touches the wheel.

[ ... ]

This is a pre-existing issue, but I noticed a potential problem in
gip_disconnect() within drivers/input/joystick/gip/gip-core.c:

[Severity: High]
Does cancelling the work before killing the URB allow the URB completion
handler to schedule new work that outlives the device?

drivers/input/joystick/gip/gip-core.c:gip_disconnect() {
	...
	gip_shutdown(gip);

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

If the URB is still active and a packet arrives just after gip_shutdown()
cancels the device->receive_message work struct, the URB completion handler
can call schedule_work() again.

When gip_disconnect() completes and frees the gip structure, the system
workqueue could execute the corrupted workqueue item and trigger a
use-after-free kernel panic.

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