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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Dangling pointer `hidpp->input` leads to Use-After-Free if `input_register_device()` fails.
- [High] Unbounded 64-byte structure copy in `hidpp_raw_hidpp_event` causes a Kernel Heap Out-of-Bounds Read.
--

commit 13dc21807e8eb6f76d9029e89353e4bd2e6aa1c3
Author: Elliot Douglas <edouglas7358@gmail.com>
Subject: 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,
[ ... ]
> +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 ||

[Severity: High]
This is a pre-existing issue, but could this lead to a use-after-free?

Looking at hidpp_connect_event(), if input_register_device() fails, it
cleans up by calling input_free_device() but leaves the hidpp->input 
pointer dangling:

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

Because hid_device_io_start() has already enabled incoming packets before
this point, could a newly arriving event trigger this new
hidpp20_reprog_controls_raw_event() handler, pass the !hidpp->input check,
and dereference the freed pointer?

> +	    !hidpp->reprog_controls ||
> +	    hidpp->reprog_controls_feature_index == 0xff)
> +		return 0;
[ ... ]
> @@ -3971,6 +4168,10 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
>  			return ret;
>  	}
>  
> +	ret = hidpp20_reprog_controls_raw_event(hidpp, data, size);
> +	if (ret != 0)
> +		return ret;
> +

[Severity: High]
This is a pre-existing issue, but does hidpp_raw_hidpp_event() perform an
out-of-bounds read on the incoming data buffer?

Earlier in this function, if the packet matches a pending question, it does
a direct structure assignment:

    if (hidpp_match_answer(question, report) ||
            hidpp_match_error(question, report)) {
        *answer = *report;

Since struct hidpp_report is a 64-byte packed structure, won't this pointer
assignment perform a fixed 64-byte copy regardless of the actual size of the
received buffer (which could be as small as 7 bytes for a 
REPORT_ID_HIDPP_SHORT packet)? 

Could this copy adjacent heap memory into the answer buffer, potentially 
leaking it when the data is parsed later?

>  	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/cover.1783094954.git.edouglas7358@gmail.com?part=1