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

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

New issues:
- [High] Mismatched raw value offset and declared axis bounds for the ABS_WHEEL axis.
- [High] Brittle wraparound logic for the TrueForce wheel hardware rotary dial causes erratic backward spikes.
- [Medium] Packet length validation occurs after partially processing events, causing dropped input_sync() calls and stuck events.

Pre-existing issues:
- [High] Dangling pointer left in attachment->input when input_register_device() fails, leading to Use-After-Free on subsequent input reports.
- [High] AB-BA Deadlock between gip_receive_fragment and gip_fragment_timeout workqueue handler.
--

--- Patch [12]: [PATCH v4 12/12] Input: xbox_gip - Add wheel support ---
commit fd5878269a3ac89add6a8a8b7336791b58d87703
Author: Vicki Pfau <[email protected]>

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.

Signed-off-by: Vicki Pfau <[email protected]>

> diff --git a/drivers/input/joystick/gip/gip-wheel.c b/drivers/input/joystick/gip/gip-wheel.c
> new file mode 100644
> index 0000000000000..333910448c901
> --- /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]
If the packet length is less than 17,
gip_driver_navigation.handle_input_report() has already processed the first
2 bytes and pushed button states to the input core buffer.

Returning -EINVAL here causes the caller to exit without calling
input_sync(). Will this leave the parsed button events stuck in the input
buffer, leading to desynchronized state in userspace?

Could we validate the total packet length before calling the base handler to
modify device state?

> +
> +	input_report_abs(input, ABS_WHEEL, get_unaligned_le16(&bytes[2]) - 0x8000);

[Severity: High]
In gip_handle_wheel_ll_static_configuration(), max_angle is calculated as
65535 for 16-bit precision, setting the ABS_WHEEL bounds to [-65536, 65535].

Here, the raw 16-bit wheel value (0-65535) is shifted by a hardcoded 0x8000
(32768), resulting in physical values ranging strictly from -32768 to 32767.
Does this restrict the reported values to only cover 50% of the declared
logical bounding box?

Additionally, if the device reports a precision other than 16 bits, will
this hardcoded 0x8000 shift push the values completely outside the bounding
box?

[ ... ]

> +static int gip_handle_trueforce_wheel_report(struct gip_attachment *attachment,
> +	struct input_dev *input, const uint8_t *bytes, int num_bytes)
> +{

[ ... ]

> +	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: High]
If the user spins the hardware dial rapidly, the 3-bit value might jump by
more than one step per report (e.g., from 7 to 1).

Will this bypass the single-step wrap checks and fall back to
state->dial - dial (7 - 1 = 6), erroneously reporting a massive 6-step
backward jump instead of the actual 2-step forward movement?

Can this be updated to use modular arithmetic (like sign-extending the
3-bit difference) instead of checking for exact neighbor boundaries?

[ ... ]

> diff --git a/drivers/input/joystick/gip/gip-core.c b/drivers/input/joystick/gip/gip-core.c
> --- a/drivers/input/joystick/gip/gip-core.c
> +++ b/drivers/input/joystick/gip/gip-core.c

[ ... ]

>  int gip_setup_input_device(struct gip_attachment *attachment)
>  {
>  	struct input_dev *input;

[ ... ]

>  	input_set_drvdata(input, attachment);
>  	rcu_assign_pointer(attachment->input, input);
>  	rc = input_register_device(input);
>  	if (rc)
>  		goto err_free_device;

[ ... ]

>  err_free_device:
>  	input_free_device(input);
>  	return rc;
>  }

[Severity: High]
This is a pre-existing issue, but in gip_setup_input_device(), the allocated
input_dev is assigned to attachment->input via rcu_assign_pointer().

If input_register_device() fails, we jump to err_free_device and call
input_free_device(input). Does this leave a dangling pointer in
attachment->input that could cause a use-after-free when a subsequent
input report is processed?

[ ... ]

>  static void gip_receive_fragment(struct gip_attachment *attachment,
>  	const uint8_t *bytes, int num_bytes)
>  {

[ ... ]

> 		cancel_delayed_work_sync(&attachment->in_fragment_timeout);

[Severity: High]
This is a pre-existing issue, but I noticed a potential deadlock between
gip_receive_fragment() and gip_fragment_timeout().

gip_receive_message() acquires attachment->lock before calling
gip_receive_fragment(), which then calls
cancel_delayed_work_sync(&attachment->in_fragment_timeout).

However, the delayed work handler, gip_fragment_timeout(), immediately
attempts to acquire attachment->lock:

static void gip_fragment_timeout(struct work_struct *work)
{
        ...
        guard(mutex)(&attachment->lock);
        ...
}

If the delayed work has already started executing on another CPU, will
cancel_delayed_work_sync() block forever waiting for the work to finish,
while the work handler blocks forever waiting for attachment->lock to
be released?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12