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

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