Re: [PATCH v2] 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:
- [High] Use-after-free of `wd->input` on input device registration failure.
- [Medium] Devres memory leak of `devm_name` on connection error paths.
- [Medium] Mismatch between advertised clickpad capabilities and actual button events.
- [Low] Compilation failure due to use of undeclared variable `wd`.
- [Low] Missing READ_ONCE barrier when accessing `wd->input`.
--

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

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

This commit adds support for the native raw touchpad reports on the
Logitech K400 Plus by creating a separate multitouch input device.

Link: https://lore.kernel.org/linux-input/20260824132445.C64EE1F000E9@smtp.kernel.org/

> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 1504de32b1c84..9a6b04283130f 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -2963,8 +2966,13 @@ static int wtp_populate_input(struct hidpp_device *hidpp,
>  
> [ ... ]
>  
> -	/* 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);
>  
>  	if (hidpp->quirks & HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS)
>  		input_set_capability(input_dev, EV_KEY, BTN_RIGHT);
>  	else
>  		__set_bit(INPUT_PROP_BUTTONPAD, input_dev->propbit);

[Severity: Medium]
Are these capabilities accurately reflecting the device's behavior?

The commit message states that clickpad buttons should not be exposed
for the CLASS_KBD_WTP device. However, wtp_populate_input() still sets
the INPUT_PROP_BUTTONPAD property and the BTN_LEFT capability.

Userspace input stacks might see these capabilities and treat the device
as a clickpad, waiting for physical clicks that the driver explicitly
suppresses.

> @@ -3066,8 +3079,13 @@ static int wtp_raw_event(struct hid_device *hdev, u8 *data, int size)
>  	struct wtp_data *wd = hidpp->private_data;
>  	struct hidpp_report *report = (struct hidpp_report *)data;
>  	struct hidpp_touchpad_raw_xy raw;
> +	struct input_dev *input;
>  
> -	if (!wd || !hidpp->input)
> +	if (!wd)
> +		return 1;
> +
> +	input = wd->input;
> +	if (!input)
>  		return 1;

[Severity: Low]
Since wd->input is published locklessly using WRITE_ONCE() in
hidpp_connect_event(), shouldn't we use READ_ONCE() here in
wtp_raw_event() to ensure proper memory ordering?

> @@ -4524,7 +4562,54 @@ 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) {
> +		if (!wd->input) {
> +			struct wtp_data *wd = hidpp->private_data;

[Severity: Low]
Does this cause a compilation failure? The if condition evaluates
wd->input before the wd variable is declared and initialized on the
very next line.

> +
> +			ret = wtp_get_config(hidpp);
> +			if (ret) {
> +				hid_err(hdev, "cannot get WTP config: %d\n", ret);
> +				return;
> +			}
> +
> +			input = hidpp_allocate_input(hdev);
> +			if (!input) {
> +				hid_err(hdev, "cannot allocate touchpad input device\n");
> +				return;
> +			}
> +
> +			devm_name = devm_kasprintf(&hdev->dev, GFP_KERNEL,
> +						   "%s Touchpad", hidpp->name);
> +			if (!devm_name)
> +				return;

[Severity: Medium]
Can this cause a memory leak if a subsequent step like
wtp_populate_input() or input_register_device() fails?

If a failure occurs, the function returns without setting wd->input.
Because the string is devres-managed and bound to the parent HID device,
the next time hidpp_connect_event() runs, the if (!wd->input) check will
still be true, allocating another string that won't be freed until the
parent device is unbound.

> @@ -4615,7 +4700,12 @@ static void hidpp_connect_event(struct work_struct *work)
>  		return;
>  	}
>  
> -	hidpp_populate_input(hidpp, input);
> +	ret = hidpp_populate_input(hidpp, input);
> +	if (ret) {
> +		hidpp->input = NULL;
> +		input_free_device(input);
> +		return;
> +	}

[Severity: High]
Could this lead to a use-after-free on the event reporting path if
registration fails?

In hidpp_populate_input(), the pointer is assigned to the private
structure:

hidpp_populate_input() {
        ...
        wd->input = input;
        ...
}

If input_register_device() fails shortly after, this error path frees the
input device and clears hidpp->input, but does not clear wd->input.

When the hardware sends a raw HID event, wtp_raw_event() will fetch and
dereference the dangling wd->input pointer.

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