Re: [PATCH v3 3/8] HID: asus: add support for xgm led

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing led_classdev_unregister() in the probe error path leads to a use-after-free.
- [Medium] An error during keyboard backlight resume causes the XGM LED resume to be skipped.
- [Medium] Returning a positive short transfer length from brightness_set_blocking instead of a negative error code.
--

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

HID: asus: add support for xgm led

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.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 52d755659d951..95e7dc6a447d8 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: Medium]
If a short transfer occurs, asus_kbd_set_report() will return a positive
number less than ROG_XGM_REPORT_SIZE. 

Will returning this positive value cause the LED subsystem API to incorrectly
interpret the short transfer as a success instead of a failure?

> +	}
> +
> +	return 0;
> +}
> +
[ ... ]
> @@ -1178,6 +1248,14 @@ static int __maybe_unused asus_resume(struct hid_device *hdev)
>  		}
>  	}
>  

[Severity: Medium]
If restoring the keyboard backlight fails earlier in asus_resume(), the
early return via goto asus_resume_err causes this XGM LED resume logic
to be skipped.

Should the device resume function try to resume as many independent
components as possible instead of unconditionally aborting here?

> +	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;
>  }
[ ... ]
> @@ -1304,6 +1382,16 @@ 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))) {
> +		ret = asus_xgm_init(hdev, drvdata);

[Severity: High]
If asus_probe() encounters an error later on (such as asus_start_multitouch()
failing), the error handling path does not unregister the LED class device.

Because drvdata->xgm_led was allocated with devm_kzalloc(), devres will
free it. Does this lead to a use-after-free if userspace interacts with the
sysfs interface of the leaked cdev structure?

Would using devm_led_classdev_register() in asus_xgm_init() instead of
led_classdev_register() cleanly avoid this issue?

> +		if (ret) {
> +			hid_err(hdev, "Failed to initialize xg mobile: %d\n", ret);
> +			goto err_stop_hw;
> +		}
> +	}
> +

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