Re: [PATCH] HID: logitech-hidpp: add native touchpad support for K400 Plus

[sashiko-bot@kernel.org]

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

New issues:
- [High] Early assignment of wd->input creates a Time-of-Check to Time-of-Use (TOCTOU) race condition with the interrupt handler, leading to a NULL pointer dereference.
- [Medium] Repeated connection failures in the hidpp_connect_event workqueue lead to a devres memory leak due to missing input_free_device() on error paths.
- [Medium] The K400 Plus touchpad device incorrectly advertises INPUT_PROP_BUTTONPAD and emits conflicting BTN_LEFT events.

Pre-existing issues:
- [High] The return value of input_mt_init_slots is ignored, potentially registering a broken MT device.
- [High] Missing cancel_work_sync for reset_hi_res_work in the probe error path leads to a Use-After-Free.
--

--- Patch [1]: [PATCH] HID: logitech-hidpp: add native touchpad support for K400 Plus ---
commit f41a7d9a8fed9a9f7dba87c18d14bd22f84ca93d
Author: Dmitriy Geels <dmitriy.geels@gmail.com>

HID: logitech-hidpp: add native touchpad support for K400 Plus

This patch adds native raw touchpad support for the Logitech K400 Plus by
creating a separate multitouch input device. It ensures physical button
reports remain on the existing HID input device and disables ABS_MT_PRESSURE
when contact area is zero to prevent hover interpretation.

> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 1504de32b1c84..3cb239d818882 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c

[ ... ]

> @@ -2953,6 +2956,8 @@ static void wtp_populate_input(struct hidpp_device *hidpp,
>  {
>  	struct wtp_data *wd = hidpp->private_data;
>  
> +	wd->input = input_dev;
> +

[Severity: High]
Could this early assignment create a race condition? If an interrupt arrives
here, wtp_raw_event() will see wd->input as non-NULL. If
input_register_device() subsequently fails in hidpp_connect_event(), it
sets wd->input to NULL. If wtp_raw_event() uses the pointer exactly when it
is cleared, could this cause a NULL pointer dereference in input_event()?

>  	__set_bit(EV_ABS, input_dev->evbit);
>  	__set_bit(EV_KEY, input_dev->evbit);
>  	__clear_bit(EV_REL, input_dev->evbit);

[ ... ]

> @@ -2963,8 +2968,13 @@ static void wtp_populate_input(struct hidpp_device *hidpp,
>  	input_set_abs_params(input_dev, ABS_MT_POSITION_Y, 0, wd->y_size, 0, 0);
>  	input_abs_set_res(input_dev, ABS_MT_POSITION_Y, wd->resolution);
>  
> -	/* Max pressure is not given by the devices, pick one */
> -	input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 50, 0, 0);
> +	/*
> +	 * The driver exposes contact area as pressure.  Do not advertise a
> +	 * pressure axis when the device does not report contact area; userspace
> +	 * may otherwise interpret a permanently-zero pressure value as hover.
> +	 */
> +	if (wd->area_range)
> +		input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 50, 0, 0);
>  
>  	input_set_capability(input_dev, EV_KEY, BTN_LEFT);

[Severity: Medium]
Further down in this function, devices without HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS
are assigned INPUT_PROP_BUTTONPAD. Since the K400 Plus uses
HIDPP_QUIRK_CLASS_KBD_WTP without that quirk, does this cause userspace to
misconfigure the device, given that the K400 Plus actually has discrete
physical buttons?

[Severity: High]
This is a pre-existing issue, but the return value of input_mt_init_slots()
called slightly below here is ignored. If memory allocation fails, dev->mt
remains NULL, which will cause the core to silently drop all subsequent
multitouch events.

[ ... ]

> @@ -3013,11 +3026,11 @@ static void wtp_send_raw_xy_event(struct hidpp_device *hidpp,
>  
>  	if (raw->end_of_frame &&
>  	    !(hidpp->quirks & HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS))
> -		input_event(hidpp->input, EV_KEY, BTN_LEFT, raw->button);
> +		input_event(wd->input, EV_KEY, BTN_LEFT, raw->button);

[Severity: Medium]
Could this emit conflicting BTN_LEFT events? For the K400 Plus, the
physical buttons are routed through the other interface, so raw->button
is typically 0. This might spam release events at the end of every frame
and conflict with the physical hardware buttons.

[ ... ]

> @@ -4524,7 +4549,45 @@ static void hidpp_connect_event(struct work_struct *work)
>  		return;
>  	}
>  
> -	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
> +	if (hidpp->quirks & HIDPP_QUIRK_CLASS_KBD_WTP) {
> +		struct wtp_data *wd = hidpp->private_data;

[ ... ]

> +			ret = input_register_device(input);
> +			if (ret) {
> +				hid_err(hdev,
> +					"cannot register touchpad input device: %d\n",
> +					ret);
> +				wd->input = NULL;
> +				return;
> +			}

[Severity: Medium]
If input_register_device() fails, could this leak the devres allocations for
both the input device and devm_name? Since hidpp_connect_event() is a
workqueue that can run multiple times on reconnection, these allocations
might permanently accumulate without a call to input_free_device().

[ ... ]

> @@ -4755,7 +4818,8 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  	    hidpp_application_equals(hdev, HID_GD_KEYBOARD))
>  		hidpp->quirks |= HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS;
>  
> -	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
> +	if (hidpp->quirks &
> +	    (HIDPP_QUIRK_CLASS_WTP | HIDPP_QUIRK_CLASS_KBD_WTP)) {
>  		ret = wtp_allocate(hdev, id);
>  		if (ret)
>  			return ret;

[Severity: High]
This is a pre-existing issue, but earlier in hidpp_probe(),
hidpp->reset_hi_res_work is initialized. In the hid_hw_start_fail error path
(which executes after wtp_allocate), cancel_work_sync(&hidpp->work) is
called, but cancel_work_sync(&hidpp->reset_hi_res_work) is omitted. If the
devres group is freed, could a pending high-res scroll work item access
freed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824130901.84600-1-dmitriy.geels@gmail.com?part=1