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 [off-list ref]

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

[ ... ]

[ ... ]

+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;

[ ... ]

[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/20260812195842.77724-1-edouglas7358@gmail.com?part=1