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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] TOCTOU race condition causing Use-After-Free during hidpp_connect_event error cleanup.
- [High] Use-After-Free due to dangling hidpp->input if hidinput_connect() fails during initial probe.
- [High] NULL pointer dereference in input_ff_upload if hidpp_ff_init fails midway.
--

commit 0f581c2f8ce1a26d47a4afafa4a00600b3d649dd
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..2800c31e54e41 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -4296,6 +4296,7 @@ static void hidpp_connect_event(struct work_struct *work)
>  	ret = input_register_device(input);
>  	if (ret) {
>  		input_free_device(input);
> +		hidpp->input = NULL;

[Severity: High]
This is a pre-existing issue, but does setting this pointer to NULL after
freeing the input device leave a race window?

Because hidpp_connect_event() runs asynchronously in a workqueue, incoming
HID reports can be processed concurrently by hidpp_raw_event() in
interrupt context. 

Could an interrupt arrive right after input_free_device() but before the
pointer is cleared, causing hidpp_raw_event() to bypass the
"if (!hidpp->input)" check and dereference the freed memory?

Would it be safer to clear the pointer before calling input_free_device(),
or does this require further synchronization since hidpp_raw_event()
accesses the pointer without holding locks?

[Severity: High]
This is a pre-existing issue, but does a similar use-after-free exist
during the initial probe path?

In hidpp_input_configured(), hidpp->input is persistently set before the
HID core attempts to register the input device:

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

If input_register_device() subsequently fails inside hidinput_connect(),
the input core destroys the input_dev object, but hidpp->input is never
cleared. 

Since hid_connect() can still succeed if other interfaces connect
successfully, incoming HID reports routed to hidpp_raw_event()
would then dereference the dangling hidpp->input pointer. 

Could this lead to a use-after-free?

[Severity: High]
This is a pre-existing issue, but is there a potential NULL pointer
dereference if hidpp_ff_init() fails midway?

In hidpp_ff_init(), input_ff_create() is called early, which allocates
dev->ff and enables the EV_FF bit. If a subsequent memory allocation fails:

drivers/hid/hid-logitech-hidpp.c:hidpp_ff_init() {
    ...
    data = kmemdup(data, sizeof(*data), GFP_KERNEL);
    if (!data)
        return -ENOMEM;
    ...
}

The function returns without calling input_ff_destroy(). Later,
hidpp_probe() explicitly ignores this error:

drivers/hid/hid-logitech-hidpp.c:hidpp_probe() {
    ...
    if (ret) {
        hid_warn(hidpp->hid_dev,
            "Unable to initialize force feedback support, errno %d\n",
            ret);
        ret = 0;
    }
    ...
}

This allows the broken device to be registered. If userspace then sends an
EVIOCSFF ioctl, the input core invokes input_ff_upload(), which
unconditionally calls the driver's ff->upload function. 

Since ff->upload was never assigned due to the early failure, could
dereferencing this NULL pointer cause a crash?

>  		return;
>  	}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/r7qq6043-p432-51o0-3s93-r9382q44n027@xreary.bet?part=1