[PATCH] HID: mcp2221: validate raw report length

[PATCH] HID: mcp2221: validate raw report length

[Pengpeng Hou]

HID calls a driver raw_event() callback before it performs the generic
report-length validation. mcp2221_raw_event() subsequently reads fixed
response fields up to byte 55 and can copy a response payload through byte
63.

A short input report can therefore be interpreted using bytes beyond the
current report length before the HID core rejects it. Validate the
descriptor-derived report length at the beginning of the driver callback.

Complete the pending command with -EINVAL for a truncated report so that a
malformed response does not turn into an unrelated timeout.

Fixes: 67a95c21463d ("HID: mcp2221: add usb to i2c-smbus host bridge")
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/hid/hid-mcp2221.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index e4ddd8e9293b..67afef1a275f 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -861,6 +861,17 @@ static int mcp2221_raw_event(struct hid_device *hdev,
 	u8 *buf;
 	struct mcp2221 *mcp = hid_get_drvdata(hdev);
 
+	/*
+	 * HID invokes .raw_event() before its generic report-length check.
+	 * The response handlers below read fixed offsets from @data, so reject
+	 * a truncated current report before interpreting any of its fields.
+	 */
+	if (size < hid_report_len(report)) {
+		mcp->status = -EINVAL;
+		complete(&mcp->wait_in_report);
+		return 1;
+	}
+
 	switch (data[0]) {
 
 	case MCP2221_I2C_WR_DATA:
-- 
2.43.0

Re: [PATCH] HID: mcp2221: validate raw report length

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Stack Use-After-Free via stale `mcp->rxbuf` pointer
- [High] Resource leak / infinite loop due to static `retries` variable
- [High] Race condition during device probe leading to NULL pointer dereference
- [Medium] Incomplete Fix / Vulnerability Bypass in report length validation
- [Medium] Data race on shared state in GPIO callbacks
--

--- Patch [1]: [PATCH] HID: mcp2221: validate raw report length ---
commit fe663280b0cd79c441444742c0ab076f8dbfec5d
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

HID: mcp2221: validate raw report length

This commit adds validation of the report length at the beginning of the
mcp2221_raw_event callback. This prevents the driver from reading beyond the
current report length and returns an error for truncated reports to avoid
unrelated timeouts.

> diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
> index e4ddd8e9293b6..67afef1a275f9 100644
> --- a/drivers/hid/hid-mcp2221.c
> +++ b/drivers/hid/hid-mcp2221.c
> @@ -861,6 +861,17 @@ static int mcp2221_raw_event(struct hid_device *hdev,
>  	u8 *buf;
>  	struct mcp2221 *mcp = hid_get_drvdata(hdev);

[Severity: High]
This is a pre-existing issue, but could this lead to a NULL pointer dereference
of the mcp pointer during device probe?

In mcp2221_probe(), hid_hw_open() is called to start the hardware and submit
the interrupt IN URB before hid_set_drvdata() is called. If the device sends
an early interrupt IN report, mcp2221_raw_event() could be triggered while
hid_get_drvdata() still returns NULL, causing a panic when accessing
mcp->status.

>  
> +	/*
> +	 * HID invokes .raw_event() before its generic report-length check.
> +	 * The response handlers below read fixed offsets from @data, so reject
> +	 * a truncated current report before interpreting any of its fields.
> +	 */
> +	if (size < hid_report_len(report)) {

[Severity: Medium]
This isn't a bug introduced by this patch, but does this bounds check use the
untrustworthy descriptor length instead of the required minimum length for the
fixed-offset reads performed subsequently?

If a malicious USB device presents a spoofed HID report descriptor with a very
small report length (e.g., 1 byte), hid_report_len(report) will evaluate to 1.
When a 1-byte packet arrives, this check evaluates to false and is bypassed.
The driver then performs fixed-offset reads, potentially copying uninitialized
or stale data from the buffer to userspace. Should the validation check
against the hardcoded structural length instead?

> +		mcp->status = -EINVAL;
> +		complete(&mcp->wait_in_report);
> +		return 1;
> +	}
> +
>  	switch (data[0]) {
>  
>  	case MCP2221_I2C_WR_DATA:

[ ... ]


[Severity: High]
This is a pre-existing issue, but does the static retries variable cause
infinite delayed work scheduling?

drivers/hid/hid-mcp2221.c:mcp_init_work() {
    ...
    static int retries = 5;
    ...
}

Because retries is declared as static, it is shared across all device
instances. If initialization fails multiple times and retries decrements below
zero, the check can evaluate to false on subsequent runs. This causes the work
to infinitely reschedule itself, leaking CPU resources and preventing system
suspend. Should this variable be an instance member in struct mcp2221 instead?


[Severity: Critical]
This is a pre-existing issue, but does this code use the stale mcp->rxbuf
pointer after the stack frame is freed?

drivers/hid/hid-mcp2221.c:mcp_i2c_smbus_read() {
    ...
    mcp->rxbuf = msg->buf;
    ...
}

In mcp_i2c_smbus_read(), mcp->rxbuf is assigned a buffer that may be stack
allocated, and the function returns without resetting mcp->rxbuf to NULL.

If the device subsequently sends an unsolicited input report,
mcp2221_raw_event() will execute:

drivers/hid/hid-mcp2221.c:mcp2221_raw_event() {
    ...
    memcpy(&buf[mcp->rxbuf_idx], &data[4], data[3]);
    ...
}

Could this overwrite up to 60 bytes of kernel stack memory with
attacker-controlled data?


[Severity: Medium]
This is a pre-existing issue, but does this code create a data race on the
shared mcp->txbuf and mcp->gp_idx variables?

drivers/hid/hid-mcp2221.c:mcp_gpio_set() {
    ...
    memset(mcp->txbuf, 0, 18);
    mcp->txbuf[0] = MCP2221_GPIO_SET;
    mcp->gp_idx = offsetof(struct mcp_set_gpio, gpio[offset].value);
    
    mutex_lock(&mcp->lock);
    ...
}

The shared txbuf and gp_idx fields are modified here before the mutex lock is
acquired. If multiple threads access GPIOs concurrently, their unlocked
modifications to the shared mcp->txbuf and mcp->gp_idx variables will
interleave. Can this cause the device to receive a corrupted command or
mcp2221_raw_event() to use the wrong index when interpreting the response?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715083754.31210-1-pengpeng@iscas.ac.cn?part=1