* Re: [PATCH] HID: asus: add i2c entry for FA808UM and other TUFs
2026-06-12 12:48 [PATCH] HID: asus: add i2c entry for FA808UM and other TUFs Denis Benato
@ 2026-06-12 13:01 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-06-12 13:01 UTC (permalink / raw)
To: Denis Benato; +Cc: dmitry.torokhov, linux-input
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
^ permalink raw reply [flat|nested] 2+ messages in thread