[PATCH] HID: synchronize input before cleaning up a failed probe

[PATCH] HID: synchronize input before cleaning up a failed probe

[Yousef Alhouseen]

hid_device_io_start() allows reports to run concurrently with probe. If
the probe subsequently fails, __hid_device_probe() releases driver
resources and clears hdev->driver without first excluding those report
callbacks.

For example, a report may enter hidraw_report_event() while the failure
path frees the associated hidraw object, leading to a use-after-free when
the report takes the object's list lock.

Stop input before performing failed-probe cleanup. This reacquires
driver_input_lock and waits for any report callback already in progress.

Fixes: c849a6143bec ("HID: Separate struct hid_device's driver_lock into two locks.")
Reported-by: syzbot+9eebf5f6544c5e873858@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=9eebf5f6544c5e873858
Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
 drivers/hid/hid-core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 41a79e43c82b..469b849b7a2a 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2835,6 +2835,8 @@ static int __hid_device_probe(struct hid_device *hdev, struct hid_driver *hdrv)
 	 */
 
 	if (ret) {
+		if (hdev->io_started)
+			hid_device_io_stop(hdev);
 		devres_release_group(&hdev->dev, hdev->devres_group_id);
 		hid_close_report(hdev);
 		hdev->driver = NULL;
-- 
2.54.0

Re: [PATCH] HID: synchronize input before cleaning up a failed probe

[sashiko-bot]

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

Pre-existing issues:
- [High] Use-After-Free in hidraw_report_event() due to drivers calling hid_hw_stop() before hid_device_io_stop() on probe error paths.
--

commit 6c338032433c7e95cf76d26b6790e7b5f487a9b4
Author: Yousef Alhouseen <alhouseenyousef@gmail.com>

HID: synchronize input before cleaning up a failed probe
    
hid_device_io_start() allows reports to run concurrently with probe. If
the probe subsequently fails, __hid_device_probe() releases driver
resources and clears hdev->driver without first excluding those report
callbacks.

Closes: https://syzkaller.appspot.com/bug?extid=9eebf5f6544c5e873858

> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 41a79e43c82b3..469b849b7a2a4 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -2835,6 +2835,8 @@ static int __hid_device_probe(struct hid_device *hdev, struct hid_driver *hdrv)
>  	 */
>  
>  	if (ret) {
> +		if (hdev->io_started)
> +			hid_device_io_stop(hdev);

[Severity: High]
This is a pre-existing issue, but does this fix cover all the cases where the
use-after-free can occur?

If drivers call hid_hw_stop(hdev) in their own probe error paths before
returning an error, they might bypass the core's new hid_device_io_stop()
cleanup.

hid_hw_stop() unconditionally calls hid_disconnect(), which frees the hidraw
object. If HID_QUIRK_ALWAYS_POLL is set (or for Bluetooth devices), polling
continues, and a concurrent report could enter hidraw_report_event() with a
dangling pointer before driver_input_lock is taken.

For example, this pattern appears in nintendo_hid_probe():

drivers/hid/hid-nintendo.c:nintendo_hid_probe() {
    ...
err_close:
	hid_hw_close(hdev);
err_stop:
	hid_hw_stop(hdev);
    ...
}

And similarly in corsairpsu_probe():

drivers/hwmon/corsair-psu.c:corsairpsu_probe() {
    ...
fail_and_close:
	hid_hw_close(hdev);
fail_and_stop:
	hid_hw_stop(hdev);
    ...
}

Could these drivers also trigger the use-after-free by tearing down the
hardware and freeing hidraw while IO is still started and driver_input_lock
is unheld?

>  		devres_release_group(&hdev->dev, hdev->devres_group_id);
>  		hid_close_report(hdev);
>  		hdev->driver = NULL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260628005846.31248-1-alhouseenyousef@gmail.com?part=1