Re: [PATCH v11 14/18] HID: asus: expose XG Mobile identity and firmware information

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] HID: asus: unchecked return and unaligned scan in asus_xgm_tlv_str()
--

commit a88274a1b2531f51181e1da6036282f6753460c3
Author: Denis Benato <benato.denis96@gmail.com>

HID: asus: expose XG Mobile identity and firmware information

This commit queries the XG Mobile station for its firmware version and
generation at probe time and exposes the station's identity and firmware
fields through read-only sysfs attributes on the HID device.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index e751bf1634349..f438cad13264a 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -6204,6 +6206,148 @@ static bool asus_xgm_is_supported_model(const struct asus_xgm_info *info)
>  	return false;
>  }
>  
> +/**
> + * asus_xgm_tlv_str() - Extract a "tag, length, data" field
> + * @dst: destination buffer
> + * @dst_len: size of @dst, including the trailing NUL
> + * @buf: answer of a version query
> + * @len: size of @buf
> + * @tag: three characters tag to look for
> + *
> + * The version query answers with an 8-byte binary blob followed by
> + * "tag, length, data" fields, where GEN holds the station generation and
> + * VER the firmware version.
> + *
> + * Return: 0 on success, -ENOENT if @tag is missing or its data does not
> + * fit @dst
> + */
> +static int asus_xgm_tlv_str(char *dst, size_t dst_len, const u8 *buf,
> +			    size_t len, const char *tag)
> +{
> +	size_t i;
> +	u8 n;
> +
> +	for (i = 3; i + 4 <= len; i++) {
> +		if (memcmp(buf + i, tag, 3))
> +			continue;
> +
> +		n = buf[i + 3];
> +		if (i + 4 + n > len || n >= dst_len)
> +			return -ENOENT;
> +
> +		memcpy(dst, buf + i + 4, n);
> +		dst[n] = '\0';
> +
> +		return 0;
> +	}
> +
> +	return -ENOENT;
> +}
> +
> +/**
> + * asus_xgm_get_version() - Query the firmware version of a station
> + * @hdev: HID device
> + * @info: identity structure whose generation and firmware_version fields
> + * get filled in
> + *
> + * Return: 0 on success, negative errno on failure
> + */
> +static int asus_xgm_get_version(struct hid_device *hdev,
> +				struct asus_xgm_info *info)
> +{
> +	size_t len = asus_report_len(hdev, FEATURE_KBD_LED_REPORT_ID2);
> +	int ret;
> +
> +	if (!len)
> +		return -EINVAL;
> +
> +	u8 *buf __free(kfree) = kzalloc(len, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = asus_xgm_query(hdev, ASUS_XGM_QUERY_VERSION, buf, len);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (len < 4 || buf[1] != ASUS_XGM_CMD_QUERY ||
> +	    buf[2] != ASUS_XGM_QUERY_VERSION)
> +		return -ENODEV;
> +
> +	asus_xgm_tlv_str(info->generation, sizeof(info->generation),
> +			 buf, len, "GEN");
> +	asus_xgm_tlv_str(info->firmware_version, sizeof(info->firmware_version),
> +			 buf, len, "VER");
> +
> +	return 0;
> +}

[Severity: Medium]
In drivers/hid/hid-asus.c, asus_xgm_tlv_str() is documented to follow an
8-byte binary blob after the 3-byte report/command header (buf[0..2]) and
return -ENOENT when a tag is missing or its length is out of bounds.

However, in the asus_probe() -> asus_xgm_init() -> asus_xgm_get_version() ->
asus_xgm_tlv_str() path, two things stand out:

1. asus_xgm_tlv_str() starts scanning at i = 3 (inside the 8-byte binary blob
at buf[3..10]) and increments byte-by-byte (i++) rather than starting at
offset 11 and advancing across record boundaries by 4 + n. If the binary blob
or an earlier field's payload happens to contain the bytes "GEN" or "VER",
could asus_xgm_tlv_str() treat the following byte as the length and either
abort early with -ENOENT or copy unintended bytes into dst?

2. asus_xgm_get_version() ignores the return values of both
asus_xgm_tlv_str() calls and unconditionally returns 0. When a tag is missing
or truncated, asus_xgm_init() will log empty strings for
info->firmware_version and info->generation via hid_info() instead of taking
the ret < 0 warning branch:

	ret = asus_xgm_get_version(hdev, info);
	if (ret < 0)
		hid_warn(hdev, "Failed to query XG Mobile version: %d\n", ret);
	else
		hid_info(hdev, "XG Mobile: firmware %s, generation %s\n",
			 info->firmware_version, info->generation);

Should asus_xgm_tlv_str() start after the 8-byte binary header (or step by
TLV record length), and should asus_xgm_get_version() check and propagate the
return values of asus_xgm_tlv_str()?

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