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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-After-Free during device removal if asus_kbd_register_leds() fails
- [High] Invalid type cast and Out-of-Bounds read for non-USB devices mimicking QUIRK_ROG_ALLY_XPAD
--

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

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

This patch adds an I2C HID device entry for the ASUS ROG NKEY Keyboard 2
in the hid-asus driver. This allows newer TUF laptops, which use the same
PID/VID over I2C instead of USB, to successfully bind to the driver.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 3f5e96900b67a..5a2ab3cfef0b5 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -1494,6 +1494,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: High]
This isn't a bug introduced by this patch, but while looking at the
QUIRK_USE_KBD_BACKLIGHT initialization, could there be a use-after-free
during device removal if asus_kbd_register_leds() fails?

If asus_hid_register_listener() fails within asus_kbd_register_leds() (e.g.,
due to missing asus-wmi or another keyboard already being registered), the
allocated drvdata->kbd_backlight is freed using devm_kfree():

	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);
	}

However, the pointer itself is not set to NULL. Since this error is
intentionally ignored by the caller (asus_probe()), the device probe
succeeds.

When the device is later unbound or unplugged, asus_remove() checks
if (drvdata->kbd_backlight) and dereferences the dangling pointer to
unregister the listener:

static void asus_remove(struct hid_device *hdev)
{
	...
	if (drvdata->kbd_backlight) {
		asus_hid_unregister_listener(&drvdata->kbd_backlight->listener);

Will this unconditionally dereference the dangling pointer, leading to a
use-after-free?

[Severity: High]
This is a pre-existing issue, but while looking at devices mimicking
QUIRK_ROG_ALLY_XPAD, could asus_kbd_register_leds() perform an invalid
type cast and out-of-bounds read?

In asus_kbd_register_leds(), devices with QUIRK_ROG_ALLY_XPAD unconditionally
cast the parent device to a usb_interface:

	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));
	}

This assumes the device is always backed by a USB transport.

However, a local user can use uhid to inject a fake HID device with matching
USB vendor and product IDs, causing this driver to bind to it. For uhid
devices, the parent is the uhid character device, not a USB interface.

Would this blind cast lead to type confusion, memory corruption, and
out-of-bounds reads when attempting to access the USB device descriptor?
Should there be a hid_is_usb(hdev) check before treating the parent as a
USB interface?

>  	{ 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/20260612124830.984592-1-denis.benato@linux.dev?part=1