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

[sashiko-bot@kernel.org]

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