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 <[email protected]>

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.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> -	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/[email protected]?part=1