Re: [PATCH v2 2/3] HID: mcp2221: clear rxbuf after I2C/SMBus transfer completes

[sashiko-bot@kernel.org]

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

New issues:
- [High] Time-Of-Check to Time-Of-Use (TOCTOU) race condition leading to NULL pointer dereference or Use-After-Free.

Pre-existing issues:
- [High] Silent I2C data corruption due to brittle lock-step synchronization.
- [High] Unkillable task hang (infinite loop) on 0-length payload.
- [High] Kernel stack buffer overflow or broken I2C_SMBUS_BLOCK_DATA read due to SMBus API misuse.
- [High] Out-of-bounds read in the HID raw event handler due to missing report size validation.
--

--- Patch [2]: [PATCH v2 2/3] HID: mcp2221: clear rxbuf after I2C/SMBus transfer completes ---
commit 3603851056413fb3957e5edfb92c7c98a7d4fdf4
Author: Jiangshan Yi <yijiangshan@kylinos.cn>
Subject: HID: mcp2221: clear rxbuf after I2C/SMBus transfer completes

This patch clears the caller-supplied buffer pointer in mcp->rxbuf and resets
mcp->rxbuf_size when an I2C/SMBus transfer completes or times out. This aims
to prevent a write use-after-free where a delayed or spurious
MCP2221_I2C_GET_DATA report could memcpy device data into a freed memory
region.

> diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
> index 5c7fc56c7c671..9e03d1f733ff8 100644
> --- a/drivers/hid/hid-mcp2221.c
> +++ b/drivers/hid/hid-mcp2221.c
> @@ -343,7 +343,7 @@ static int mcp_i2c_smbus_read(struct mcp2221 *mcp,
>  
>  	ret = mcp_send_data_req_status(mcp, mcp->txbuf, 4);
>  	if (ret)
> -		return ret;
> +		goto out;

[Severity: High]
This isn't a bug introduced by this patch, but does the driver's timeout
handling lead to silent data corruption?

When waiting for a report, the driver uses a lock-step timeout:

drivers/hid/hid-mcp2221.c:mcp_send_data_req_status() {
        t = wait_for_completion_timeout(&mcp->wait_in_report,
                                        msecs_to_jiffies(4000));
        if (!t)
                return -ETIMEDOUT;
}

If this times out, the driver starts a new transfer. However, if a delayed HID
report from the timed-out transfer arrives later, mcp2221_raw_event()
unconditionally completes the wait:

drivers/hid/hid-mcp2221.c:mcp2221_raw_event() {
        buf = mcp->rxbuf;
        memcpy(&buf[mcp->rxbuf_idx], &data[4], data[3]);
        mcp->rxbuf_idx = mcp->rxbuf_idx + data[3];
        mcp->status = 0;
        break;
        ...
        complete(&mcp->wait_in_report);
}

Can this wake up the new transfer and mistakenly copy the old, delayed data
into the new transfer's buffer?

>  
>  	mcp->rxbuf_idx = 0;
>  
> @@ -365,7 +365,7 @@ static int mcp_i2c_smbus_read(struct mcp2221 *mcp,
>  			} else {
>  				usleep_range(980, 1000);
>  				mcp_cancel_last_cmd(mcp);
> -				return ret;
> +				goto out;
>  			}
>  		} else {
>  			retries = 0;

[Severity: High]
This is a pre-existing issue, but does this loop risk an unkillable task hang
if a 0-length payload is received?

drivers/hid/hid-mcp2221.c:mcp2221_raw_event() {
        if (!mcp_get_i2c_eng_state(mcp, data, 2)
                && (data[3] == 0)) {
                mcp->status = 0;
                break;
        }
}

If a USB device continuously returns a 0-length payload, mcp2221_raw_event()
sets mcp->status = 0 and calls complete().

The read loop then observes a success and resets retries = 0, but rxbuf_idx
does not advance. Does this cause the loop to spin infinitely?

> @@ -375,6 +375,10 @@ static int mcp_i2c_smbus_read(struct mcp2221 *mcp,
>  	usleep_range(980, 1000);
>  	ret = mcp_chk_last_cmd_status_free_bus(mcp);
>  
> +out:
> +	mcp->rxbuf = NULL;
> +	mcp->rxbuf_size = 0;

[Severity: High]
Could this lockless clearing introduce a Time-Of-Check to Time-Of-Use race?

In mcp2221_raw_event(), which runs asynchronously in softirq context, the
code checks the pointer and then dereferences it:

drivers/hid/hid-mcp2221.c:mcp2221_raw_event() {
        if (!mcp->rxbuf || mcp->rxbuf_idx < 0 || data[3] > 60) {
        ...
        buf = mcp->rxbuf;
        memcpy(&buf[mcp->rxbuf_idx], &data[4], data[3]);
}

If this function clears mcp->rxbuf concurrently after mcp2221_raw_event()
passes the check but before assigning it to buf, couldn't buf become NULL,
causing memcpy() to trigger a panic?

> +
>  	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but does the driver misuse the SMBus API,
potentially causing a kernel stack buffer overflow?

For I2C_SMBUS_BLOCK_DATA reads, the driver uses data->block[0] as the
input read length:

drivers/hid/hid-mcp2221.c:mcp_smbus_xfer() {
        case I2C_SMBUS_BLOCK_DATA:
                if (read_write == I2C_SMBUS_READ) {
                        ...
                        ret = mcp_i2c_smbus_read(mcp, NULL,
                                                MCP2221_I2C_RD_RPT_START,
                                                addr, data->block[0] + 1,
                                                data->block);
}

If an in-kernel driver calls i2c_smbus_read_block_data(), data.block[0] is an
uninitialized stack variable. Does the driver then use this uninitialized
stack garbage as the read length, causing mcp2221_raw_event() to blindly copy
up to 255 bytes into the 34-byte data.block array on the caller's kernel
stack?

[Severity: High]
This isn't a bug introduced by this patch, but does mcp2221_raw_event()
perform out-of-bounds reads due to missing report size validation?

The function receives the size of the incoming HID report but never validates
it before accessing fields far into the buffer:

drivers/hid/hid-mcp2221.c:mcp2221_raw_event() {
#if IS_REACHABLE(CONFIG_IIO)
                memcpy(&mcp->adc_values, &data[50], sizeof(mcp->adc_values));
#endif
...
        case MCP2221_GET_SRAM_SETTINGS:
                switch (data[1]) {
                case MCP2221_SUCCESS:
                        memcpy(&mcp->mode, &data[22], 4);
}

If a malicious USB device presents a HID report descriptor with a very small
report size, will the driver read past the bounds of the allocation on the
kernel heap?

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