Re: [PATCH RFC 2/4] HID: logitech: handle HID++ thumbwheel reports

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] hid: logitech-hidpp: divide-by-zero in thumbwheel scroll due to unprotected memset
- [Medium] hid: logitech-hidpp: M560 vertical scroll dropped on init failure
--

commit 67886caf8cb1bbca1343ff56109a68fec18fa393
Author: Erik Håkansson <erikhakan@gmail.com>

    HID: logitech: handle HID++ thumbwheel reports

This commit adds support for HID++ thumbwheel reports for devices with the
0x2150 feature and generalizes scroll handling. For Bolt devices, it defaults
to diverting thumbwheel reports to HID++ to get device index support.

> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index ff6a1095e7ba9..464685ad21bbc 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c

[ ... ]

> @@ -3323,7 +3391,8 @@ static int m560_raw_event(struct hid_device *hdev, u8 *data, int size)
>  		v = sign_extend32(data[6], 7);
>  		if (v != 0)
>  			hidpp_scroll_counter_handle_scroll(hidpp->input,
> -					&hidpp->vertical_wheel_counter, v);
> +					&hidpp->vertical_wheel_counter, v,
> +					REL_WHEEL_HI_RES, REL_WHEEL);

[Severity: Medium]
When high-resolution scroll initialization fails, scale_denominator remains
zero. In this scenario, hidpp_scroll_counter_handle_scroll() will return
false. Does this code need a fallback path to ensure standard low-resolution
scroll events are still reported instead of silently dropping the vertical
scroll completely?

>  
>  		input_sync(hidpp->input);
>  	}

[ ... ]

> @@ -4033,6 +4105,49 @@ static int hidpp_initialize_hires_scroll(struct hidpp_device *hidpp)
>  	return 0;
>  }
>  
> +static void thumbwheel_scroll_enable(struct hidpp_device *hidpp)
> +{
> +	bool divert;
> +	int ret;
> +
> +	memset(&hidpp->thumbwheel_counter, 0,
> +	       sizeof(hidpp->thumbwheel_counter));

[Severity: High]
Can this memset() race with hidpp20_thumbwheel_raw_event() in softirq context?

If a wireless device wakes from sleep, hidpp_connect_event() could be queued
in a worker thread, calling thumbwheel_scroll_enable().

If the user scrolls concurrently, hidpp20_thumbwheel_raw_event() might
evaluate the scale_denominator check using the old counter value:

	if (!hidpp->input || !hidpp->thumbwheel_counter.scale_numerator ||
	    !hidpp->thumbwheel_counter.scale_denominator)
		return 1;

And then the worker thread zeroes the struct just before it is used in
hidpp_scroll_counter_handle_scroll():

	scaled = div_s64((s64)value * 120 * counter->scale_numerator,
			 counter->scale_denominator);

Could this result in a divide-by-zero kernel panic?

> +	ret = hidpp_thumbwheel_get_info(hidpp,
> +			&hidpp->thumbwheel_counter.scale_numerator,
> +			&hidpp->thumbwheel_counter.scale_denominator,
> +			&hidpp->thumbwheel_positive_right);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002-bolt-input-rfc-v1-0-333e1f350586@gmail.com?part=2