Re: [PATCH v2] HID: asus: do not send keyboard init reports to touchpads

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

Pre-existing issues:
- [Critical] Missing `hid_is_usb()` check before casting the parent device to a `usb_interface` in `asus_kbd_register_leds()`.
- [High] Dangling pointer `drvdata->kbd_backlight` causes a Use-After-Free in `asus_remove()` if `asus_hid_register_listener()` fails.
- [High] Incorrect teardown order in `asus_remove()` cancels `fn_lock_sync_work` before stopping the hardware, leading to a Use-After-Free race condition.
- [High] Dangling pointer dereference of `drvdata->input` in `asus_probe()` when a device has multiple report IDs or applications and the last one has no mapped usages.
--

--- Patch [1]: [PATCH v2] HID: asus: do not send keyboard init reports to touchpads ---

commit 83a2843d77c483e6b894c00208643414ac5b2d26
Author: Panz Dev [off-list ref]

HID: asus: do not send keyboard init reports to touchpads

This commit avoids sending keyboard feature reports to touchpad endpoints
by wrapping the initialization loop in an if (!drvdata->tp) check. This
prevents sending invalid feature requests that could corrupt the touchpad
probe state.

[ ... ]

-	for (int r = 0; r < ARRAY_SIZE(asus_report_id_init); r++) {
-		if (asus_has_report_id(hdev, asus_report_id_init[r])) {
-			ret = asus_kbd_init(hdev, asus_report_id_init[r]);
-			if (ret < 0)
-				hid_warn(hdev, "Failed to initialize 0x%x: %d.\n",
-					 asus_report_id_init[r], ret);
+	if (!drvdata->tp) {
+		for (int r = 0; r < ARRAY_SIZE(asus_report_id_init); r++) {
+			if (asus_has_report_id(hdev, asus_report_id_init[r])) {
+				ret = asus_kbd_init(hdev, asus_report_id_init[r]);
+				if (ret < 0)
+					hid_warn(hdev, "Failed to initialize 0x%x: %d.\n",
+						 asus_report_id_init[r], ret);
+			}
 		}
 	}

[ ... ]

 	if (drvdata->input && (hdev->claimed & HID_CLAIMED_INPUT)) {
 		if (drvdata->tp)
 			drvdata->input->name = "Asus TouchPad";

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

When a device uses HID_QUIRK_MULTI_INPUT, hidinput_connect() creates a
separate hid_input for each application. The asus_input_configured() function
unconditionally assigns drvdata->input = hi->input.

If the last report ID processed has no valid input usages, the hid_input and
its input_dev are freed by the core, leaving drvdata->input pointing to
freed memory.

Since earlier valid report IDs successfully registered, HID_CLAIMED_INPUT is
set. Could this dereference the freed pointer here to set the name?

While reviewing the driver, I also noticed a few other pre-existing issues
in this file:

static int asus_kbd_register_leds(struct hid_device *hdev)
{

[ ... ]

	if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
		intf = to_usb_interface(hdev->dev.parent);
		udev = interface_to_usbdev(intf);
		validate_mcu_fw_version(hdev,
			le16_to_cpu(udev->descriptor.idProduct));
	}

[Severity: Critical]
This is a pre-existing issue, but does asus_kbd_register_leds() need to verify
the transport type before casting the parent device?

If the driver is bound via a non-USB transport like an injected uhid device,
the parent is not a USB interface. Can this read of the product ID access
invalid memory and cause a crash?

[ ... ]

	ret = asus_hid_register_listener(&drvdata->kbd_backlight->listener);
	if (ret < 0) {
		/* No need to have this still around */
		devm_kfree(&hdev->dev, drvdata->kbd_backlight);
	}

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

The error path calls devm_kfree() on drvdata->kbd_backlight but doesn't
set the pointer to NULL.

During unbind, asus_remove() checks if (drvdata->kbd_backlight) and
dereferences it. Could this cause a use-after-free?

[ ... ]

static void asus_remove(struct hid_device *hdev)
{
	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);

[ ... ]

	if (drvdata->quirks & QUIRK_HID_FN_LOCK)
		cancel_work_sync(&drvdata->fn_lock_sync_work);

	hid_hw_stop(hdev);

[Severity: High]
This is a pre-existing issue, but is the teardown order in asus_remove()
safe from race conditions?

cancel_work_sync() is called before hid_hw_stop(). Because the hardware is
not stopped, incoming HID reports can continue to trigger asus_event() and
unconditionally re-queue the worker:

drivers/hid/hid-asus.c:asus_event() {
    ...
    if (drvdata->quirks & QUIRK_HID_FN_LOCK) {
        drvdata->fn_lock = !drvdata->fn_lock;
        schedule_work(&drvdata->fn_lock_sync_work);
    }
    ...
}

Could the pending work item execute after devres frees drvdata, leading to a
use-after-free?

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