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

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

[Pengpeng Hou]

The MCP2221A data sheet specifies 64-byte HID command and response
reports. mcp2221_raw_event() reads fixed response fields up to byte 55
and can copy a 60-byte I2C payload from byte 4 through byte 63 without
first checking the received size.

HID invokes raw_event() before generic report validation. A short report
can therefore be interpreted beyond its received length. Reject reports
shorter than the fixed protocol size before reading any response fields.

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>
---
Changes since v1: https://lore.kernel.org/all/20260715083754.31210-1-pengpeng@iscas.ac.cn/
- validate the fixed 64-byte protocol size instead of the
  descriptor-derived report length
- use a named report-size constant and remove matching magic lengths
---
 drivers/hid/hid-mcp2221.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index e4ddd8e..c910f66 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -90,7 +90,8 @@ enum {
 	MCP2221_DIR_IN = 0x01,
 };
 
-#define MCP_NGPIO	4
+#define MCP2221_REPORT_SIZE	64
+#define MCP_NGPIO		4
 
 /* MCP GPIO set command layout */
 struct mcp_set_gpio {
@@ -126,7 +127,7 @@ struct mcp2221 {
 	struct completion wait_in_report;
 	struct delayed_work init_work;
 	u8 *rxbuf;
-	u8 txbuf[64];
+	u8 txbuf[MCP2221_REPORT_SIZE];
 	int rxbuf_idx;
 	int rxbuf_size;
 	int status;
@@ -639,11 +640,11 @@ static int mcp_gpio_read_sram(struct mcp2221 *mcp)
 {
 	int ret;
 
-	memset(mcp->txbuf, 0, 64);
+	memset(mcp->txbuf, 0, sizeof(mcp->txbuf));
 	mcp->txbuf[0] = MCP2221_GET_SRAM_SETTINGS;
 
 	mutex_lock(&mcp->lock);
-	ret = mcp_send_data_req_status(mcp, mcp->txbuf, 64);
+	ret = mcp_send_data_req_status(mcp, mcp->txbuf, sizeof(mcp->txbuf));
 	mutex_unlock(&mcp->lock);
 
 	return ret;
@@ -682,7 +683,7 @@ static int mcp2221_check_gpio_pinfunc(struct mcp2221 *mcp)
 	 * Set all bytes to 0, so Bit 7 is not set. The chip
 	 * only changes content of a register when bit 7 is set.
 	 */
-	memset(mcp->txbuf, 0, 64);
+	memset(mcp->txbuf, 0, sizeof(mcp->txbuf));
 	mcp->txbuf[0] = MCP2221_SET_SRAM_SETTINGS;
 
 	/*
@@ -703,7 +704,7 @@ static int mcp2221_check_gpio_pinfunc(struct mcp2221 *mcp)
 	}
 
 	mutex_lock(&mcp->lock);
-	ret = mcp_send_data_req_status(mcp, mcp->txbuf, 64);
+	ret = mcp_send_data_req_status(mcp, mcp->txbuf, sizeof(mcp->txbuf));
 	mutex_unlock(&mcp->lock);
 
 	return ret;
@@ -861,6 +862,12 @@ static int mcp2221_raw_event(struct hid_device *hdev,
 	u8 *buf;
 	struct mcp2221 *mcp = hid_get_drvdata(hdev);
 
+	if (size < MCP2221_REPORT_SIZE) {
+		mcp->status = -EINVAL;
+		complete(&mcp->wait_in_report);
+		return 1;
+	}
+
 	switch (data[0]) {
 
 	case MCP2221_I2C_WR_DATA:
-- 
2.50.1 (Apple Git-155)

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

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Use-After-Free and Out-Of-Bounds Write due to a dangling `mcp->rxbuf` pointer after an I2C transaction times out.
- [Critical] Time-of-Check to Time-of-Use (TOCTOU) Buffer Overflow in `mcp2221_raw_event` due to non-atomic buffer state updates.
- [High] Concurrent GPIO and I2C operations modify the shared hardware buffer (`mcp->txbuf`) outside of the `mcp->lock` mutex, leading to data corruption.
- [High] Teardown asymmetry and Use-After-Free caused by runtime modification of the `gpio_mode_enforce` module parameter.
--

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

The MCP2221A data sheet specifies 64-byte HID command and response
reports. mcp2221_raw_event() reads fixed response fields up to byte 55
and can copy a 60-byte I2C payload from byte 4 through byte 63 without
first checking the received size. Reject reports shorter than the fixed
protocol size before reading any response fields.

Link: https://lore.kernel.org/all/20260715083754.31210-1-pengpeng@iscas.ac.cn/

> diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
> index e4ddd8e9293b6..c910f6636ac95 100644
> --- a/drivers/hid/hid-mcp2221.c
> +++ b/drivers/hid/hid-mcp2221.c

[ ... ]

> @@ -639,11 +640,11 @@ static int mcp_gpio_read_sram(struct mcp2221 *mcp)
>  {
>  	int ret;
>  
> -	memset(mcp->txbuf, 0, 64);
> +	memset(mcp->txbuf, 0, sizeof(mcp->txbuf));

[Severity: High]
This is a pre-existing issue, but I noticed mcp->txbuf is being zeroed and
populated here in mcp_gpio_read_sram() (and similarly in mcp_gpio_set() and
mcp_gpio_get()) before acquiring mcp->lock.

Because mcp->txbuf is shared with I2C operations, could this lead to data
corruption if mcp_i2c_xfer() is concurrently preparing an I2C transaction
under the lock? It looks like sysfs GPIO nodes and I2C device interfaces
can race here.

>  	mcp->txbuf[0] = MCP2221_GET_SRAM_SETTINGS;
>  
>  	mutex_lock(&mcp->lock);
> -	ret = mcp_send_data_req_status(mcp, mcp->txbuf, 64);
> +	ret = mcp_send_data_req_status(mcp, mcp->txbuf, sizeof(mcp->txbuf));
>  	mutex_unlock(&mcp->lock);
>  
>  	return ret;

[ ... ]

> @@ -861,6 +862,12 @@ static int mcp2221_raw_event(struct hid_device *hdev,
>  	u8 *buf;
>  	struct mcp2221 *mcp = hid_get_drvdata(hdev);
>  
> +	if (size < MCP2221_REPORT_SIZE) {
> +		mcp->status = -EINVAL;
> +		complete(&mcp->wait_in_report);
> +		return 1;
> +	}
> +

[Severity: Critical]
This is a pre-existing issue, but does mcp2221_raw_event() have a
use-after-free or out-of-bounds write risk when handling delayed responses?

If mcp_send_data_req_status() times out inside mcp_i2c_smbus_read(), it
returns early without setting mcp->rxbuf to NULL. When a delayed I2C
response arrives later, wouldn't mcp2221_raw_event() copy up to 60 bytes
into this dangling mcp->rxbuf pointer?

[Severity: Critical]
This is also a pre-existing issue, but is there a TOCTOU race here with how
mcp->rxbuf and mcp->rxbuf_size are updated in mcp_i2c_smbus_read()?

If mcp_i2c_smbus_read() is updating these non-atomically:

mcp_i2c_smbus_read() {
    ...
    mcp->rxbuf = msg->buf;
    mcp->rxbuf_size = msg->len;
    ...
}

and an interrupt fires exactly between assigning a new 1-byte buffer to
mcp->rxbuf and updating mcp->rxbuf_size, could mcp2221_raw_event() bypass
the bounds check by reading the old, larger size?

>  	switch (data[0]) {
>  
>  	case MCP2221_I2C_WR_DATA:

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but could runtime modifications
to the module parameter gpio_mode_enforce lead to a use-after-free during
device removal?

In mcp2221_remove(), the delayed work is conditionally canceled:

mcp2221_remove() {
    ...
    if (!gpio_mode_enforce)
        cancel_delayed_work_sync(&mcp->init_work);
    ...
}

Since gpio_mode_enforce has 0644 permissions, if it is changed to true
after the device is plugged in, won't mcp2221_remove() skip canceling
mcp->init_work? This would leave the work running after the mcp structure
is freed.

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