Re: [PATCH v2 4/4] HID: intel-ish-hid: add full entry bounds check to report_list parsing

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing check against MAX_HID_DEVICES allows out-of-bounds array indexing in client_data.
- [High] Reusing previously allocated heap memory without size checks on redundant initialization responses causes heap buffer overflows.
- [High] Flawed bounds checking in HOSTIF_DM_ENUM_DEVICES allows out-of-bounds reads across messages and beyond the receive buffer.
- [High] Missing cancellation of workqueues during device removal leads to Use-After-Free.
- [Medium] Missing minimum payload length checks allow reading past the end of the packet buffer.
--

--- Patch [4]: [PATCH v2 4/4] HID: intel-ish-hid: add full entry bounds check to report_list parsing ---
commit ac6e6de794611e6aba0e28b1be2df9e2337fcb9e
Author: Shen Yongchao <grayhat@foxmail.com>

HID: intel-ish-hid: add full entry bounds check to report_list parsing

This commit adds a bounds check to ensure the full entry (report header plus
payload) fits within the message before processing it. It also updates the
report list and list end pointers to use the current message payload instead
of a stale variable.

> diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> index efe5c5326de3b..0ac7be0c4adf5 100644
> --- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> +++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> @@ -282,9 +282,9 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
>  
>  		case HOSTIF_PUBLISH_INPUT_REPORT_LIST:
>  			report_type = HID_INPUT_REPORT;
> -			reports_list = (struct report_list *)payload;
> +			reports_list = (struct report_list *)recv_msg->payload;
>  			pos = (u8 *)reports_list->reports;
> -			list_end = (u8 *)payload + payload_len;
> +			list_end = (u8 *)recv_msg->payload + payload_len;
>  
>  			for (j = 0; j < reports_list->num_of_reports; j++) {

[Severity: Medium]
This isn't a bug introduced by this patch, but does this code read past the end 
of the packet buffer if the payload is truncated?

If the firmware sends a zero-length or truncated payload for
HOSTIF_PUBLISH_INPUT_REPORT_LIST, we access reports_list->num_of_reports
without verifying payload_len is at least sizeof(struct report_list).

I noticed a similar lack of minimum payload checks earlier in this function
when accessing *payload:

drivers/hid/intel-ish-hid/ishtp-hid-client.c:process_recv() {
    ...
    client_data->hid_dev_count = (unsigned int)*payload;
    ...
}

Can these accesses read from adjacent out-of-bounds memory when the message
sits exactly at the end of the receive buffer?

[ ... ]

[Severity: High]
This is a pre-existing issue, but I noticed a potential out-of-bounds array
access related to hid_dev_count when reviewing the rest of process_recv():

drivers/hid/intel-ish-hid/ishtp-hid-client.c:process_recv() {
    ...
    client_data->hid_dev_count = (unsigned int)*payload;
    ...
}

and later in ishtp_enum_enum_devices():

drivers/hid/intel-ish-hid/ishtp-hid-client.c:ishtp_enum_enum_devices() {
    ...
    client_data->num_hid_devices = client_data->hid_dev_count;
    ...
}

Could a firmware response provide a hid_dev_count up to 255, which is then
assigned to client_data->num_hid_devices without being bounded to
MAX_HID_DEVICES?

When this value is used as an index:

drivers/hid/intel-ish-hid/ishtp-hid-client.c:process_recv() {
    ...
    if (!client_data->hid_descr[curr_hid_dev])
    ...
}

Does this code allow out-of-bounds reads and writes to the hid_descr array?

[Severity: High]
This isn't a bug introduced by this patch, but does this memory allocation
pattern allow a heap buffer overflow if redundant initialization responses
are received?

drivers/hid/intel-ish-hid/ishtp-hid-client.c:process_recv() {
    ...
    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 multiple HOSTIF_GET_HID_DESCRIPTOR responses arrive, the first one
allocates the buffer based on the initial payload_len. On subsequent responses,
the allocation check fails and skips reallocation, but the memcpy uses the new,
potentially much larger payload_len.

Can this corrupt adjacent heap memory?

[Severity: High]
This is a pre-existing issue, but the bounds check for device enumeration
seems to compare against the total buffer size incorrectly:

drivers/hid/intel-ish-hid/ishtp-hid-client.c:process_recv() {
    ...
    if (1 + sizeof(struct device_info) * i >= data_len)
        break;
    ...
    memcpy(client_data->hid_devices + i,
           dev_info,
           sizeof(struct device_info));
    ...
}

Since payload is an offset within the recv_buf (equal to
recv_buf + cur_pos + sizeof(header)), does this check allow the actual
accessed address to extend past the end of recv_buf by up to cur_pos +
sizeof(header) bytes?

Would this result in copying out-of-bounds heap memory into the newly
allocated hid_devices array?

[Severity: High]
This isn't a bug introduced by this patch, but I have a question about the
device removal lifecycle:

drivers/hid/intel-ish-hid/ishtp-hid-client.c:hid_ishtp_cl_remove() {
    ...
}

If hid_ishtp_cl_remove() returns without calling cancel_work_sync(), devres
will free the client_data memory.

If a background work item (like client_data->work or client_data->resume_work)
is still pending or executing, will it dereference the freed client_data
structure and cause a use-after-free regression?

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