Re: [PATCH v2 4/4] HID: asus: add i2c entry for FA808UM and other TUFs

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Improper Workqueue Initialization / Memory Corruption via Re-initialization
- [High] Use-After-Free due to Race Condition in `asus_remove()`
- [High] Missing NULL check for `devm_kasprintf` allocation leading to Kernel Oops
--

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

HID: asus: add i2c entry for FA808UM and other TUFs

On newer TUF laptops the keyboard HID device uses the same PID/VID of a
USB device that was found in ROG laptops: add it to hid-asus as i2c too.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 0f78242b6a1db..7964fc96196e5 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -1565,6 +1565,9 @@ static const struct hid_device_id asus_devices[] = {
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>  	    USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
>  	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_HID_FN_LOCK },
> +	{ HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK,
> +	    USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
> +	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_HID_FN_LOCK },

[Severity: Critical]
This is a pre-existing issue, but since this patch enables QUIRK_HID_FN_LOCK
for a new device, could the fn_lock_sync_work initialization cause workqueue
corruption?

In asus_input_configured(), INIT_WORK(&drvdata->fn_lock_sync_work, ...) is
called per-input. For devices with multiple inputs, if hid_hw_start() enables
interrupts before all inputs are configured, an early KEY_FN_ESC event could
schedule the work before the next input is configured. 

The next call to asus_input_configured() would then re-execute INIT_WORK() on
the already queued work item, clearing its list pointers and causing a panic. 

Additionally, for devices with 0 inputs, it is never initialized, causing a
warning in cancel_work_sync().

[Severity: High]
This is a pre-existing issue, but is there a potential use-after-free during
device removal when QUIRK_HID_FN_LOCK is enabled?

In asus_remove(), cancel_work_sync(&drvdata->fn_lock_sync_work) is called
before hid_hw_stop(hdev). Since the hardware is still active, a late
KEY_FN_ESC event can trigger asus_event() and unconditionally call
schedule_work() after the work was supposedly canceled. 

Once the driver is removed and drvdata is freed by devres, could the
workqueue execute asus_sync_fn_lock() and dereference the freed drvdata?

[Severity: High]
This is a pre-existing issue, but does the XGM LED name allocation need a
NULL check?

In asus_probe(), devm_kasprintf() is used to allocate the name for
drvdata->xgm_led->cdev.name:

	drvdata->xgm_led->cdev.name = devm_kasprintf(&hdev->dev, GFP_KERNEL,
			"asus:xgm-%s:led",
			strlen(hdev->uniq) ?
			hdev->uniq : dev_name(&hdev->dev));
	drvdata->xgm_led->cdev.brightness = 1;

If devm_kasprintf() fails under memory pressure, it returns NULL. This NULL
pointer is then passed to led_classdev_register(), which passes it to
led_classdev_next_name() and eventually to strscpy() as the source string. 
Since strscpy() does not check for a NULL source, this would cause an
immediate NULL pointer dereference.

>  	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>  	    USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
>  	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },

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