[PATCH v2 0/3] DO-NOT-MERGE: HID: mcp2221: stop device IO before hid_hw_stop and fix UAF/OOB-read

[PATCH v2 0/3] DO-NOT-MERGE: HID: mcp2221: stop device IO before hid_hw_stop and fix UAF/OOB-read

[Jiangshan Yi]

This series revisits the "stop device IO before hid_hw_stop" v1
submission and, while reviewing it, fixes two pre-existing security
issues in the same driver.

Patch 1 is v2 of the original single-patch submission. v1 added an
unconditional hid_device_io_stop() to the devm cleanup callback, which
prints a spurious "io already stopped" warning on normal device
removal: hid_device_remove() clears io_started before the devres group
is released, so the callback always sees io_started == false. v2 guards
the call with io_started so it only runs on the probe-failure path that
actually needs to balance hid_device_io_start().

Patches 2 and 3 are independent, pre-existing bugs (both dating back to
the driver's introduction in 67a95c21463d) that the Sashiko AI review
flagged while reviewing v1. They are self-contained, each carries its
own Fixes: tag for -stable, and neither depends on patch 1 or on each
other:

  * Patch 2 - mcp->rxbuf is left dangling after an I2C/SMBus transfer
    completes or times out; a delayed or spurious report can then write
    device data into the freed buffer (write use-after-free).
  * Patch 3 - mcp2221_raw_event() never validates the report size and
    trusts data[3] as the copy length, so a malicious short report can
    leak uninitialized kernel memory back through the I2C read path.

The original v1 submission is available at:
https://lore.kernel.org/r/20260728061919.310367-1-yijiangshan@kylinos.cn

Signed-off-by: Jiangshan Yi <yijiangshan@kylinos.cn>

Jiangshan Yi (3):
  HID: mcp2221: stop device IO before hid_hw_stop
  HID: mcp2221: clear rxbuf after I2C/SMBus transfer completes
  HID: mcp2221: validate report size in mcp2221_raw_event()

 drivers/hid/hid-mcp2221.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

--
2.25.1

[PATCH v2 1/3] HID: mcp2221: stop device IO before hid_hw_stop

[Jiangshan Yi]

Quiesce device IO at the start of the devm cleanup callback
mcp2221_hid_unregister() so that incoming HID reports cannot race with
hardware teardown during probe failure or device removal, addressing a
potential use-after-free.

Guard the call to hid_device_io_stop() with io_started. On normal
removal hid_device_remove() has already cleared io_started before the
devres group is released, so an unconditional call would otherwise hit
the !io_started path and emit a spurious "io already stopped" warning
on every removal. The guard preserves the probe-failure balancing,
where io_started is still set after hid_device_io_start(), while
staying silent on the normal removal path.

Fixes: d4b50ac06ea6 ("HID: mcp2221: Allow IO to start during probe")
Cc: stable@vger.kernel.org
Signed-off-by: Jiangshan Yi <yijiangshan@kylinos.cn>
---
Changes in v2:
- Guard hid_device_io_stop() with io_started to avoid a spurious
  "io already stopped" warning on normal device removal, as flagged by
  the Sashiko AI review of v1.

v1: https://lore.kernel.org/r/20260728061919.310367-1-yijiangshan@kylinos.cn

 drivers/hid/hid-mcp2221.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index e4ddd8e..5c7fc56 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -1049,6 +1049,8 @@ static void mcp2221_hid_unregister(void *ptr)
 {
 	struct hid_device *hdev = ptr;

+	if (hdev->io_started)
+		hid_device_io_stop(hdev);
 	hid_hw_close(hdev);
 	hid_hw_stop(hdev);
 }
--
2.25.1

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

[Jiangshan Yi]

mcp_i2c_smbus_read() stores the caller-supplied buffer pointer in
mcp->rxbuf for the duration of a transfer but never clears it when the
transfer finishes or times out. Once the caller frees or reuses the
buffer, mcp->rxbuf becomes a dangling pointer. A delayed or spurious
MCP2221_I2C_GET_DATA report can then drive mcp2221_raw_event() to
memcpy device data into the freed memory, causing a write
use-after-free.

Route all return paths through a single exit point that clears
mcp->rxbuf and mcp->rxbuf_size, so that the existing !mcp->rxbuf guard
in the raw_event handler can reject any report arriving after the
transfer has ended.

Fixes: 67a95c21463d ("HID: mcp2221: add usb to i2c-smbus host bridge")
Cc: stable@vger.kernel.org
Signed-off-by: Jiangshan Yi <yijiangshan@kylinos.cn>
---
 drivers/hid/hid-mcp2221.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index 5c7fc56..9e03d1f 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;

 	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;
@@ -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;
+
 	return ret;
 }
 
-- 
2.25.1

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

[sashiko-bot]

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

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

[Jiangshan Yi]

mcp2221_raw_event() never validates the size of incoming HID reports.
In the MCP2221_I2C_GET_DATA path it trusts the device-supplied data[3]
as the copy length without checking that 4 + data[3] bytes actually
exist in the received report. A malicious or misbehaving USB device can
send a short report with a large data[3], causing the memcpy to read
past the valid report data in the HID transfer buffer and leak
uninitialized kernel memory back to userspace through the I2C/SMBus
read path.

Add a minimum size check at entry and validate that the source range
fits within the received report before the copy.

Fixes: 67a95c21463d ("HID: mcp2221: add usb to i2c-smbus host bridge")
Cc: stable@vger.kernel.org
Signed-off-by: Jiangshan Yi <yijiangshan@kylinos.cn>
---
 drivers/hid/hid-mcp2221.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index 9e03d1f..d52ce35 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;
+
 	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]);
 				mcp->rxbuf_idx = mcp->rxbuf_idx + data[3];
--
2.25.1

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

[sashiko-bot]

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