Re: [PATCH v2 3/3] HID: mcp2221: validate report size in mcp2221_raw_event()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Unprotected Time-Of-Check to Time-Of-Use (TOCTOU) race condition in `mcp->rxbuf` access leads to a Use-After-Free and kernel stack corruption.
- [High] The patch incompletely fixes out-of-bounds reads by only validating the payload length for `MCP2221_I2C_GET_DATA`, leaving other command branches vulnerable.
- [High] Data race on the shared `mcp->txbuf` buffer and `mcp->gp_idx` during GPIO operations corrupts concurrent USB command payloads.
- [High] Lack of request/response sequence tracking leads to completion crosstalk and state machine corruption.
--

--- Patch [3]: [PATCH v2 3/3] HID: mcp2221: validate report size in mcp2221_raw_event() ---
commit 771b21acaed8d565e6718447cf1dd1d9d3045324
Author: Jiangshan Yi <yijiangshan@kylinos.cn>

HID: mcp2221: validate report size in mcp2221_raw_event()

This patch adds a minimum size check to incoming HID reports in
mcp2221_raw_event() and validates that the source range fits within the
received report before performing a memcpy in the MCP2221_I2C_GET_DATA path.
This prevents a malicious USB device from causing an out-of-bounds read
that could leak uninitialized kernel memory.

> diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
> index 9e03d1f733ff8..d52ce3531ab70 100644
> --- a/drivers/hid/hid-mcp2221.c
> +++ b/drivers/hid/hid-mcp2221.c
> @@ -865,6 +865,9 @@ static int mcp2221_raw_event(struct hid_device *hdev,
>  	u8 *buf;
>  	struct mcp2221 *mcp = hid_get_drvdata(hdev);
>  
> +	if (size < 4)
> +		return 0;
> +

[Severity: High]
This is a pre-existing issue, but while this patch adds a size check for
the MCP2221_I2C_GET_DATA command, does it leave other commands vulnerable?

For instance, the MCP2221_I2C_PARAM_OR_STATUS branch copies from data[50]:

drivers/hid/hid-mcp2221.c:mcp2221_raw_event() {
    ...
    case MCP2221_I2C_PARAM_OR_STATUS:
        ...
        memcpy(&mcp->adc_values, &data[50], sizeof(mcp->adc_values));
    ...
}

And MCP2221_GET_SRAM_SETTINGS accesses data[22]:

drivers/hid/hid-mcp2221.c:mcp2221_raw_event() {
    ...
    case MCP2221_GET_SRAM_SETTINGS:
        ...
        memcpy(&mcp->mode, &data[22], 4);
    ...
}

Could a short USB report of 4 bytes still leak uninitialized memory through
these paths via the IIO subsystem?

>  	switch (data[0]) {
>  
>  	case MCP2221_I2C_WR_DATA:
[ ... ]
> @@ -930,6 +933,10 @@ static int mcp2221_raw_event(struct hid_device *hdev,
>  					mcp->status = -EINVAL;
>  					break;
>  				}
> +				if (4 + data[3] > size) {
> +					mcp->status = -EINVAL;
> +					break;
> +				}
>  				buf = mcp->rxbuf;
>  				memcpy(&buf[mcp->rxbuf_idx], &data[4], data[3]);

[Severity: Critical]
This isn't a bug introduced by this patch, but is there an unprotected
race condition between the interrupt handler and process context here?

If an I2C read times out, mcp_i2c_smbus_read() clears mcp->rxbuf in
process context:

drivers/hid/hid-mcp2221.c:mcp_i2c_smbus_read() {
    ...
out:
    mcp->rxbuf = NULL;
    mcp->rxbuf_size = 0;
    ...
}

If a delayed USB response arrives concurrently, could mcp2221_raw_event()
check mcp->rxbuf successfully earlier in the function, but before this
memcpy runs, the process context clears it and destroys the stack frame,
causing the memcpy to corrupt kernel stack memory?

>  				mcp->rxbuf_idx = mcp->rxbuf_idx + data[3];


[Severity: High]
This isn't a bug introduced by this patch, but does the unconditional
completion of the wait queue lead to state machine corruption?

drivers/hid/hid-mcp2221.c:mcp2221_raw_event() {
    ...
    complete(&mcp->wait_in_report);
    ...
}

If a command times out and mcp_cancel_last_cmd() resets the wait state,
could a delayed hardware response for the original command trigger this
complete() call, falsely waking up the cancellation command or subsequent
transactions early?


[Severity: High]
This is a pre-existing issue, but does mutating shared state before
acquiring the lock in the GPIO handlers cause a data race?

In mcp_gpio_set(), mcp->txbuf and mcp->gp_idx are modified before taking
mcp->lock:

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);
    mcp->txbuf[mcp->gp_idx - 1] = 1;
    mcp->txbuf[mcp->gp_idx] = !!value;

    mutex_lock(&mcp->lock);
    ...
}

If another thread is concurrently executing mcp_i2c_xfer() and assembling its
I2C payload in mcp->txbuf under the lock, could this zero out and corrupt
the I2C payload while it is being actively sent? Similar unprotected
modifications also appear to occur in mcp_gpio_get().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728131442.834604-1-yijiangshan@kylinos.cn?part=3