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

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 <[email protected]>

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: [email protected]

> 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/[email protected]?part=3