Re: [PATCH] HID: hidpp: fix potential UAF in hidpp_connect_event()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Dangling pointer Use-After-Free when `input_register_device()` fails during synchronous probe via `hid_connect()`.
- [High] Time-of-Check to Time-of-Use (TOCTOU) race condition leading to a Use-After-Free.
--

commit 958cf4bb8a3ce7336c278ac4fb346b928f5fe9b9
Author: Jiri Kosina <jikos@kernel.org>

HID: hidpp: fix potential UAF in hidpp_connect_event()
    
If input_register_device() fails, we call input_free_device(), but keep
stale pointer to the old device in hidpp->input, which could potentially
lead to UAF. Fix that by resetting it to NULL before returning from
hidpp_connect_event().

Reported-by: zdi-disclosures@trendmicro.com
Signed-off-by: Jiri Kosina <jkosina@suse.com>

> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index ccbf28869a968..d8e86b6ccf375 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -4295,6 +4295,7 @@ static void hidpp_connect_event(struct work_struct *work)
>  
>  	ret = input_register_device(input);
>  	if (ret) {
> +		hidpp->input = NULL;

[Severity: High]
This is a pre-existing issue, but does this clear the shared state too late to
prevent a race condition?

Since hidpp_connect_event() runs asynchronously in a workqueue, hardware
events can be processed concurrently in interrupt context. The allocated
device pointer is published to the global hidpp->input state via
hidpp_populate_input() before the registration attempt.

A concurrent raw event could read hidpp->input and pass the safety check:

drivers/hid/hid-logitech-hidpp.c:wtp_raw_event() {
	if (!wd || !hidpp->input)
		return 1;
}

If input_register_device() then fails, the error path clears hidpp->input
and frees the memory locklessly.

Could the concurrent raw event handler, already past the safety check,
access the freed memory and cause a use-after-free?

>  		input_free_device(input);
>  		return;

[Severity: Critical]
This is also a pre-existing issue, but does a similar dangling pointer remain
if registration fails during synchronous probe?

While this patch addresses a dangling pointer in hidpp_connect_event(), a
similar situation appears to exist in the hid core connection path. For
non-delayed devices, hidinput_connect() calls hidpp_input_configured(),
which caches hidinput->input into hidpp->input:

drivers/hid/hid-logitech-hidpp.c:hidpp_input_configured() {
	hidpp_populate_input(hidpp, input);
}

Immediately after, hidinput_connect() calls input_register_device(). If
registration fails, the error path frees the device:

drivers/hid/hid-input.c:hidinput_connect() {
		if (input_register_device(hidinput->input))
			goto out_unwind;
}

Because hid_connect() ignores the hidinput_connect() failure when the
driver has a raw_event handler, probe succeeds and the device remains active.

Would incoming hardware events then trigger hidpp_raw_event(), successfully
pass the check against hidpp->input, and dereference the freed pointer?

>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/qq420675-14n0-p23s-10oo-1n8113s2nq22@xreary.bet?part=1