Re: [PATCH v2 5/7] HID: asus: Do not send feature reports larger than declared

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] hid: asus: handshake string truncated by report size clamping
--

commit fda25fdf61cb84326a7d3ef09500a6da741dd0bc
Author: Paolo Pisati <p.pisati@gmail.com>

HID: asus: Do not send feature reports larger than declared

This commit limits feature report write sizes to the length declared by the
device descriptor. This prevents control transfer timeouts on Zenbook Duo
keyboards, which declare a 15-byte feature report instead of 64 bytes.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index d8a931290c9a7..9e80a2bb54c37 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -618,6 +618,17 @@ static int asus_raw_event(struct hid_device *hdev,
>  
>  static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t buf_size)
>  {
> +	struct hid_report *report;
> +
> +	/*
> +	 * Do not send more than the declared size of the report: the
> +	 * Zenbook Duo keyboards declare report 0x5a as 15 bytes long and
> +	 * do not answer the 64 byte writes sized for the ROG keyboards.
> +	 */
> +	report = hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[buf[0]];
> +	if (report)
> +		buf_size = min_t(size_t, buf_size, hid_report_len(report));

[Severity: Medium]
Will this clamp the 16-byte initialization handshake buffer and drop its null
terminator?

The commit message notes that "the 16 byte handshake goes through" on the
Zenbook Duo. However, looking at asus_kbd_init(), it sends a 16-byte magic
string for initialization:

	const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54,
		     0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 };
	int ret;

	ret = asus_kbd_set_report(hdev, buf, sizeof(buf));

If the Zenbook Duo declares report 0x5a as 15 bytes, the min_t function will
reduce buf_size to 15. The trailing 0x00 null terminator of "ASUS Tech.Inc.\0"
will be silently truncated before the buffer is duplicated and sent to the
hardware via hid_hw_raw_request().

Could this unterminated string cause the firmware to reject the initialization
sequence?

> +
>  	u8 *dmabuf __free(kfree) = kmemdup(buf, buf_size, GFP_KERNEL);
>  	if (!dmabuf)
>  		return -ENOMEM;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009153459.124752-1-p.pisati@gmail.com?part=5