Re: [PATCH] HID: intel-ish-hid: clamp HID device count to MAX_HID_DEVICES

Re: [PATCH] HID: intel-ish-hid: clamp HID device count to MAX_HID_DEVICES

[Shen Yongchao]

Hi Greg,

> Why min_t()?  Was this generated by a LLM?  If so, you have to
> document that as well.

Yes, the patch was drafted with AI assistance; the vulnerability
analysis, source-level verification, and reproducer work were
done manually.  I should have disclosed that -- thank you for
asking.

And you are right about min_t(), it is unnecessarily obscure
here.  v2 below uses a plain if instead.

Regards,
Shen Yongchao

[PATCH v2] HID: intel-ish-hid: clamp HID device count to MAX_HID_DEVICES

[Shen Yongchao]

The HOSTIF_DM_ENUM_DEVICES response handler takes the HID device
count from the first payload byte of the ISH firmware response
(max 255) and stores it in hid_dev_count without any bounds
check.  This value propagates to num_hid_devices and is used to
index five fixed-size arrays in struct ishtp_cl_data
(MAX_HID_DEVICES = 32): report_descr[], report_descr_size[],
hid_sensor_hubs[], hid_descr[], and hid_descr_size[].

If the firmware reports more than 32 devices, hid_ishtp_cl_init()
writes past all five arrays, corrupting subsequent struct fields
(including work_struct members with embedded function pointers)
and potentially adjacent heap objects.

Clamp hid_dev_count to MAX_HID_DEVICES at the single point where
it enters the driver (process_recv, ENUM_DEVICES branch), which
covers both the probe and the reset paths.

This is a data-validation hardening fix: the ISH firmware is
within the platform trust boundary (loaded via CSME).

This patch was drafted with AI assistance; the vulnerability
analysis and source-level verification were done manually.

Signed-off-by: Shen Yongchao <grayhat@foxmail.com>
Fixes: 0b28cb4bcb17 ("HID: intel-ish-hid: ISH HID client driver")
Cc: stable@vger.kernel.org
---
 drivers/hid/intel-ish-hid/ishtp-hid-client.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
index 6d64008..XXXXXXX 100644
--- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
+++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
@@ -123,6 +123,8 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
 				break;
 			}
 			client_data->hid_dev_count = (unsigned int)*payload;
+			if (client_data->hid_dev_count > MAX_HID_DEVICES)
+				client_data->hid_dev_count = MAX_HID_DEVICES;
 			if (!client_data->hid_devices)
 				client_data->hid_devices = devm_kcalloc(
 						cl_data_to_dev(client_data),

Re: [PATCH v2] HID: intel-ish-hid: clamp HID device count to MAX_HID_DEVICES

[Greg Kroah-Hartman]

On Thu, Jul 30, 2026 at 08:25:08PM +0800, Shen Yongchao wrote:
> The HOSTIF_DM_ENUM_DEVICES response handler takes the HID device
> count from the first payload byte of the ISH firmware response
> (max 255) and stores it in hid_dev_count without any bounds
> check.  This value propagates to num_hid_devices and is used to
> index five fixed-size arrays in struct ishtp_cl_data
> (MAX_HID_DEVICES = 32): report_descr[], report_descr_size[],
> hid_sensor_hubs[], hid_descr[], and hid_descr_size[].
> 
> If the firmware reports more than 32 devices, hid_ishtp_cl_init()
> writes past all five arrays, corrupting subsequent struct fields
> (including work_struct members with embedded function pointers)
> and potentially adjacent heap objects.
> 
> Clamp hid_dev_count to MAX_HID_DEVICES at the single point where
> it enters the driver (process_recv, ENUM_DEVICES branch), which
> covers both the probe and the reset paths.
> 
> This is a data-validation hardening fix: the ISH firmware is
> within the platform trust boundary (loaded via CSME).
> 
> This patch was drafted with AI assistance; the vulnerability
> analysis and source-level verification were done manually.

Great, please use the Assisted-by: tag as the documentation requries :)

thanks,

greg k-h

Re: [PATCH v2] HID: intel-ish-hid: clamp HID device count to MAX_HID_DEVICES

[sashiko-bot]

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

Pre-existing issues:
- [High] Heap buffer overflow due to missing reallocation on repeated firmware enumeration.
- [High] Pointer arithmetic bug and integer underflow in HOSTIF_PUBLISH_INPUT_REPORT_LIST leads to massive out-of-bounds read.
- [High] Missing unregistration of HID devices on probe error path leads to Use-After-Free.
- [High] Dangling ISHTP event callback on probe error leads to Use-After-Free.
- [High] Missing cancellation of workqueues in the device remove path leads to Use-After-Free.
- [Medium] Out-of-bounds read in ENUM_DEVICES due to flawed bounds check.
--

--- Patch [1]: [PATCH v2] HID: intel-ish-hid: clamp HID device count to MAX_HID_DEVICES ---
commit c49d3200c2984f75996b1adb9efa6e6cf870abe9
Author: Shen Yongchao <grayhat@foxmail.com>
Subject: HID: intel-ish-hid: clamp HID device count to MAX_HID_DEVICES

This patch addresses an out-of-bounds array write in the ISH HID client
driver. It clamps the reported HID device count to MAX_HID_DEVICES during
the HOSTIF_DM_ENUM_DEVICES response parsing, preventing firmware from
overwriting fixed-size arrays within the client data structure.

> diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> index 6d64008f2ce05..6ea997932aeab 100644
> --- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> +++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> @@ -123,6 +123,8 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
>  				break;
>  			}
>  			client_data->hid_dev_count = (unsigned int)*payload;
> +			if (client_data->hid_dev_count > MAX_HID_DEVICES)
> +				client_data->hid_dev_count = MAX_HID_DEVICES;
>  			if (!client_data->hid_devices)
>  				client_data->hid_devices = devm_kcalloc(
>  						cl_data_to_dev(client_data),

[Severity: High]
This is a pre-existing issue, but does this allocation logic leave us
vulnerable if the firmware sends the HOSTIF_DM_ENUM_DEVICES packet multiple
times?

If a reset occurs or multiple packets are sent, and the new hid_dev_count is
larger than the previous one, the check for (!client_data->hid_devices) will
skip reallocation.

The loop that follows will then copy the new, larger count of devices into
the older, smaller buffer, which can corrupt adjacent heap memory.


[Severity: Medium]
This isn't a bug introduced by this patch, but is there a bounds checking
flaw in the device enumeration loop?

drivers/hid/intel-ish-hid/ishtp-hid-client.c: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);
...
}

The check uses data_len (the absolute total message length) but compares it
to an offset relative to payload. Since payload is already offset by the
header size (6 bytes), this check permits reading out of bounds.

Also, it only verifies the start offset, missing the 9-byte size of the
struct itself, which can allow uninitialized ring buffer memory to leak.


[Severity: High]
This is a pre-existing issue, but can this pointer arithmetic lead to a
massive out-of-bounds read in the HOSTIF_PUBLISH_INPUT_REPORT_LIST handler?

drivers/hid/intel-ish-hid/ishtp-hid-client.c:process_recv() {
...
    payload_len = report_len - sizeof(struct hostif_msg_hdr);
    ...
    report += sizeof(*report) + payload_len;
...
}

If report_len is less than 6, payload_len underflows to a very large integer.

Furthermore, because report is a typed pointer to struct report (which is
size 8), adding (sizeof(*report) + payload_len) multiplies that offset by 8.

This advances the pointer far past the message boundary, causing subsequent
iterations to read arbitrary memory and pass it to hid_input_report().


[Severity: High]
This isn't a bug introduced by this patch, but does the initialization path
leak HID devices on probe failure?

drivers/hid/intel-ish-hid/ishtp-hid-client.c:hid_ishtp_cl_init() {
...
    for (i = 0; i < client_data->num_hid_devices; ++i) {
        ...
        rv = ishtp_hid_probe(i, client_data);
        if (rv) {
            dev_err(...);
            goto err_cl_disconnect;
        }
    }
...
}

If ishtp_hid_probe() fails during the loop, it returns without unregistering
the devices that succeeded in previous iterations. 

When the probe fails, devres frees the client_data, but the leaked active
HID devices retain dangling pointers to this freed memory via their
driver_data, leading to a use-after-free when accessed.


[Severity: High]
This is a pre-existing issue, but is the event callback left dangling on
probe errors?

drivers/hid/intel-ish-hid/ishtp-hid-client.c:hid_ishtp_cl_init() {
...
    ishtp_register_event_cb(client_data->cl_device, ish_cl_event_cb);
...
}

The callback is registered here but is not unregistered in the
err_cl_disconnect path or the hid_ishtp_cl_probe() error path. 

If operations like ishtp_enum_enum_devices() fail, probe aborts and
client_data is freed. An incoming ISHTP message would then trigger the
registered callback, dereferencing the freed client_data and causing a
use-after-free.


[Severity: High]
This isn't a bug introduced by this patch, but are the workqueues properly
canceled during device removal?

drivers/hid/intel-ish-hid/ishtp-hid-client.c:hid_ishtp_cl_remove() {
    struct ishtp_cl *hid_ishtp_cl = ishtp_get_drvdata(cl_device);
    struct ishtp_cl_data *client_data = ishtp_get_client_data(hid_ishtp_cl);
    ...
}

The remove function tears down the driver instance but omits stopping the
embedded work items (client_data->work and client_data->resume_work) via
cancel_work_sync(). 

Since client_data is devres-managed, it is freed shortly after remove()
returns. If a reset or resume event occurs concurrently, the uncanceled works
will execute using freed memory, resulting in a use-after-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_D20397201115F1DCF4D1798EA0BCC6A5B007@qq.com?part=1

Re: [PATCH v2] HID: intel-ish-hid: clamp HID device count to MAX_HID_DEVICES

[srinivas pandruvada]

+Lixu

On Thu, 2026-07-30 at 20:25 +0800, Shen Yongchao wrote:
> The HOSTIF_DM_ENUM_DEVICES response handler takes the HID device
> count from the first payload byte of the ISH firmware response
> (max 255) and stores it in hid_dev_count without any bounds
> check.  This value propagates to num_hid_devices and is used to
> index five fixed-size arrays in struct ishtp_cl_data
> (MAX_HID_DEVICES = 32): report_descr[], report_descr_size[],
> hid_sensor_hubs[], hid_descr[], and hid_descr_size[].
> 
> If the firmware reports more than 32 devices, hid_ishtp_cl_init()
> writes past all five arrays, corrupting subsequent struct fields
> (including work_struct members with embedded function pointers)
> and potentially adjacent heap objects.
> 
> Clamp hid_dev_count to MAX_HID_DEVICES at the single point where
> it enters the driver (process_recv, ENUM_DEVICES branch), which
> covers both the probe and the reset paths.
> 
> This is a data-validation hardening fix: the ISH firmware is
> within the platform trust boundary (loaded via CSME).
> 
> This patch was drafted with AI assistance; the vulnerability
> analysis and source-level verification were done manually.
> 
> Signed-off-by: Shen Yongchao <grayhat@foxmail.com>
> Fixes: 0b28cb4bcb17 ("HID: intel-ish-hid: ISH HID client driver")
> Cc: stable@vger.kernel.org

Missing

Assisted-by: AGENT_NAME:MODEL_VERSION [TOOL1] [TOOL2]


Thanks,
Srinivas

> ---
>  drivers/hid/intel-ish-hid/ishtp-hid-client.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> index 6d64008..XXXXXXX 100644
> --- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> +++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> @@ -123,6 +123,8 @@ static void process_recv(struct ishtp_cl
> *hid_ishtp_cl, void *recv_buf,
>  				break;
>  			}
>  			client_data->hid_dev_count = (unsigned
> int)*payload;
> +			if (client_data->hid_dev_count >
> MAX_HID_DEVICES)
> +				client_data->hid_dev_count =
> MAX_HID_DEVICES;
>  			if (!client_data->hid_devices)
>  				client_data->hid_devices =
> devm_kcalloc(
>  						cl_data_to_dev(clien
> t_data),