Re: [PATCH] HID: logitech-hidpp: fix hi-res scroll for Bolt-connected MX Master

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unconditional and improperly guarded cast of the parent device to `struct hid_device` in `hidpp_is_bolt_child()` causes type confusion and out-of-bounds memory access for Bluetooth and native USB devices.
- [High] Time-of-check to time-of-use (TOCTOU) data race between `hidpp20_hires_wheel_raw_event()` and `hidpp_connect_event()` leading to a Use-After-Free of `hidpp->input`.
--

commit 1aae454b03bbd4a45bdfdb3bc2a196f15d06fbfa
Author: Rafael Passos <rafael@rcpassos.me>

HID: logitech-hidpp: fix hi-res scroll for Bolt-connected MX Master

This commit fixes hi-res scrolling on the MX Master 3S when connected
via the Bolt receiver by routing wheel events through the HID++ driver
instead of hid_generic. It also adjusts initialization checks for Bolt
devices.

> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 1504de32b1c84..960543bfd5ea1 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -3900,6 +3900,19 @@ static int hidpp10_consumer_keys_raw_event(struct hidpp_device *hidpp,
>  	return 1;
>  }
>  
> +static bool hidpp_is_bolt_child(struct hid_device *hdev)
> +{
> +	struct device *parent = hdev->dev.parent;
> +	struct hid_device *receiver_hdev;
> +
> +	if (!parent)
> +		return false;
> +
> +	receiver_hdev = to_hid_device(parent);
> +	return receiver_hdev->vendor == USB_VENDOR_ID_LOGITECH &&
> +	       receiver_hdev->product == USB_DEVICE_ID_LOGITECH_BOLT_RECEIVER;
> +}
> +

[Severity: High]
Does this safely handle native USB devices? When this is called for a native
USB device, the parent device is a struct usb_interface, not a struct
hid_device. 

Using to_hid_device(parent) subtracts the container_of offset for a
hid_device from the dev pointer, resulting in a pointer well before the
allocated usb_interface structure. Accessing receiver_hdev->vendor will then
cause an out-of-bounds memory read.

[ ... ]

> @@ -3910,7 +3923,9 @@ static int hi_res_scroll_enable(struct hidpp_device *hidpp)
>  	u8 multiplier = 1;
>  
>  	if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP20_HI_RES_WHEEL) {
> -		ret = hidpp_hrw_set_wheel_mode(hidpp, false, true, false);
> +		bool use_hidpp = hidpp_is_bolt_child(hidpp->hid_dev);
> +

[Severity: High]
Could this unconditionally trigger the out-of-bounds read described above for
all HID++ 2.0 devices? This includes native USB and Bluetooth devices where
the parent is not a hid_device.

[ ... ]

> @@ -3998,6 +4013,19 @@ static int hidpp20_hires_wheel_raw_event(struct hidpp_device *hidpp,
>  		return 1;
>  	}
>  
> +	/* wheel movement event: 16-bit signed delta in HID++ ticks */
> +	if ((data[3] & 0xf0) == 0x00 && size >= 7 && hidpp->input &&
> +	    hidpp->vertical_wheel_counter.wheel_multiplier) {
> +		s16 delta = get_unaligned_be16(&data[5]);
> +
> +		if (delta) {
> +			hidpp_scroll_counter_handle_scroll(hidpp->input,
> +				&hidpp->vertical_wheel_counter, delta);

[Severity: High]
Is there a Time-of-Check to Time-of-Use (TOCTOU) race condition here?

Since hid_device_io_start() has already been called during probe, hardware
events can fire concurrently with hidpp_connect_event().

If this raw event handler checks hidpp->input and proceeds, but
concurrently hidpp_connect_event() encounters an error (e.g. from
input_register_device failing) and executes:

    hidpp->input = NULL;
    input_free_device(input);

Could this lockless dereference of hidpp->input in
hidpp_scroll_counter_handle_scroll() result in a use-after-free?

[ ... ]

> @@ -4579,7 +4594,9 @@ static void hidpp_connect_event(struct work_struct *work)
>  	}
>  
>  	hidpp_initialize_battery(hidpp);
> -	if (!hid_is_usb(hidpp->hid_dev))
> +
> +	if (!hid_is_usb(hidpp->hid_dev) ||
> +	    hidpp_is_bolt_child(hidpp->hid_dev))
>  		hidpp_initialize_hires_scroll(hidpp);

[Severity: High]
Could this also trigger the out-of-bounds read in hidpp_is_bolt_child() for
native USB devices?

For a native USB device, hid_is_usb() is true, so !hid_is_usb() evaluates to
false. This forces the logical OR to evaluate hidpp_is_bolt_child(), passing
the USB device into the function that unsafely casts the parent device.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904034843.1340846-1-rafael@rcpassos.me?part=1