Re: [PATCH v7 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 length checks in input report handlers leave events unsynchronized on truncated packets.
- [Medium] Incorrect modulo arithmetic for relative dial causes erratic reverse jumps.
- [Medium] Static configuration of `max_angle` with 0 precision causes integer underflow, bypassing validation.

Pre-existing issues:
- [High] AB-BA deadlock between LED sysfs operations and device unregistration.
--

commit 533e8fbcd84078cc304b29f07232970fb0a6e7d7
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-core.c b/drivers/input/joystick/gip/gip-core.c
> index 720b9a075ed3a..42e191f0aef24 100644
> --- a/drivers/input/joystick/gip/gip-core.c
> +++ b/drivers/input/joystick/gip/gip-core.c
[ ... ]
> @@ -334,6 +334,8 @@ static const struct gip_driver *base_drivers[] = {
>  	&gip_driver_gamepad,
>  	&gip_driver_arcade_stick,
>  	&gip_driver_pdp_jaguar,
> +	&gip_driver_trueforce_wheel,
> +	&gip_driver_wheel,

[Severity: High]
This is a pre-existing issue, but does adding these devices expose them to
the AB-BA deadlock between LED sysfs operations and device unregistration?

During device disconnect, gip_reset_attachment() acquires attachment->lock and
calls gip_free_devices(), which manually calls input_unregister_device(). If
there are no open evdev handles, this synchronously tears down the device and
its devres resources, invoking led_classdev_unregister(). This function blocks
in kernfs_drain waiting for active sysfs operations to complete. Concurrently,
a sysfs write to the LED brightness holds the kernfs active reference and calls
gip_guide_led_set(), which blocks trying to acquire attachment->lock.

> diff --git a/drivers/input/joystick/gip/gip-wheel.c b/drivers/input/joystick/gip/gip-wheel.c
> new file mode 100644
> index 0000000000000..bd281b295eac1
> --- /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 this length check leave partial events unsynchronized if the packet is
truncated?

If num_bytes is less than 17, this returns -EINVAL after
gip_driver_navigation.handle_input_report() has already reported events via
input_report_key(). The outer function will then skip input_sync(), leaving the
reported events stuck in the input core's buffers until the next sync.

[ ... ]
> +static int gip_handle_wheel_ll_static_configuration(struct gip_attachment *attachment,
> +	const struct gip_header *header, const uint8_t *bytes, int num_bytes)
> +{
> +	struct gip_wheel_info *info = attachment->driver_data;
> +
> +	if (!info)
> +		return -ENODEV;
> +
> +	if (num_bytes < 11)
> +		return -EINVAL;
> +
> +	info->max_angle = BIT(min(bytes[0], 16)) / 2 - 1;

[Severity: Medium]
Could this formula cause an integer underflow if bytes[0] is 0?

If a 0-bit precision configuration is received, BIT(0) is 1, and 1 / 2 - 1
evaluates to -1, which underflows the uint16_t info->max_angle to 65535. This
could bypass validation checks and assign corrupted bounds to ABS_WHEEL.

[ ... ]
> +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]
Will this length check leave partial events unsynchronized if the packet is
truncated?

Similar to the check in gip_handle_wheel_report(), this returns -EINVAL after
the base wheel handler has already injected state changes, causing the core loop
to abort before calling input_sync().

[ ... ]
> +	/* 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 if (state->dial >= 0)
> +		input_report_rel(input, REL_DIAL,
> +			state->dial - dial);

[Severity: Medium]
Could this cause erratic relative dial jumps if the dial skips a step?

If the dial moves rapidly across the 0-7 boundary and skips a step (e.g., from
previous state 6 to current dial 0), the hardcoded wraparound checks fail. It
then falls back to state->dial - dial (6 - 0 = 6), reporting +6 instead of the
actual shortest distance of -2.

Should standard modulo arithmetic be used here instead?

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