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

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

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