[PATCH v2 0/4] HID: asus: security fixes and more hardware support

[PATCH v2 0/4] HID: asus: security fixes and more hardware support

[Denis Benato]

Hi all,

I have added support for controlling the (way too bright) XG mobile
LEDs in hid-asus and added the i2c version of already supported
hardware that was probed only when it's a USB.

Auto-review bot has spotted two pre-existing possible security problems
alongside problems in my own code therefore I am producing a V2 including
those fixes and improvements.

Link: https://lore.kernel.org/all/20260612124830.984592-1-denis.benato@linux.dev/
Link: https://lore.kernel.org/all/30258f28-4036-4e99-9494-031212544dc2@linux.dev/

Best regards,
Denis Benato

Changelog:
  - v1
    - Initial submission (as separate patches)
  - v2
    - HID: asus: add i2c entry for FA808UM and other TUFs
      - Joined to the patchset
    - HID: asus: add support for xgm led
      - Use the correct callback set_brightness_blocking
      - Stop hardcoding the cled name
    - HID: asus: prevent wrong pointer cast
      - Added
    - HID: asus: mitigate possible use-after-free
      - Added

Denis Benato (4):
  HID: asus: mitigate possible use-after-free
  HID: asus: prevent wrong pointer cast
  HID: asus: add support for xgm led
  HID: asus: add i2c entry for FA808UM and other TUFs

 drivers/hid/hid-asus.c | 76 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 75 insertions(+), 1 deletion(-)

-- 
2.47.3

[PATCH v2 1/4] HID: asus: mitigate possible use-after-free

[Denis Benato]

The kbd_backlight pointer is not being set to NULL after being freed in
asus_kbd_register_leds and other portions of the driver checks that field,
therefore set the pointer to NULL after being freed.

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

Suggested-by: sashiko-bot@kernel.org
Signed-off-by: Denis Benato <denis.benato@linux.dev>
---
 drivers/hid/hid-asus.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index d34d74df3dc0..4d7110b892bd 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -777,6 +777,7 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
 	if (ret < 0) {
 		/* No need to have this still around */
 		devm_kfree(&hdev->dev, drvdata->kbd_backlight);
+		drvdata->kbd_backlight = NULL;
 	}
 
 	return ret;
-- 
2.47.3

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

[Denis Benato]

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/

Fixes: 56d1b33e644c ("HID: asus: simplify RGB init sequence")
Suggested-by: sashiko-bot@kernel.org
Signed-off-by: Denis Benato <denis.benato@linux.dev>
---
 drivers/hid/hid-asus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 4d7110b892bd..323dc0b7f3ff 100644
--- 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,
-- 
2.47.3

[PATCH v2 3/4] HID: asus: add support for xgm led

[Denis Benato]

XG mobile stations have very bright leds behind the fan that can be
turned either ON or OFF: add a cled interface to allow controlling the
brightness of those red leds.

Signed-off-by: Denis Benato <denis.benato@linux.dev>
---
 drivers/hid/hid-asus.c | 70 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 323dc0b7f3ff..21c4a60d224e 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -51,6 +51,8 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
 #define FEATURE_KBD_LED_REPORT_ID1 0x5d
 #define FEATURE_KBD_LED_REPORT_ID2 0x5e
 
+#define ROG_XGM_REPORT_SIZE 300
+
 #define ROG_ALLY_REPORT_SIZE 64
 #define ROG_ALLY_X_MIN_MCU 313
 #define ROG_ALLY_MIN_MCU 319
@@ -118,6 +120,11 @@ struct asus_kbd_leds {
 	bool removed;
 };
 
+struct asus_xgm_led {
+	struct led_classdev cdev;
+	struct hid_device *hdev;
+};
+
 struct asus_touchpad_info {
 	int max_x;
 	int max_y;
@@ -143,6 +150,7 @@ struct asus_drvdata {
 	unsigned long battery_next_query;
 	struct work_struct fn_lock_sync_work;
 	bool fn_lock;
+	struct asus_xgm_led *xgm_led;
 };
 
 static int asus_report_battery(struct asus_drvdata *, u8 *, int);
@@ -941,6 +949,23 @@ static int asus_battery_probe(struct hid_device *hdev)
 	return ret;
 }
 
+static int asus_xgm_led_set(struct led_classdev *led_cdev, enum led_brightness value)
+{
+	const u8 buf[ROG_XGM_REPORT_SIZE] = {
+		FEATURE_KBD_LED_REPORT_ID2, 0xC5, (value) ? 0x50 : 0x00
+	};
+	struct asus_xgm_led *xgm = container_of(led_cdev, struct asus_xgm_led, cdev);
+	int ret;
+
+	ret = asus_kbd_set_report(xgm->hdev, buf, ROG_XGM_REPORT_SIZE);
+	if (ret != ROG_XGM_REPORT_SIZE) {
+		hid_err(xgm->hdev, "Unable to set XG mobile led state: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
 static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
 {
 	struct input_dev *input = hi->input;
@@ -1184,6 +1209,14 @@ static int __maybe_unused asus_resume(struct hid_device *hdev)
 		}
 	}
 
+	if (drvdata->xgm_led) {
+		ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
+		if (ret) {
+			hid_err(hdev, "Asus failed to restore xgm brightness: %d\n", ret);
+			goto asus_resume_err;
+		}
+	}
+
 asus_resume_err:
 	return ret;
 }
@@ -1310,6 +1343,40 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		}
 	}
 
+	if (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID) &&
+	    ((hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2022) ||
+	     (hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2023))) {
+		drvdata->xgm_led = devm_kzalloc(&hdev->dev, sizeof(*drvdata->xgm_led), GFP_KERNEL);
+		if (drvdata->xgm_led == NULL) {
+			ret = -ENOMEM;
+			goto err_stop_hw;
+		}
+		drvdata->xgm_led->hdev = hdev;
+		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;
+		drvdata->xgm_led->cdev.max_brightness = 1;
+		drvdata->xgm_led->cdev.brightness_set_blocking = asus_xgm_led_set;
+
+		/*
+		 * Sometimes after a reboot LEDs are ON, sometimes are OFF: set them to
+		 * what the default brightness resets when doing a cold boot.
+		 */
+		ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
+		if (ret) {
+			hid_err(hdev, "Asus failed to set xgm led: %d\n", ret);
+			goto err_stop_hw;
+		}
+
+		ret = led_classdev_register(&hdev->dev, &drvdata->xgm_led->cdev);
+		if (ret) {
+			hid_err(hdev, "Asus failed to register xgm led: %d\n", ret);
+			goto err_stop_hw;
+		}
+	}
+
 	/* Laptops keyboard backlight is always at 0x5a */
 	if (is_vendor && (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) &&
 	    (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID)) &&
@@ -1366,6 +1433,9 @@ static void asus_remove(struct hid_device *hdev)
 	if (drvdata->quirks & QUIRK_HID_FN_LOCK)
 		cancel_work_sync(&drvdata->fn_lock_sync_work);
 
+	if (drvdata->xgm_led)
+		led_classdev_unregister(&drvdata->xgm_led->cdev);
+
 	hid_hw_stop(hdev);
 }
 
-- 
2.47.3

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

[Denis Benato]

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.

Signed-off-by: Denis Benato <denis.benato@linux.dev>
---
 drivers/hid/hid-asus.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 21c4a60d224e..b24222fdb319 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -1571,6 +1571,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 },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
 	    USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
 	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
-- 
2.47.3

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

[sashiko-bot]

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

Re: [PATCH v2 3/4] HID: asus: add support for xgm led

[sashiko-bot]

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

New issues:
- [High] Missing NULL check for devm_kasprintf() return value causes a kernel panic on allocation failure.
- [High] Missing led_classdev_unregister() in the asus_probe() error path creates a structural Use-After-Free risk.
- [Low] Incorrect error propagation in asus_xgm_led_set() when asus_kbd_set_report() returns a short byte count or 0.

Pre-existing issues:
- [High] Missing cleanup of kbd_backlight->listener and fn_lock_sync_work on asus_probe() error path causes Use-After-Free.
--

commit a9179e65e5727586c134f2af39cf55f67620125b
Author: Denis Benato <denis.benato@linux.dev>
Subject: HID: asus: add support for xgm led

This patch adds support for controlling the brightness of the red leds behind
the fan on XG mobile stations. It introduces a cled interface via the
led_classdev to turn them on or off.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 52d755659d951..0f78242b6a1db 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -941,6 +949,23 @@ static int asus_battery_probe(struct hid_device *hdev)
>  	return ret;
>  }
>  
> +static int asus_xgm_led_set(struct led_classdev *led_cdev, enum led_brightness value)
> +{
> +	const u8 buf[ROG_XGM_REPORT_SIZE] = {
> +		FEATURE_KBD_LED_REPORT_ID2, 0xC5, (value) ? 0x50 : 0x00
> +	};
> +	struct asus_xgm_led *xgm = container_of(led_cdev, struct asus_xgm_led, cdev);
> +	int ret;
> +
> +	ret = asus_kbd_set_report(xgm->hdev, buf, ROG_XGM_REPORT_SIZE);
> +	if (ret != ROG_XGM_REPORT_SIZE) {
> +		hid_err(xgm->hdev, "Unable to set XG mobile led state: %d\n", ret);
> +		return ret;

[Severity: Low]
Could this return a positive value or 0 on failure?

asus_kbd_set_report() returns the number of bytes transferred on success or a
negative error code on failure. If a partial transfer occurs or if it returns
0, this check will evaluate to true and return the non-negative value. The
caller might silently treat a 0 return value as success, bypassing error
handling expectations.

> +	}
> +
> +	return 0;
> +}
> +
[ ... ]
> @@ -1304,6 +1337,40 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		}
>  	}
>  
> +	if (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID) &&
> +	    ((hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2022) ||
> +	     (hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2023))) {
> +		drvdata->xgm_led = devm_kzalloc(&hdev->dev, sizeof(*drvdata->xgm_led), GFP_KERNEL);
> +		if (drvdata->xgm_led == NULL) {
> +			ret = -ENOMEM;
> +			goto err_stop_hw;
> +		}
> +		drvdata->xgm_led->hdev = hdev;
> +		drvdata->xgm_led->cdev.name = devm_kasprintf(&hdev->dev, GFP_KERNEL,
> +					"asus:xgm-%s:led",
> +					strlen(hdev->uniq) ?
> +					hdev->uniq : dev_name(&hdev->dev));

[Severity: High]
Does this code need to check if devm_kasprintf() returns null? 

Under memory pressure, if this allocation fails, passing the null cdev.name to
led_classdev_register() will lead to a null pointer dereference in the led
subsystem when it uses strscpy() to copy the device name.

> +		drvdata->xgm_led->cdev.brightness = 1;
> +		drvdata->xgm_led->cdev.max_brightness = 1;
> +		drvdata->xgm_led->cdev.brightness_set_blocking = asus_xgm_led_set;
> +
> +		/*
> +		 * Sometimes after a reboot LEDs are ON, sometimes are OFF: set them to
> +		 * what the default brightness resets when doing a cold boot.
> +		 */
> +		ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
> +		if (ret) {
> +			hid_err(hdev, "Asus failed to set xgm led: %d\n", ret);
> +			goto err_stop_hw;
> +		}
> +
> +		ret = led_classdev_register(&hdev->dev, &drvdata->xgm_led->cdev);
> +		if (ret) {
> +			hid_err(hdev, "Asus failed to register xgm led: %d\n", ret);
> +			goto err_stop_hw;

[Severity: High]
If asus_probe() fails later in the probe process and jumps to err_stop_hw,
does the error path miss unregistering the led device?

Because the xgm_led structure is allocated with devm_kzalloc(), it will be
automatically freed when the probe fails. If led_classdev_unregister() isn't
called on the err_stop_hw path, the led subsystem may retain active references
to the freed cdev, potentially causing a use-after-free when sysfs led
attributes are accessed.

[Severity: High]
This isn't a bug introduced by this patch, but looking at the error path for
asus_probe() when jumping to err_stop_hw:

err_stop_hw:
	hid_hw_stop(hdev);
	return ret;

It seems we miss cleaning up kbd_backlight->listener and fn_lock_sync_work
if the probe fails downstream. Once devres cleans up drvdata, the listener
remains in the global list in asus-wmi, which might lead to a use-after-free
if a WMI event triggers the listener and accesses the freed memory.

> +		}
> +	}
> +

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

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

[sashiko-bot]

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

Re: [PATCH v2 3/4] HID: asus: add support for xgm led

[Antheas Kapenekakis]

On Fri, 12 Jun 2026 at 16:23, Denis Benato <denis.benato@linux.dev> wrote:
>
> XG mobile stations have very bright leds behind the fan that can be
> turned either ON or OFF: add a cled interface to allow controlling the
> brightness of those red leds.
>
> Signed-off-by: Denis Benato <denis.benato@linux.dev>
> ---
>  drivers/hid/hid-asus.c | 70 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
>
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 323dc0b7f3ff..21c4a60d224e 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -51,6 +51,8 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>  #define FEATURE_KBD_LED_REPORT_ID1 0x5d
>  #define FEATURE_KBD_LED_REPORT_ID2 0x5e
>
> +#define ROG_XGM_REPORT_SIZE 300
> +
>  #define ROG_ALLY_REPORT_SIZE 64
>  #define ROG_ALLY_X_MIN_MCU 313
>  #define ROG_ALLY_MIN_MCU 319
> @@ -118,6 +120,11 @@ struct asus_kbd_leds {
>         bool removed;
>  };
>
> +struct asus_xgm_led {
> +       struct led_classdev cdev;
> +       struct hid_device *hdev;
> +};
> +
>  struct asus_touchpad_info {
>         int max_x;
>         int max_y;
> @@ -143,6 +150,7 @@ struct asus_drvdata {
>         unsigned long battery_next_query;
>         struct work_struct fn_lock_sync_work;
>         bool fn_lock;
> +       struct asus_xgm_led *xgm_led;
>  };
>
>  static int asus_report_battery(struct asus_drvdata *, u8 *, int);
> @@ -941,6 +949,23 @@ static int asus_battery_probe(struct hid_device *hdev)
>         return ret;
>  }
>
> +static int asus_xgm_led_set(struct led_classdev *led_cdev, enum led_brightness value)
> +{
> +       const u8 buf[ROG_XGM_REPORT_SIZE] = {
> +               FEATURE_KBD_LED_REPORT_ID2, 0xC5, (value) ? 0x50 : 0x00
> +       };
> +       struct asus_xgm_led *xgm = container_of(led_cdev, struct asus_xgm_led, cdev);
> +       int ret;
> +
> +       ret = asus_kbd_set_report(xgm->hdev, buf, ROG_XGM_REPORT_SIZE);
> +       if (ret != ROG_XGM_REPORT_SIZE) {
> +               hid_err(xgm->hdev, "Unable to set XG mobile led state: %d\n", ret);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
>  static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
>  {
>         struct input_dev *input = hi->input;
> @@ -1184,6 +1209,14 @@ static int __maybe_unused asus_resume(struct hid_device *hdev)
>                 }
>         }
>
> +       if (drvdata->xgm_led) {
> +               ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
> +               if (ret) {
> +                       hid_err(hdev, "Asus failed to restore xgm brightness: %d\n", ret);
> +                       goto asus_resume_err;
> +               }
> +       }
> +
>  asus_resume_err:
>         return ret;
>  }
> @@ -1310,6 +1343,40 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>                 }
>         }
>
> +       if (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID) &&
> +           ((hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2022) ||
> +            (hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2023))) {
> +               drvdata->xgm_led = devm_kzalloc(&hdev->dev, sizeof(*drvdata->xgm_led), GFP_KERNEL);
> +               if (drvdata->xgm_led == NULL) {
> +                       ret = -ENOMEM;
> +                       goto err_stop_hw;
> +               }
> +               drvdata->xgm_led->hdev = hdev;
> +               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;
> +               drvdata->xgm_led->cdev.max_brightness = 1;
> +               drvdata->xgm_led->cdev.brightness_set_blocking = asus_xgm_led_set;
> +
> +               /*
> +                * Sometimes after a reboot LEDs are ON, sometimes are OFF: set them to
> +                * what the default brightness resets when doing a cold boot.
> +                */

I think this is set by the driver, so you should reformat the comment
above, so you should trim the comment.

Perhaps, "The LED state is arbitrary on boot, therefore default to the
initial brightness set above". This way it does not become outdated if
cdev.brightness changes.

> +               ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
> +               if (ret) {
> +                       hid_err(hdev, "Asus failed to set xgm led: %d\n", ret);
> +                       goto err_stop_hw;
> +               }

You already do this in asus_input_configured so you do it twice?
Perhaps skip one if you end up keeping them? I think that it's better
to keep this block.

Or even better return an error in _get so that on boot it is
ambiguous? I assume the leds remain to the state they had prior to the
reboot? With this change, imagine a user that turned off the leds in
windows, permabooted into Linux, and now has the lights always turn on
during boot.

Moreover, can systemd restore this or is it out of scope for its led
handler? Perhaps it is an ambitious idea though, and better skipped.

> +
> +               ret = led_classdev_register(&hdev->dev, &drvdata->xgm_led->cdev);
> +               if (ret) {
> +                       hid_err(hdev, "Asus failed to register xgm led: %d\n", ret);
> +                       goto err_stop_hw;
> +               }
> +       }
> +
>         /* Laptops keyboard backlight is always at 0x5a */
>         if (is_vendor && (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) &&
>             (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID)) &&
> @@ -1366,6 +1433,9 @@ static void asus_remove(struct hid_device *hdev)
>         if (drvdata->quirks & QUIRK_HID_FN_LOCK)
>                 cancel_work_sync(&drvdata->fn_lock_sync_work);
>
> +       if (drvdata->xgm_led)
> +               led_classdev_unregister(&drvdata->xgm_led->cdev);
> +
>         hid_hw_stop(hdev);
>  }
>
> --
> 2.47.3
>
>

Re: [PATCH v2 1/4] HID: asus: mitigate possible use-after-free

[Antheas Kapenekakis]

On Fri, 12 Jun 2026 at 16:23, Denis Benato <denis.benato@linux.dev> wrote:
>
> The kbd_backlight pointer is not being set to NULL after being freed in
> asus_kbd_register_leds and other portions of the driver checks that field,
> therefore set the pointer to NULL after being freed.
>
> Link: https://lore.kernel.org/all/20260612130125.AAAE71F000E9@smtp.kernel.org/
>
> Suggested-by: sashiko-bot@kernel.org

Good catch. I verified devm_kfree does not mutate the pointer. I
recall this being there before from the comment.

Reviewed-by: Antheas Kapenekakis <lkml@antheas.dev>

> Signed-off-by: Denis Benato <denis.benato@linux.dev>
> ---
>  drivers/hid/hid-asus.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index d34d74df3dc0..4d7110b892bd 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -777,6 +777,7 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
>         if (ret < 0) {
>                 /* No need to have this still around */
>                 devm_kfree(&hdev->dev, drvdata->kbd_backlight);
> +               drvdata->kbd_backlight = NULL;
>         }
>
>         return ret;
> --
> 2.47.3
>
>

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

[Antheas Kapenekakis]

On Fri, 12 Jun 2026 at 16:23, Denis Benato <denis.benato@linux.dev> wrote:
>
> 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/
>
> Fixes: 56d1b33e644c ("HID: asus: simplify RGB init sequence")

Proper fixes is:
Fixes: 00e005c952f7 ("hid-asus: check ROG Ally MCU version and warn")

56d1b33e644c just moved the block.

> Suggested-by: sashiko-bot@kernel.org
> Signed-off-by: Denis Benato <denis.benato@linux.dev>
> ---
>  drivers/hid/hid-asus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 4d7110b892bd..323dc0b7f3ff 100644
> --- 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,
> --
> 2.47.3
>
>

Re: [PATCH v2 3/4] HID: asus: add support for xgm led

[Denis Benato]

On 6/12/26 16:39, Antheas Kapenekakis wrote:
> On Fri, 12 Jun 2026 at 16:23, Denis Benato <denis.benato@linux.dev> wrote:
>> XG mobile stations have very bright leds behind the fan that can be
>> turned either ON or OFF: add a cled interface to allow controlling the
>> brightness of those red leds.
>>
>> Signed-off-by: Denis Benato <denis.benato@linux.dev>
>> ---
>>  drivers/hid/hid-asus.c | 70 ++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 70 insertions(+)
>>
>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>> index 323dc0b7f3ff..21c4a60d224e 100644
>> --- a/drivers/hid/hid-asus.c
>> +++ b/drivers/hid/hid-asus.c
>> @@ -51,6 +51,8 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>>  #define FEATURE_KBD_LED_REPORT_ID1 0x5d
>>  #define FEATURE_KBD_LED_REPORT_ID2 0x5e
>>
>> +#define ROG_XGM_REPORT_SIZE 300
>> +
>>  #define ROG_ALLY_REPORT_SIZE 64
>>  #define ROG_ALLY_X_MIN_MCU 313
>>  #define ROG_ALLY_MIN_MCU 319
>> @@ -118,6 +120,11 @@ struct asus_kbd_leds {
>>         bool removed;
>>  };
>>
>> +struct asus_xgm_led {
>> +       struct led_classdev cdev;
>> +       struct hid_device *hdev;
>> +};
>> +
>>  struct asus_touchpad_info {
>>         int max_x;
>>         int max_y;
>> @@ -143,6 +150,7 @@ struct asus_drvdata {
>>         unsigned long battery_next_query;
>>         struct work_struct fn_lock_sync_work;
>>         bool fn_lock;
>> +       struct asus_xgm_led *xgm_led;
>>  };
>>
>>  static int asus_report_battery(struct asus_drvdata *, u8 *, int);
>> @@ -941,6 +949,23 @@ static int asus_battery_probe(struct hid_device *hdev)
>>         return ret;
>>  }
>>
>> +static int asus_xgm_led_set(struct led_classdev *led_cdev, enum led_brightness value)
>> +{
>> +       const u8 buf[ROG_XGM_REPORT_SIZE] = {
>> +               FEATURE_KBD_LED_REPORT_ID2, 0xC5, (value) ? 0x50 : 0x00
>> +       };
>> +       struct asus_xgm_led *xgm = container_of(led_cdev, struct asus_xgm_led, cdev);
>> +       int ret;
>> +
>> +       ret = asus_kbd_set_report(xgm->hdev, buf, ROG_XGM_REPORT_SIZE);
>> +       if (ret != ROG_XGM_REPORT_SIZE) {
>> +               hid_err(xgm->hdev, "Unable to set XG mobile led state: %d\n", ret);
>> +               return ret;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>  static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
>>  {
>>         struct input_dev *input = hi->input;
>> @@ -1184,6 +1209,14 @@ static int __maybe_unused asus_resume(struct hid_device *hdev)
>>                 }
>>         }
>>
>> +       if (drvdata->xgm_led) {
>> +               ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
>> +               if (ret) {
>> +                       hid_err(hdev, "Asus failed to restore xgm brightness: %d\n", ret);
>> +                       goto asus_resume_err;
>> +               }
>> +       }
>> +
>>  asus_resume_err:
>>         return ret;
>>  }
>> @@ -1310,6 +1343,40 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>                 }
>>         }
>>
>> +       if (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID) &&
>> +           ((hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2022) ||
>> +            (hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2023))) {
>> +               drvdata->xgm_led = devm_kzalloc(&hdev->dev, sizeof(*drvdata->xgm_led), GFP_KERNEL);
>> +               if (drvdata->xgm_led == NULL) {
>> +                       ret = -ENOMEM;
>> +                       goto err_stop_hw;
>> +               }
>> +               drvdata->xgm_led->hdev = hdev;
>> +               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;
>> +               drvdata->xgm_led->cdev.max_brightness = 1;
>> +               drvdata->xgm_led->cdev.brightness_set_blocking = asus_xgm_led_set;
>> +
>> +               /*
>> +                * Sometimes after a reboot LEDs are ON, sometimes are OFF: set them to
>> +                * what the default brightness resets when doing a cold boot.
>> +                */
> I think this is set by the driver, so you should reformat the comment
> above, so you should trim the comment.
>
> Perhaps, "The LED state is arbitrary on boot, therefore default to the
> initial brightness set above". This way it does not become outdated if
> cdev.brightness changes.
yeah better spelling. I agree.
>> +               ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
>> +               if (ret) {
>> +                       hid_err(hdev, "Asus failed to set xgm led: %d\n", ret);
>> +                       goto err_stop_hw;
>> +               }
> You already do this in asus_input_configured so you do it twice?
> Perhaps skip one if you end up keeping them? I think that it's better
> to keep this block.
In asus_input_configured? Will take a look in the next days. I tought
the other was in asus_resume since at resume they resets back...
> Or even better return an error in _get so that on boot it is
> ambiguous? I assume the leds remain to the state they had prior to the
> reboot? With this change, imagine a user that turned off the leds in
> windows, permabooted into Linux, and now has the lights always turn on
> during boot.
Cold boot sets them to ON, while rebooting keep them at what they were.

After exiting from sleep they are always ON, but this is on an ally,
I don't know if on an old rog flow it's the same.
> Moreover, can systemd restore this or is it out of scope for its led
> handler? Perhaps it is an ambitious idea though, and better skipped.
I don't see realistic for this to fail if it was successful at probe so it
shouldn't matter. As for systemd restoring them it would have to
be informed that they changed (but there is no read-back) so either
way something has to happen at resume, but doing this means no
additional software is necessary and user preference is being
respected regardless of anything else.
>> +
>> +               ret = led_classdev_register(&hdev->dev, &drvdata->xgm_led->cdev);
>> +               if (ret) {
>> +                       hid_err(hdev, "Asus failed to register xgm led: %d\n", ret);
>> +                       goto err_stop_hw;
>> +               }
>> +       }
>> +
>>         /* Laptops keyboard backlight is always at 0x5a */
>>         if (is_vendor && (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) &&
>>             (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID)) &&
>> @@ -1366,6 +1433,9 @@ static void asus_remove(struct hid_device *hdev)
>>         if (drvdata->quirks & QUIRK_HID_FN_LOCK)
>>                 cancel_work_sync(&drvdata->fn_lock_sync_work);
>>
>> +       if (drvdata->xgm_led)
>> +               led_classdev_unregister(&drvdata->xgm_led->cdev);
>> +
>>         hid_hw_stop(hdev);
>>  }
>>
>> --
>> 2.47.3
>>
>>
Thanks,
Denis