Re: [PATCH v12 01/17] HID: asus: size feature reports from the HID report descriptor

[Denis Benato <denis.benato@linux.dev>]

On 10/31/26 19:04, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] HID: asus: wrong report ID in mcu_request_version() breaks MCU check
> --
>
> commit 95aa403a07769e0a4dbd2dee41abe2766cd2fe36
> Author: Denis Benato <denis.benato@linux.dev>
>
> HID: asus: size feature reports from the HID report descriptor
>
> This commit derives feature report transfer lengths from the HID report
> descriptor via asus_report_len() instead of using hardcoded sizes across
> asus_kbd_set_report(), asus_kbd_get_report(), mcu_request_version(), and
> asus_xgm_led_set().
>
>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>> index f6aa1be09ad53..327bb4ab1a0d6 100644
>> --- a/drivers/hid/hid-asus.c
>> +++ b/drivers/hid/hid-asus.c
> [ ... ]
>> @@ -900,10 +992,15 @@ static int mcu_parse_version_string(const u8 *response, size_t response_size)
>>  
>>  static int mcu_request_version(struct hid_device *hdev)
>>  {
>> -	u8 *response __free(kfree) = kzalloc(ROG_ALLY_REPORT_SIZE, GFP_KERNEL);
>> -	const u8 request[] = { 0x5a, 0x05, 0x03, 0x31, 0x00, 0x20 };
>> +	size_t hw_len = asus_report_len(hdev, FEATURE_REPORT_ID);
>> +	const u8 request[] = { FEATURE_KBD_REPORT_ID, 0x05, 0x03, 0x31, 0x00, 0x20 };
>>  	int ret;
>>  
>> +	if (!hw_len)
>> +		return -EINVAL;
>> +
>> +	u8 *response __free(kfree) = kzalloc(hw_len, GFP_KERNEL);
>> +
>>  	if (!response)
>>  		return -ENOMEM;
>>  
>> @@ -911,17 +1008,16 @@ static int mcu_request_version(struct hid_device *hdev)
>>  	if (ret < 0)
>>  		return ret;
>>  
>> -	ret = hid_hw_raw_request(hdev, FEATURE_REPORT_ID, response,
>> -				ROG_ALLY_REPORT_SIZE, HID_FEATURE_REPORT,
>> -				HID_REQ_GET_REPORT);
>> +	ret = hid_hw_raw_request(hdev, FEATURE_REPORT_ID, response, hw_len,
>> +				 HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
>>  	if (ret < 0)
>>  		return ret;
> [Severity: High]
> Does asus_report_len(hdev, FEATURE_REPORT_ID) return 0 here on ROG Ally and
> ROG Ally X devices?
>
> In drivers/hid/hid-asus.c, FEATURE_REPORT_ID (0x0d) is the I2C touchpad
> multitouch report ID used by asus_start_multitouch(), whereas the ROG Ally
> N-Key vendor HID interface declares feature report 0x5a
> (FEATURE_KBD_REPORT_ID) in its HID report descriptor (and as documented above
> mcu_parse_version_string(), the MCU response payload starts with "5a 05 03 31
> 00 1a 13").
>
> Previously, mcu_request_version() passed a hardcoded ROG_ALLY_REPORT_SIZE (64)
> to hid_hw_raw_request(), which succeeded because usbhid_get_raw_report() does
> not look up the report ID in hdev->report_enum[HID_FEATURE_REPORT].
>
> With this change, when probing USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY or
> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY_X via:
>
>   asus_probe()
>     -> asus_kbd_register_leds()
>       -> validate_mcu_fw_version()
>         -> mcu_request_version()
>
> looking up FEATURE_REPORT_ID (0x0d) in hdev->report_enum[HID_FEATURE_REPORT]
> via asus_report_len() will not find a report and will return 0, causing
> mcu_request_version() to return -EINVAL. validate_mcu_fw_version() then
> silently aborts without checking the MCU version or configuring MCU powersave:
>
>   version = mcu_request_version(hdev);
>   if (version < 0)
>           return;
>
> Should mcu_request_version() look up FEATURE_KBD_REPORT_ID instead of
> FEATURE_REPORT_ID when sizing the buffer with asus_report_len()?
No, this was reported by my sashiko-bot too and attempting to follow the suggestion
has lead to driver failing and I had to use what's in there now.