Re: [PATCH] HID: intel-ish-hid: ignore post-init ENUM_DEVICES from firmware

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Duplicate firmware responses during initialization (ENUM_DEVICES, GET_HID_DESCRIPTOR, GET_REPORT_DESCRIPTOR) cause heap buffer overflows.
- [High] Missing payload length validation and flawed bounds checks in HOSTIF_DM_ENUM_DEVICES parsing lead to out-of-bounds memory reads.
- [High] Unscaled pointer arithmetic and integer underflow in HOSTIF_PUBLISH_INPUT_REPORT_LIST lead to massive out-of-bounds memory accesses.
--

commit 44b4c0eea0f41216fd9e39bd67a4356eeadc37c7
Author: Wahid <wahidzk0091@gmail.com>

HID: intel-ish-hid: ignore post-init ENUM_DEVICES from firmware

This commit splits the guard condition for HOSTIF_DM_ENUM_DEVICES messages
to safely discard valid ENUM_DEVICES messages that arrive after
initialization is complete. This prevents unnecessary ISH soft-resets
caused by periodic firmware announcements.

> diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> --- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> +++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> @@ -113,8 +113,7 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
>  
>  		switch (recv_msg->hdr.command & CMD_MASK) {
>  		case HOSTIF_DM_ENUM_DEVICES:
> -			if ((!(recv_msg->hdr.command & ~CMD_MASK) ||
> -					client_data->init_done)) {
> +			if (!(recv_msg->hdr.command & ~CMD_MASK)) {
>  				++client_data->bad_recv_cnt;
>  				report_bad_packet(hid_ishtp_cl, recv_msg,
>  						  cur_pos,
> @@ -122,6 +121,8 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
>  				ish_hw_reset(ishtp_get_ishtp_device(hid_ishtp_cl));
>  				break;
>  			}
> +			if (client_data->init_done)
> +				break;
>  			client_data->hid_dev_count = (unsigned int)*payload;

[Severity: High]
This is a pre-existing issue, but does this code read out-of-bounds memory
if the payload length is zero?

If a HOSTIF_DM_ENUM_DEVICES message is received with a zero-length payload,
*payload is dereferenced here without validation.

Furthermore, in the loop that follows:

process_recv() {
    ...
    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));
    ...
}

The bounds check compares the relative payload offset against the absolute
buffer size (data_len) instead of the payload size. This allows the offset
to read out-of-bounds memory, which is then copied into the persistent
device information cache, leaking the contents.

>  			if (!client_data->hid_devices)
>  				client_data->hid_devices = devm_kcalloc(

[Severity: Critical]
This is a pre-existing issue, but can duplicate firmware responses cause a
heap buffer overflow here?

If multiple HOSTIF_DM_ENUM_DEVICES or GET_HID_DESCRIPTOR messages arrive
back-to-back before init_done becomes true, this allocation is incorrectly
skipped because the pointer is already non-NULL.

The memory is then copied into the existing buffer based on a potentially
larger, unvalidated hid_dev_count or payload_len:

process_recv() {
    ...
    memcpy(client_data->hid_devices + i, dev_info,
           sizeof(struct device_info));
    ...
    memcpy(client_data->hid_descr[curr_hid_dev], payload, payload_len);
    ...
}

This could cause memory to be copied into the original buffer without
reallocation, causing a heap buffer overflow.

[ ... ]

[Severity: High]
This is a pre-existing issue, but is there an integer underflow and pointer
arithmetic issue when parsing HOSTIF_PUBLISH_INPUT_REPORT_LIST?

Looking further down in process_recv():

process_recv() {
    ...
    case HOSTIF_PUBLISH_INPUT_REPORT_LIST:
        ...
        payload_len = report_len - sizeof(struct hostif_msg_hdr);
        ...
        report += sizeof(*report) + payload_len;
    ...
}

If the firmware-controlled report_len is less than 6
(sizeof(struct hostif_msg_hdr)), payload_len underflows.

Additionally, because report is a typed pointer (struct report *), this
addition is implicitly scaled by sizeof(struct report) (8 bytes), advancing
the pointer out of bounds by a factor of 8.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260627224753.35075-1-wahidzk0091@gmail.com?part=1