Re: [PATCH AUTOSEL 6.18-6.1] HID: hidpp: fix potential UAF in hidpp_connect_event()

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

Pre-existing issues:
- [Critical] Force feedback initialization in `hidpp_ff_init()` creates a TOCTOU race that leads to a guaranteed NULL pointer dereference in `input_ff_upload()`.
- [Critical] Missing `hid_hw_stop()` and resource cleanup in the `probe()` error path for G920 devices leads to a catastrophic Use-After-Free.
- [High] Concurrent access to `hidpp->input` between `hidpp_connect_event()` error path and `hidpp_raw_event()` leaves a race window leading to a Use-After-Free.
- [High] Unhandled permanent Use-After-Free for non-delayed initialization devices if `hidinput_connect()` fails.
- [High] Missing `cancel_work_sync(&hidpp->reset_hi_res_work)` in the `hidpp_probe()` error path leads to a Use-After-Free if the work was scheduled.
--

commit 4c51abc31c5c6d9a8be9e395178ab70d3b017d36
Author: Jiri Kosina [off-list ref]
Subject: HID: hidpp: fix potential UAF in hidpp_connect_event()

This commit attempts to fix a potential use-after-free (UAF) by setting
hidpp->input to NULL before calling input_free_device() in the error
path of hidpp_connect_event().

[Severity: High]
This is a pre-existing issue, but does this still leave a TOCTOU race window?

hidpp_populate_input() assigns the allocated input to hidpp->input before
calling input_register_device(), exposing it to concurrent raw hardware
events processed in SoftIRQ (like wtp_raw_event()).

If a concurrent SoftIRQ thread evaluates !hidpp->input just before it is
set to NULL here, will it subsequently dereference the freed pointer?

 		input_free_device(input);
 		return;
 	}

[Severity: High]
This isn't a bug introduced by this patch, but does this leave a permanent
dangling pointer if registration fails?

drivers/hid/hid-logitech-hidpp.c:hidpp_input_configured() {
	...
	if (!hidpp)
		return 0;

	hidpp_populate_input(hidpp, input);
	...
}

hidinput_connect() allocates input_dev and calls .input_configured, which
unconditionally sets hidpp->input = input here.

If input_register_device() then fails inside hidinput_connect(), the core
branches to out_unwind and frees the input device.

However, hid_connect() may still return success (e.g., if hidraw binds
successfully), allowing probe to finish. hidpp->input is never cleared and
remains a dangling pointer, accessed on the next hardware event.

[Severity: Critical]
This is a pre-existing issue, but does this FF initialization create a TOCTOU
race leading to a NULL pointer dereference?

drivers/hid/hid-logitech-hidpp.c:hidpp_probe() {
	...
	if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
		struct hidpp_ff_private_data data;

		ret = g920_get_config(hidpp, &data);
		if (!ret)
			ret = hidpp_ff_init(hidpp, &data);
	...
}

hid_connect() registers the input device, making it exposed to userspace.
Then hidpp_ff_init() is called, which calls input_ff_create() setting the
EV_FF bit.

If a subsequent memory allocation in hidpp_ff_init() fails, it returns
-ENOMEM leaving ff->upload as NULL. Because the device is already
registered, userspace can open it and issue the EVIOCSFF ioctl, which
will blindly execute ff->upload(dev, effect, old).

[Severity: Critical]
This is a pre-existing issue, but does this bypass the error cleanup labels
and skip hid_hw_stop() when G920 initialization fails?

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);
	}

	/*
	 * This relies on logi_dj_ll_close() being a no-op so that DJ connection
	 * events will still be received.
	 */
	hid_hw_close(hdev);
	return ret;
	...
}

When ret is non-zero, it returns directly here. This leaves the device
active in the HID subsystem, processing hardware interrupts, while the
driver's devm resources are freed by the core, leading to a Use-After-Free.

[Severity: High]
This is a pre-existing issue, but does this error path fail to cancel
reset_hi_res_work?

drivers/hid/hid-logitech-hidpp.c:hidpp_probe() {
	...
hid_hw_start_fail:
	sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
	cancel_work_sync(&hidpp->work);
	mutex_destroy(&hidpp->send_mutex);
	return ret;
}

hidpp_probe() calls hid_device_io_start(), enabling incoming hardware events.
A battery online event can trigger hidpp_raw_hidpp_event() to schedule
reset_hi_res_work.

If a subsequent step in probe (like hid_connect()) fails, the error cleanup
path cancels hidpp->work but misses hidpp->reset_hi_res_work. The hidpp
struct is then freed by devres, leaving the scheduled work item pointing to
freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831133314.4125787-633-sashal@kernel.org?part=1