Re: [PATCH v11 15/18] HID: asus: size ally gamepad commands from the HID report descriptor

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] HID: asus: ally_dev_get_report() sizes 0x0D read from 0x5A report length
--

commit 10b46a2b16e439d019a0b1a24882abca2924c954
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: size ally gamepad commands from the HID report descriptor

This commit replaces the hardcoded ROG_ALLY_REPORT_SIZE constant with dynamic
buffer sizing derived from the HID report descriptor via asus_report_len() for
ROG Ally gamepad commands and MCU version requests.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index f438cad13264a..e444cad85b32c 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -69,7 +69,6 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>  #define ASUS_XGM_QUERY_VERSION 0x03
>  #define ASUS_XGM_INFO_LEN 32
>  
> -#define ROG_ALLY_REPORT_SIZE 64
>  #define ROG_ALLY_X_MIN_MCU 313
>  #define ROG_ALLY_MIN_MCU 319
>  
> @@ -943,28 +942,33 @@ static int ally_gamepad_send_receive_packet(struct ally_handheld *ally,
>  
>  /**
>   * ally_alloc_cmd() - Construct a command buffer for the gamepad
> + * @hdev: HID device the command is built for
>   * @cmd: Command code to send
>   * @payload: Optional payload data to include in the command
>   * @payload_size: Size of the payload data
> + * @size: Where to store the length of the allocated buffer
>   *
> - * The constructed buffer is 64 bytes long, and it is the caller
> - * responsibility to free the buffer using kfree().
> + * The buffer is sized after the feature report the device declares for
> + * the gamepad commands, and it is the caller responsibility to free the
> + * buffer using kfree().
>   *
>   * Return: the newly allocated buffer containing the command, or NULL on
>   * allocation failure or if the payload would not fit in the report
>   */
> -static u8 *ally_alloc_cmd(u8 cmd, const u8 *payload, u8 payload_size)
> +static u8 *ally_alloc_cmd(struct hid_device *hdev, u8 cmd, const u8 *payload,
> +			  u8 payload_size, size_t *size)
>  {
> +	size_t hw_len = asus_report_len(hdev, HID_ALLY_SET_REPORT_ID);
>  	u8 *hidbuf;
>  
>  	/*
>  	 * The payload is written after the 4-byte command header:
>  	 * reject one that would spill past the end of the report.
>  	 */
> -	if (payload_size > ROG_ALLY_REPORT_SIZE - 4)
> +	if (hw_len < 4 || payload_size > hw_len - 4)
>  		return NULL;
>  
> -	hidbuf = kzalloc(ROG_ALLY_REPORT_SIZE, GFP_KERNEL);
> +	hidbuf = kzalloc(hw_len, GFP_KERNEL);
>  
>  	if (!hidbuf)
>  		return NULL;
> @@ -977,6 +981,8 @@ static u8 *ally_alloc_cmd(u8 cmd, const u8 *payload, u8 payload_size)
>  	if (payload_size > 0 && payload)
>  		memcpy(&hidbuf[4], payload, payload_size);
>  
> +	*size = hw_len;
> +
>  	return hidbuf;
>  }
>  
> @@ -992,15 +998,17 @@ static bool ally_check_capability(struct hid_device *hdev, struct ally_handheld
>  				  enum ally_command_codes check_cmd)
>  {
>  	u8 payload[] = { 0x00 };
> +	size_t buf_len;
>  	int ret;
>  
> -	u8 *buf __free(kfree) = ally_alloc_cmd(check_cmd, payload, sizeof(payload));
> +	u8 *buf __free(kfree) = ally_alloc_cmd(hdev, check_cmd, payload,
> +					       sizeof(payload), &buf_len);
>  	if (!buf) {
>  		hid_err(hdev, "Failed to allocate buffer for capability check.\n");
>  		return false;
>  	}
>  
> -	ret = ally_gamepad_send_receive_packet(ally, hdev, buf, ROG_ALLY_REPORT_SIZE);
> +	ret = ally_gamepad_send_receive_packet(ally, hdev, buf, buf_len);

[Severity: Medium]
Does ally_gamepad_send_receive_packet() end up sizing the 0x0d feature report
read using the length of the 0x5a feature report?

In ally_alloc_cmd(), buf_len is resolved from HID_ALLY_SET_REPORT_ID (0x5a).
However, when ally_check_capability() and ally_gamepad_check_ready() pass
buf_len into ally_gamepad_send_receive_packet(), the same length is passed to
both ally_dev_set_report() and ally_dev_get_report():

drivers/hid/hid-asus.c:ally_gamepad_send_receive_packet() {
	scoped_guard(mutex, &ally->intf_mutex) {
		ret = ally_dev_set_report(hdev, buf, len);
		if (ret >= 0) {
			memset(buf, 0, len);
			ret = ally_dev_get_report(hdev, buf, len);
		}
	}
}

While ally_dev_set_report() sends report 0x5a, ally_dev_get_report() reads
from HID_ALLY_GET_REPORT_ID (0x0d):

drivers/hid/hid-asus.c:ally_dev_get_report() {
	return hid_hw_raw_request(hdev, HID_ALLY_GET_REPORT_ID, out, len,
		HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
}

If a device declares different report lengths for feature reports 0x5a and
0x0d (or omits report 0x0d from its descriptor), ally_dev_get_report() will
request the wrong transfer size rather than consulting asus_report_len(hdev,
HID_ALLY_GET_REPORT_ID). Should the receive path in ally_dev_get_report() or
ally_gamepad_send_receive_packet() also check the length of
HID_ALLY_GET_REPORT_ID from the report descriptor?

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