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

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

Pre-existing issues:
- [High] Missing cancellation of work items in driver removal leads to a Use-After-Free when the devres-allocated client data is freed.
- [High] Dynamic arrays are not reallocated on subsequent firmware responses or device resets, causing heap buffer overflows when larger payloads bypass the reallocation logic.
- [High] Flawed bounds checking and missing loop termination when parsing `HOSTIF_DM_ENUM_DEVICES` allow reading past the end of `recv_buf`.
- [High] Pointer arithmetic on a typed `report` pointer implicitly scales by the structure size in `HOSTIF_PUBLISH_INPUT_REPORT_LIST`, causing massive out-of-bounds memory reads.
- [High] If `ishtp_hid_probe()` fails for a device during initialization, previously registered HID devices are left stranded and active, leading to a Use-After-Free.
--

--- Patch [1]: [PATCH v3] HID: intel-ish-hid: clamp HID device count to MAX_HID_DEVICES ---
commit 559bcab8783934e29e559180566c360b1aa54115
Author: Shen Yongchao <[email protected]>
Subject: HID: intel-ish-hid: clamp HID device count to MAX_HID_DEVICES

This commit clamps the HID device count received from the ISH firmware to
MAX_HID_DEVICES. This prevents potential out-of-bounds array writes during
initialization when the firmware reports more devices than supported.

> 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
> @@ -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;

[Severity: High]
This is a pre-existing issue, but is it possible for the firmware to send a
message where the payload length is zero?

If payload_len is zero, dereferencing the payload here could lead to an
out-of-bounds read.

Additionally, does the parsing loop properly bounds check the device_info
payload?

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 >= payload_len) {
            dev_err(...);
        }
        if (1 + sizeof(struct device_info) * i >= data_len)
            break;
        ...
        memcpy(client_data->hid_devices + i, dev_info,
               sizeof(struct device_info));
    }
}

The loop only checks the start offset against data_len. If the end of the
device_info structure straddles the boundary, could this memcpy() read past
the end of the buffer? It also appears the loop does not break if the
payload_len bounds check fails.

> +			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 how does this handle sequential firmware
responses with varying device counts?

If a subsequent HOSTIF_DM_ENUM_DEVICES response arrives with a larger count,
the allocation is skipped. Could the parsing loop then copy up to the new,
larger count and write past the end of the previously allocated array?

This same pattern appears in the HOSTIF_GET_HID_DESCRIPTOR and
HOSTIF_GET_REPORT_DESCRIPTOR handlers, where a larger subsequent payload
might bypass reallocation and overflow the buffers.


While reviewing the rest of the file, I noticed a few other pre-existing
issues that might need attention:

[Severity: High]
This is a pre-existing issue, but does the pointer arithmetic in the
HOSTIF_PUBLISH_INPUT_REPORT_LIST handler scale correctly?

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

Since report is a pointer to struct report, C pointer arithmetic will
implicitly multiply the added value by the size of the structure. Could this
cause the loop to advance by (6 + payload_len) * 6 bytes instead of the
intended size, leading to an out-of-bounds memory read?


[Severity: High]
This is a pre-existing issue, but does the device removal path properly
cancel pending work items?

drivers/hid/intel-ish-hid/ishtp-hid-client.c:hid_ishtp_cl_remove() {
    ...
    ishtp_put_device(cl_device);
    ishtp_hid_remove(client_data);
    hid_ishtp_cl_deinit(hid_ishtp_cl);

    hid_ishtp_cl = NULL;
}

If a reset or resume event queues work concurrently with or immediately prior
to unbind, could client_data be freed by devres while the workqueue still
references it, leading to a use-after-free?


[Severity: High]
This is a pre-existing issue, but does the initialization failure path leave
previously registered HID devices stranded?

drivers/hid/intel-ish-hid/ishtp-hid-client.c:hid_ishtp_cl_init() {
    ...
    rv = ishtp_hid_probe(i, client_data);
    if (rv) {
        dev_err(cl_data_to_dev(client_data),
                "[hid-ish]: HID probe for #%u failed: %d\n", i, rv);
        goto err_cl_disconnect;
    }
    ...
}

If ishtp_hid_probe() fails for a subsequent device (e.g., index 1), the
function returns an error without unregistering the previously successful
device (index 0). Could this leave the first device fully registered in the
HID subsystem while client_data is freed by devres, leading to a
use-after-free upon interaction?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1