Re: [PATCH v3 1/2] HID: logitech-hidpp: add HID++ 2.0 reprogrammable button support

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

Pre-existing issues:
- [High] Use-after-free of `hidpp->input` when `input_register_device` fails.
- [High] Out-of-bounds read in `hidpp_raw_hidpp_event()` when copying the report structure.
--

--- Patch [1]: [PATCH v3 1/2] HID: logitech-hidpp: add HID++ 2.0 reprogrammable button support ---
commit 8ed227c4b30632c93149b188c8de90eb6920c093
Author: Elliot Douglas <[email protected]>

HID: logitech-hidpp: add HID++ 2.0 reprogrammable button support

Some Logitech HID++ 2.0 mice can report diverted reprogrammable controls
through HID++ feature 0x1b04, SpecialKeysMseButtons / REPROG_CONTROLS_V4,
instead of the normal HID mouse report.

Link: https://lekensteyn.nl/files/logitech/x1b04_specialkeysmsebuttons.html

> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 70ba1a5e40d8e..f9189e14fb780 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
[ ... ]
> @@ -3601,6 +3606,195 @@ static int hidpp10_extra_mouse_buttons_raw_event(struct hidpp_device *hidpp,
>  	return 1;
>  }
>  
> +/* -------------------------------------------------------------------------- */
> +/* HID++2.0 reprogrammable controls                                           */
> +/* -------------------------------------------------------------------------- */
[ ... ]
> +static int hidpp20_reprog_controls_raw_event(struct hidpp_device *hidpp,
> +					     u8 *data, int size)
> +{
> +	const struct hidpp_reprog_control_mapping *mapping;
> +	struct hidpp_report *report = (struct hidpp_report *)data;
> +	u16 controls[4];
> +	bool pressed;
> +	unsigned int i, j;
> +
> +	if (!(hidpp->quirks & HIDPP_QUIRK_HIDPP_REPROG_CONTROLS_BTNS) ||
> +	    !hidpp->input ||
> +	    !hidpp->reprog_controls ||
> +	    hidpp->reprog_controls_feature_index == 0xff)
> +		return 0;

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to a
use-after-free of hidpp->input if input registration fails?

Looking at hidpp_connect_event() in drivers/hid/hid-logitech-hidpp.c, if
input_register_device() fails, it frees the input device but leaves the
global hidpp->input pointer dangling:

	ret = input_register_device(input);
	if (ret) {
		input_free_device(input);
		return;
	}

Since the device is actively receiving interrupts, incoming reports can trigger
raw event handlers like this newly added hidpp20_reprog_controls_raw_event().
The handler bypasses the !hidpp->input check because the pointer is not NULL,
and subsequently calling input_report_key() with the freed pointer could cause
a use-after-free.

> +
> +	if (size < HIDPP_REPORT_LONG_LENGTH ||
> +	    report->fap.feature_index != hidpp->reprog_controls_feature_index ||
> +	    report->fap.funcindex_clientid != HIDPP_REPROG_CONTROLS_EVENT_DIVERTED)
> +		return 0;
[ ... ]
> @@ -3971,6 +4168,10 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
>  			return ret;
>  	}
>  

[Severity: High]
This is a pre-existing issue, but can the struct assignment at the top of
hidpp_raw_hidpp_event() read out of bounds?

When a matching response is found, hidpp_raw_hidpp_event() performs this
assignment:

	if (hidpp_match_answer(question, report) ||
			hidpp_match_error(question, report)) {
		*answer = *report;
		hidpp->answer_available = true;
		wake_up(&hidpp->wait);

The struct hidpp_report is 64 bytes in size. However, the report pointer is cast
directly from the incoming data buffer. If a device only supports SHORT
(7 bytes) or LONG (20 bytes) reports, the data buffer will be sized accordingly.
Does the 64-byte struct copy read out of bounds from the smaller heap buffer
when the incoming report is matched?

> +	ret = hidpp20_reprog_controls_raw_event(hidpp, data, size);
> +	if (ret != 0)
> +		return ret;
> +
>  	if (hidpp->quirks & HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS) {
>  		ret = hidpp10_consumer_keys_raw_event(hidpp, data, size);
>  		if (ret != 0)

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