Re: [PATCH v2 2/4] HID: asus: prevent wrong pointer cast

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing cleanup in asus_probe error path leads to Use-After-Free.
- [High] Calling ACPI method evaluation from HID raw_event handler leads to sleeping in atomic context.
--

commit 3fe25fc3ce0bf42eb96551214cb9606ee3c5ff97
Author: Denis Benato <denis.benato@linux.dev>

    HID: asus: prevent wrong pointer cast
    
    The firmware version check for ROG ally devices assumes the parent device
    is a USB device, therefore prevent possible out-of-bounds access by using
    hid_is_usb() to mitigate possible out-of-bounds access.

    Link: https://lore.kernel.org/all/20260612130125.AAAE71F000E9@smtp.kernel.org/

> 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
> @@ -753,7 +753,7 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
>  			return ret;
>  	}
>  
> -	if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
> +	if (hid_is_usb(hdev) && (drvdata->quirks & QUIRK_ROG_ALLY_XPAD)) {
>  		intf = to_usb_interface(hdev->dev.parent);
>  		udev = interface_to_usbdev(intf);
>  		validate_mcu_fw_version(hdev,

[Severity: High]
This isn't a bug introduced by this patch, but while looking at
asus_kbd_register_leds(), I noticed it registers a listener to a global list
and uses devm_kzalloc(). If a subsequent initialization step in asus_probe()
fails:

drivers/hid/hid-asus.c:asus_probe() {
    ...
			ret = asus_start_multitouch(hdev);
			if (ret)
				goto err_stop_hw;
    ...
err_stop_hw:
	hid_hw_stop(hdev);
	return ret;
}

Can this lead to a use-after-free on the listener?

The driver returns without unregistering the listener, and the devres
subsystem will automatically free the memory, leaving a dangling pointer
in the global list.

[Severity: High]
This is a pre-existing issue, but I also noticed that asus_raw_event() can
synchronously trigger an ACPI method evaluation:

drivers/hid/hid-asus.c:asus_raw_event() {
    ...
				int ret = asus_wmi_send_event(drvdata, ASUS_FAN_CTRL_KEY_CODE);
    ...
}

drivers/hid/hid-asus.c:asus_wmi_send_event() {
    ...
	err = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DEVS,
				       ASUS_WMI_METHODID_NOTIF, code, &retval);
    ...
}

For USB HID devices, asus_raw_event() is invoked from the URB completion
handler in atomic context. 

Since wmi_evaluate_method() acquires mutexes and may sleep, will this cause
the kernel to sleep in atomic context?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612142326.1704858-1-denis.benato@linux.dev?part=2