Re: [PATCH] HID: asus: fix touchpad multitouch initialization regression

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unconditionally calling `asus_start_multitouch()` outside the `HID_CLAIMED_INPUT` check introduces a Use-After-Free if input registration fails.

Pre-existing issues:
- [High] Incorrect cleanup order in `asus_remove()` creates a Use-After-Free race condition by cancelling the workqueue before disabling hardware events.
- [High] Missing `hid_is_usb()` check in `asus_kbd_register_leds()` allows an invalid cast to `usb_interface`, causing out-of-bounds reads or crashes.
- [Medium] Modifying `input->name` after device registration creates a data race and violates input subsystem API requirements.
--

--- Patch [1]: [PATCH] HID: asus: fix touchpad multitouch initialization regression ---
commit 53becd94719f665ca15fc8c3f4b7876a402da45f
Author: Panz Dev <panz.development@gmail.com>

HID: asus: fix touchpad multitouch initialization regression

This patch fixes probe regressions for ASUS I2C/HID touchpads by skipping
keyboard report initialization for touchpads and moving
asus_start_multitouch() outside the claimed input check block.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 3f5e96900b67a..72b93bdeb4e8f 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -1327,12 +1329,12 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  			drvdata->input->name = "Asus TouchPad";
>  		else
>  			drvdata->input->name = "Asus Keyboard";

[Severity: Medium]
This is a pre-existing issue, but can modifying drvdata->input->name here
after device registration (which occurs earlier during hid_hw_start) create a
data race?

If a userspace process like udev reads the name attribute concurrently, might
it read the default string or race with the pointer reassignment?

> +	}
>  
> -		if (drvdata->tp) {
> -			ret = asus_start_multitouch(hdev);
> -			if (ret)
> -				goto err_stop_hw;
> -		}
> +	if (drvdata->tp) {
> +		ret = asus_start_multitouch(hdev);
> +		if (ret)
> +			goto err_stop_hw;
>  	}

[Severity: High]
Does moving asus_start_multitouch() outside the HID_CLAIMED_INPUT check
introduce a use-after-free regression if input registration fails?

If input_register_device() fails during hid_hw_start(), hidinput_connect()
frees the allocated input_dev and returns an error without setting
HID_CLAIMED_INPUT.

Since the HID_CLAIMED_INPUT safety guard is now removed, the driver
will force the touchpad hardware to start sending reports. When the hardware
interrupt arrives, asus_raw_event() intercepts it and routes to
asus_report_input():

drivers/hid/hid-asus.c:asus_raw_event() {
    ...
	if (drvdata->tp && data[0] == INPUT_REPORT_ID)
		return asus_report_input(drvdata, data, size);
    ...
}

This blindly dereferences the freed drvdata->input pointer, which was assigned
earlier in asus_input_configured():

drivers/hid/hid-asus.c:asus_report_input() {
    ...
		input_mt_slot(drvdat->input, i);
		input_mt_report_slot_state(drvdat->input, toolType, down);
    ...
}

Is it possible for this to cause a use-after-free, for instance if triggered
via uhid device spoofing or resource allocation failures during probe?

>  
>  	return 0;

[Severity: High]
This isn't a bug introduced by this patch, but does the cleanup order in
asus_remove() create a use-after-free race condition?

In asus_remove(), the driver calls:

drivers/hid/hid-asus.c:asus_remove() {
    ...
	if (drvdata->quirks & QUIRK_HID_FN_LOCK)
		cancel_work_sync(&drvdata->fn_lock_sync_work);

	hid_hw_stop(hdev);
    ...
}

If the device sends a KEY_FN_ESC report via hardware interrupt immediately
after cancel_work_sync() returns but before hid_hw_stop() completes, might
asus_event() re-queue the work via schedule_work()? Once asus_remove()
finishes, the devm-managed drvdata is freed, which could leave the work item
scheduled and lead to a use-after-free when the worker executes.

[Severity: High]
This is a pre-existing issue, but could the lack of a hid_is_usb() check in
asus_kbd_register_leds() allow an invalid cast?

When handling the QUIRK_ROG_ALLY_XPAD quirk:

drivers/hid/hid-asus.c:asus_kbd_register_leds() {
    ...
	if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
		intf = to_usb_interface(hdev->dev.parent);
		udev = interface_to_usbdev(intf);
    ...
}

If the device is connected via another transport (like Bluetooth, I2C, or
spoofed via uhid), the parent device is not a USB interface. Would calling
interface_to_usbdev() on this invalid pointer lead to out-of-bounds
memory access or a crash?

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