* Re: [PATCH] HID: intel-ish-hid: ignore post-init ENUM_DEVICES from firmware
2026-06-27 22:47 [PATCH] HID: intel-ish-hid: ignore post-init ENUM_DEVICES from firmware Wahid7852
@ 2026-06-27 22:57 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-06-27 22:57 UTC (permalink / raw)
To: Wahid7852; +Cc: linux-input, dmitry.torokhov
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
^ permalink raw reply [flat|nested] 2+ messages in thread