[PATCH v2 1/4] HID: intel-ish-hid: fix report_list iterator pointer arithmetic

[PATCH v2 1/4] HID: intel-ish-hid: fix report_list iterator pointer arithmetic

[Shen Yongchao]

Since commit 63cafaf47a83 ("HID: ishtp-hid-client: replace
fake-flex arrays with flex-array members", v6.13), the
HOSTIF_PUBLISH_INPUT_REPORT_LIST handler iterates over sub-reports
using a struct report * pointer:

	report += sizeof(*report) + payload_len;

Because report is a struct report * (not a char *), the compiler
multiplies the advance by sizeof(struct report) = 8, making the
actual stride (8 + payload_len) * 8 bytes instead of the intended
8 + payload_len bytes.  On v6.13+ a legitimate aggregated list
with num_of_reports >= 2 drives the second iteration far outside
the message buffer.

Replace the struct report * iterator with a byte-granular u8 *pos
so the advance is computed in bytes.


Assisted-by: LLM
Signed-off-by: Shen Yongchao <grayhat@foxmail.com>
Fixes: 63cafaf47a83 ("HID: ishtp-hid-client: replace fake-flex arrays with flex-array members")
Cc: stable@vger.kernel.org
---
diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
index 6d64008f2..ba52e185c 100644
--- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
+++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
@@ -74,6 +74,7 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
 	int report_type;
 	struct report_list *reports_list;
 	struct report *report;
+	u8 *pos;
 	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;
@@ -280,9 +281,10 @@ 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;
-			report = reports_list->reports;
+			pos = (u8 *)reports_list->reports;
 
 			for (j = 0; j < reports_list->num_of_reports; j++) {
+				report = (struct report *)pos;
 				recv_msg = container_of(&report->msg,
 							struct hostif_msg, hdr);
 				report_len = report->size;
@@ -303,7 +305,7 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
 						0);
 					}
 
-				report += sizeof(*report) + payload_len;
+				pos += sizeof(struct report) + payload_len;
 			}
 			break;
 		default:
@@ -956,4 +958,4 @@ MODULE_AUTHOR("Daniel Drubin <daniel.drubin@intel.com>");
  */
 MODULE_AUTHOR("Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>");
 
-MODULE_LICENSE("GPL");
+MODULE_LICENSE("GPL");
\ No newline at end of file

Re: [PATCH v2 1/4] HID: intel-ish-hid: fix report_list iterator pointer arithmetic

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing validation of `hid_dev_count` against `MAX_HID_DEVICES` allows firmware to trigger out-of-bounds array accesses.
- [High] Missing boundary checks when iterating over aggregated reports in `HOSTIF_PUBLISH_INPUT_REPORT_LIST`.
- [High] Integer underflow when calculating `payload_len` in `HOSTIF_PUBLISH_INPUT_REPORT_LIST`.
- [High] Insufficient bounds checking when parsing `device_info` structures in `HOSTIF_DM_ENUM_DEVICES`.
- [High] The inner loop handling HOSTIF_PUBLISH_INPUT_REPORT_LIST overwrites the outer loop's control variables, causing parser desynchronization.
- [High] Missing cancellation of asynchronous background works on device removal leads to Use-After-Free.
- [High] Missing buffer reallocation during hardware reset allows out-of-bounds writes if device attributes expand.
--

--- Patch [1]: [PATCH v2 1/4] HID: intel-ish-hid: fix report_list iterator pointer arithmetic ---
commit 57fdd01a4139a53de9833b822824ccfd37902302
Author: Shen Yongchao <grayhat@foxmail.com>

HID: intel-ish-hid: fix report_list iterator pointer arithmetic

This commit fixes an issue where the report iterator pointer was being 
advanced by a multiple of the struct size rather than bytes. It introduces 
a u8 *pos iterator to correctly compute the pointer arithmetic in bytes.

> diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> index 6d64008f2ce05..ba52e185ce051 100644
> --- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> +++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
[ ... ]
> @@ -280,9 +281,10 @@ 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;
> -			report = reports_list->reports;
> +			pos = (u8 *)reports_list->reports;
>  
>  			for (j = 0; j < reports_list->num_of_reports; j++) {

[Severity: High]
This is a pre-existing issue, but does this loop lack boundary checks?

If the firmware sends malformed packets in HOSTIF_PUBLISH_INPUT_REPORT_LIST,
could we iterate past the end of the containing message or network buffer?
There don't appear to be bounds checks on pos and the sub-report payloads.

> +				report = (struct report *)pos;
>  				recv_msg = container_of(&report->msg,
>  							struct hostif_msg, hdr);
>  				report_len = report->size;

[Severity: High]
This is a pre-existing issue, but can this lead to an integer underflow?

When calculating payload_len:

    payload_len = report_len - sizeof(struct hostif_msg_hdr);

If a sub-report is sent with report->size less than 
sizeof(struct hostif_msg_hdr), will this underflow the size_t variable to a
huge value, which is then passed to hid_input_report() and used to advance 
the pos pointer?

[Severity: High]
This is a pre-existing issue, but does the inner loop overwrite the outer
loop's control variables?

The inner loop modifies payload and payload_len:

    payload = recv_msg->payload;
    payload_len = report_len - sizeof(struct hostif_msg_hdr);
    
Once this switch statement finishes, the outer loop uses payload_len to 
advance cur_pos:

    msg_len = payload_len + sizeof(struct hostif_msg);
    if (!cur_pos && cur_pos + msg_len < total_len)
        ++client_data->multi_packet_cnt;

    cur_pos += msg_len;

Could the corrupted payload_len (now the size of the last sub-report instead
of the aggregate list) cause parser desynchronization and out-of-bounds reads
on the next iteration?

> @@ -303,7 +305,7 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
>  						0);
>  					}
>  
> -				report += sizeof(*report) + payload_len;
> +				pos += sizeof(struct report) + payload_len;
>  			}
>  			break;
>  		default:

While reviewing this, I also noticed a few other pre-existing issues in 
this driver.

[Severity: High]
This is a pre-existing issue, but is there missing validation of 
hid_dev_count against MAX_HID_DEVICES?

In process_recv() during HOSTIF_DM_ENUM_DEVICES:

    client_data->hid_dev_count = (unsigned int)*payload;

If the firmware sends a count greater than MAX_HID_DEVICES, the driver 
directly assigns this without validation. Could subsequent loops use this to 
access fixed-size arrays like hid_descr and report_descr out of bounds, 
corrupting memory?

[Severity: High]
This is a pre-existing issue, but does this allocation logic miss buffer
reallocation during a hardware reset?

During enumeration:

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

And later:

    for (i = 0; i < client_data->hid_dev_count; ++i) {

If a hardware reset occurs and hid_ishtp_cl_reset_handler() retriggers
enumeration, the check bypasses reallocation. If the firmware returns a 
larger hid_dev_count after reset, could the loop write device info past 
the end of the existing heap buffer?

[Severity: High]
This is a pre-existing issue, but is the boundary check sufficient when 
parsing device_info structures?

In the same ENUM_DEVICES loop:

    if (1 + sizeof(struct device_info) * i >= data_len)
        break;

This compares an offset relative to payload (which is already past the header)
against the total data_len. It also only checks the start offset of the struct
rather than the end. Could this allow memcpy to read up to 
sizeof(struct device_info) + sizeof(struct hostif_msg_hdr) - 1 bytes past 
the actual recv_buf?

[Severity: High]
This is a pre-existing issue, but does hid_ishtp_cl_remove() need to 
cancel background works?

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

This teardown function unregisters the driver but does not call
cancel_work_sync() for client_data->work or client_data->resume_work. Since 
client_data is allocated with devm_kzalloc(), it is automatically freed when 
remove returns. If a reset or resume work is executing or queued concurrently, 
could this lead to a Use-After-Free?

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

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

[Shen Yongchao]

num_of_reports is a u8 (up to 255) taken directly from the
firmware message, never compared against the actual payload size.
If the count exceeds the number of sub-reports the payload can
hold, the loop iterates past the receive buffer.

Compute list_end from the outer message payload and break out of
the loop when the next struct report header would fall outside
the message.


Assisted-by: LLM
Signed-off-by: Shen Yongchao <grayhat@foxmail.com>
Fixes: 0b28cb4bcb17 ("HID: intel-ish-hid: ISH HID client driver")
Cc: stable@vger.kernel.org
---
diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
index ba52e185c..6609130ce 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);

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

[sashiko-bot]

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

[PATCH v2 3/4] HID: intel-ish-hid: prevent size_t underflow in report_list parsing

[Shen Yongchao]

payload_len is computed as report_len - sizeof(struct
hostif_msg_hdr) where report_len is a firmware-controlled u16
stored in a size_t.  Values 0..5 underflow to ~SIZE_MAX, causing
hid_input_report() to read far past the receive buffer.

Add an explicit check for report_len < sizeof(struct
hostif_msg_hdr) and use a local inner_len variable instead of
overwriting the outer loop's payload_len.


Assisted-by: LLM
Signed-off-by: Shen Yongchao <grayhat@foxmail.com>
Fixes: 0b28cb4bcb17 ("HID: intel-ish-hid: ISH HID client driver")
Cc: stable@vger.kernel.org
---
diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
index 6609130ce..efe5c5326 100644
--- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
+++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
@@ -77,6 +77,7 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
 	u8 *pos;
 	u8 *list_end;
 	size_t report_len;
+	size_t inner_len;
 	struct ishtp_cl_data *client_data = ishtp_get_client_data(hid_ishtp_cl);
 	int curr_hid_dev = client_data->cur_hid_dev;
 	struct ishtp_hid_data *hid_data = NULL;
@@ -293,8 +294,11 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
 				recv_msg = container_of(&report->msg,
 							struct hostif_msg, hdr);
 				report_len = report->size;
-				payload = recv_msg->payload;
-				payload_len = report_len -
+				if (report_len < sizeof(struct hostif_msg_hdr))
+					break;
+
+
+				inner_len = report_len -
 					sizeof(struct hostif_msg_hdr);
 
 				for (i = 0; i < client_data->num_hid_devices;
@@ -306,11 +310,11 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
 						client_data->hid_sensor_hubs[
 									i],
 						report_type,
-						payload, payload_len,
+						recv_msg->payload, inner_len,
 						0);
 					}
 
-				pos += sizeof(struct report) + payload_len;
+				pos += sizeof(struct report) + inner_len;
 			}
 			break;
 		default:

Re: [PATCH v2 3/4] HID: intel-ish-hid: prevent size_t underflow in report_list parsing

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Heap buffer overflow due to missing reallocation when processing duplicate initialization responses.
- [High] Missing payload length validation before accessing specific message fields in HOSTIF_DM_ENUM_DEVICES and HOSTIF_PUBLISH_INPUT_REPORT_LIST.
- [High] Flawed bounds checking in HOSTIF_DM_ENUM_DEVICES loop allows massive out-of-bounds read.
- [High] Missing bounds check for sub-report payload length against outer message size leads to out-of-bounds read.
- [High] Dangling pointer in hid_sensor_hubs on ishtp_hid_probe failure leading to Use-After-Free.
- [High] Missing cancellation of event_work on probe failure leads to Use-After-Free.
--

--- Patch [3]: [PATCH v2 3/4] HID: intel-ish-hid: prevent size_t underflow in report_list parsing ---
commit 527ae48f789326594bc98a86ac234f6084ebf2f5
Author: Shen Yongchao <grayhat@foxmail.com>

HID: intel-ish-hid: prevent size_t underflow in report_list parsing

This patch fixes a size_t underflow in hid_input_report() when report_len
is smaller than the hostif message header. It adds an explicit check for
the length and uses a local inner_len variable instead of overwriting the
outer loop's payload_len.

Fixes: 0b28cb4bcb17 ("HID: intel-ish-hid: ISH HID client driver")
Cc: stable@vger.kernel.org

> diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> index 6609130ce63e1..efe5c5326de3b 100644
> --- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> +++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
[ ... ]
> @@ -293,8 +294,11 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
>  				recv_msg = container_of(&report->msg,
>  							struct hostif_msg, hdr);
>  				report_len = report->size;

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for a malformed
firmware message to cause an out-of-bounds read here?

Looking at HOSTIF_PUBLISH_INPUT_REPORT_LIST in process_recv():

	reports_list = (struct report_list *)recv_msg->payload;
	pos = (u8 *)reports_list->reports;
	list_end = (u8 *)recv_msg->payload + payload_len;

	for (j = 0; j < reports_list->num_of_reports; j++) {

The num_of_reports field is accessed without checking if payload_len is large
enough to contain a struct report_list.

Additionally, in the HOSTIF_DM_ENUM_DEVICES case:

	client_data->hid_dev_count = (unsigned int)*payload;

The payload pointer is dereferenced without checking if payload_len >= 1, which
could similarly read out of bounds.

> -				payload = recv_msg->payload;
> -				payload_len = report_len -
> +				if (report_len < sizeof(struct hostif_msg_hdr))
> +					break;
> +
> +
> +				inner_len = report_len -
>  					sizeof(struct hostif_msg_hdr);

[Severity: High]
This isn't a bug introduced by this patch, but could this still lead to an
out-of-bounds read when hid_input_report() is called?

While the sub-report header is bounded against list_end in a previous check:

	if (pos + sizeof(struct report) > list_end)
		break;

There doesn't appear to be a check to ensure that the payload itself
(inner_len bytes) fits within list_end. A forged, huge sub-report length could
cause hid_input_report() to read past the end of the receive buffer.

>  
>  				for (i = 0; i < client_data->num_hid_devices;
> @@ -306,11 +310,11 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
>  						client_data->hid_sensor_hubs[
>  									i],
>  						report_type,
> -						payload, payload_len,
> +						recv_msg->payload, inner_len,
>  						0);
>  					}
>  
> -				pos += sizeof(struct report) + payload_len;
> +				pos += sizeof(struct report) + inner_len;
>  			}
>  			break;
>  		default:

[Severity: High]
This isn't a bug introduced by this patch, but does the boundary check during
device enumeration correctly prevent out-of-bounds reads?

Looking at the fallback boundary check in process_recv():

	if (1 + sizeof(struct device_info) * i >= data_len)
		break;

Since dev_info is offset by cur_pos + sizeof(struct hostif_msg_hdr) from the
start of recv_buf, this check ignores the initial payload offset. If i is large
enough, could the memory address calculated for dev_info vastly exceed
recv_buf + data_len, resulting in slab-out-of-bounds reads into the hid_devices
array?

[Severity: Critical]
This isn't a bug introduced by this patch, but could duplicate initialization
responses cause a heap buffer overflow?

When handling HOSTIF_DM_ENUM_DEVICES, the code does:

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

And similarly for HOSTIF_GET_HID_DESCRIPTOR:

	if (!client_data->hid_descr[curr_hid_dev])
		client_data->hid_descr[curr_hid_dev] = devm_kmalloc(...);

If a second response arrives before init_done is true and contains a larger
payload or count, the driver skips allocation but still uses the new
payload_len or hid_dev_count to perform memory copies. Could this copy data
past the old buffer's boundary?

[Severity: High]
This isn't a bug introduced by this patch, but could a failure in
ishtp_hid_probe() lead to a use-after-free?

In ishtp_hid_probe():

	err_hid_device:
		kfree(hid_data);
	err_hid_data:
		hid_destroy_device(hid);
		return rv;

The hid device is stored in client_data->hid_sensor_hubs[cur_hid_dev] before
this error path, but is not cleared to NULL when the error path frees the
device. Since the ISHTP event callback is already registered, could a concurrent
firmware message processed by process_recv() find the freed pointer and pass it
to hid_input_report()?

[Severity: High]
This isn't a bug introduced by this patch, but does hid_ishtp_cl_probe()
properly clean up if it fails after hid_ishtp_cl_init()?

In hid_ishtp_cl_probe():

	rv = hid_ishtp_cl_init(hid_ishtp_cl, false);
	if (rv) {
		ishtp_cl_free(hid_ishtp_cl);
		return rv;
	}

The hid_ishtp_cl_init() function registers an event callback via
ishtp_register_event_cb(). If the probe subsequently fails, the error path
frees hid_ishtp_cl without unregistering the event callback or canceling the
pending event work. Could an incoming ISHTP message arriving shortly after
execute the scheduled callback on freed memory?

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

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

[Shen Yongchao]

The report iterator is never checked against the receive buffer
boundary after computing the sub-report length, so a crafted
report_len can advance the iterator past the message and
subsequent iterations read from arbitrary out-of-bounds memory.

Add a check that the full entry (struct report header plus
payload) fits within the message before processing it.  Also
switch the reports_list and list_end sources from the stale
outer-loop payload variable to recv_msg->payload, which always
points to the current message.


Assisted-by: LLM
Signed-off-by: Shen Yongchao <grayhat@foxmail.com>
Fixes: 0b28cb4bcb17 ("HID: intel-ish-hid: ISH HID client driver")
Cc: stable@vger.kernel.org
---
diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
index efe5c5326..0ac7be0c4 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++) {
 				if (pos + sizeof(struct report) > list_end)
@@ -300,6 +300,10 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
 
 				inner_len = report_len -
 					sizeof(struct hostif_msg_hdr);
+				if (pos + sizeof(struct report) + inner_len >
+						list_end)
+					break;
+
 
 				for (i = 0; i < client_data->num_hid_devices;
 				     ++i)

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

[sashiko-bot]

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