Re: [PATCH v2 2/4] HID: intel-ish-hid: bound report_list iteration to message size

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Outer loop variables used for message parsing are clobbered inside the inner sub-report processing loop, permanently desynchronizing the outer ISHTP message parser.
- [High] Processing duplicate initialization responses (e.g., HOSTIF_GET_HID_DESCRIPTOR) before `init_done` is set leads to a heap buffer overflow.
- [High] In the HOSTIF_DM_ENUM_DEVICES case, the driver suffers from out-of-bounds reads because it improperly uses `data_len` instead of `payload_len` for loop boundaries.
--

--- Patch [2]: [PATCH v2 2/4] HID: intel-ish-hid: bound report_list iteration to message size ---
commit d383335a5cf9c011bbef2b75bc812227c6cc17cc
Author: Shen Yongchao <grayhat@foxmail.com>

HID: intel-ish-hid: bound report_list iteration to message size

The commit introduces bounds checking in the ISH HID client's report parsing
loop. It ensures the iteration does not proceed past the message buffer by
calculating a list_end pointer and verifying the next report falls within the
valid payload.

> diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> index ba52e185ce051..6609130ce63e1 100644
> --- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> +++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> @@ -75,6 +75,7 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
>  	struct report_list *reports_list;
>  	struct report *report;
>  	u8 *pos;
> +	u8 *list_end;
>  	size_t report_len;
>  	struct ishtp_cl_data *client_data = ishtp_get_client_data(hid_ishtp_cl);
>  	int curr_hid_dev = client_data->cur_hid_dev;

[ ... ]

> @@ -282,8 +283,12 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
>  			report_type = HID_INPUT_REPORT;
>  			reports_list = (struct report_list *)payload;
>  			pos = (u8 *)reports_list->reports;
> +			list_end = (u8 *)payload + payload_len;
>  
>  			for (j = 0; j < reports_list->num_of_reports; j++) {
> +				if (pos + sizeof(struct report) > list_end)
> +					break;
> +
>  				report = (struct report *)pos;
>  				recv_msg = container_of(&report->msg,
>  							struct hostif_msg, hdr);

[Severity: High]
This is a pre-existing issue, but could we trigger a heap buffer overflow in
HOSTIF_GET_HID_DESCRIPTOR?

drivers/hid/intel-ish-hid/ishtp-hid-client.c:process_recv() {
    ...
		case HOSTIF_GET_HID_DESCRIPTOR:
    ...
			if (!client_data->hid_descr[curr_hid_dev])
				client_data->hid_descr[curr_hid_dev] =
				devm_kmalloc(cl_data_to_dev(client_data),
					     payload_len, GFP_KERNEL);
			if (client_data->hid_descr[curr_hid_dev]) {
				memcpy(client_data->hid_descr[curr_hid_dev],
				       payload, payload_len);
    ...
}

If the firmware sends two HOSTIF_GET_HID_DESCRIPTOR responses back-to-back
before initialization finishes, the first response allocates the buffer with its
payload_len.

If the second response has a larger payload_len, it skips reallocation since
the pointer is no longer NULL, but then executes the memcpy() with the new
larger payload_len.

Can this overwrite the previously allocated buffer?


[Severity: High]
This is also a pre-existing issue, but I noticed a potential out-of-bounds read
in the HOSTIF_DM_ENUM_DEVICES case:

drivers/hid/intel-ish-hid/ishtp-hid-client.c:process_recv() {
    ...
		case HOSTIF_DM_ENUM_DEVICES:
    ...
			for (i = 0; i < client_data->hid_dev_count; ++i) {
    ...
				if (1 + sizeof(struct device_info) * i >=
						data_len)
					break;

				dev_info = (struct device_info *)(payload + 1 +
					sizeof(struct device_info) * i);
				if (client_data->hid_devices)
					memcpy(client_data->hid_devices + i,
					       dev_info,
					       sizeof(struct device_info));
			}
    ...
}

Does the check `1 + sizeof(struct device_info) * i >= data_len` use the wrong
boundary?

Since data_len is the total size of the receive buffer, but the payload is
offset by cur_pos and the message header size, it seems this check could allow
reading past the end of the receive buffer if payload_len is small but
hid_dev_count is large.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730115600.218172-1-grayhat@foxmail.com?part=2