Re: [PATCH 3/4] HID: mcp2221: fix out-of-bounds read 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] Stale mcp->rxbuf pointer leads to Use-After-Free or Stack Buffer Overflow when receiving delayed MCP2221_I2C_GET_DATA input reports.
- [High] An out-of-bounds read occurs in mcp2221_raw_event due to uninitialized or shared mcp->gp_idx state.
- [High] Shared transfer state variables such as mcp->rxbuf_idx are modified concurrently by process context and the HID raw event handler without locking.
- [High] The `retries` counter in `mcp_init_work` is declared as a `static int`, leading to data races and logic bugs when multiple mcp2221 devices are probed or a single device is replugged.
--

--- Patch [3]: [PATCH 3/4] HID: mcp2221: fix out-of-bounds read in mcp2221_raw_event ---
commit 8bc8521d47c1c74b70fcc5afb281169b37669d79
Author: Jiale Yao <yaojiale02@163.com>

HID: mcp2221: fix out-of-bounds read in mcp2221_raw_event

This commit adds bounds checking to the mcp2221_raw_event handler to
prevent out-of-bounds reads when processing HID reports. It introduces a
top-level size check and specific size constraints for different I2C and
GPIO operations.

> diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
> index e4ddd8e9293b6..0423b5affe8f6 100644
> --- a/drivers/hid/hid-mcp2221.c
> +++ b/drivers/hid/hid-mcp2221.c
[ ... ]
> @@ -918,7 +931,8 @@ static int mcp2221_raw_event(struct hid_device *hdev,
>  			}
>  			if (data[2] == MCP2221_I2C_READ_COMPL ||
>  			    data[2] == MCP2221_I2C_READ_PARTIAL) {
> -				if (!mcp->rxbuf || mcp->rxbuf_idx < 0 || data[3] > 60) {
> +				if (!mcp->rxbuf || mcp->rxbuf_idx < 0 ||
> +				    data[3] > 60 || size < 4 + data[3]) {
>  					mcp->status = -EINVAL;
>  					break;
>  				}

[Severity: Critical]
This is a pre-existing issue, but does this code write to a dangling
stack pointer if a delayed input report arrives?

Looking at mcp_i2c_smbus_read(), mcp->rxbuf is pointed to a local stack
buffer. If the I2C request times out after 5 retries, mcp_i2c_smbus_read()
returns without clearing mcp->rxbuf:

mcp_i2c_smbus_read() {
    ...
    retries++;
    if (retries == 5)
        return -ETIMEDOUT;
    ...
}

Could a malicious USB device send a delayed MCP2221_I2C_GET_DATA report
after the timeout, causing mcp2221_raw_event() to locklessly copy up to
60 bytes into the now-freed stack frame?

[Severity: High]
This is also a pre-existing issue, but is it safe to modify mcp->rxbuf_idx
here without a lock?

The I2C process thread holds mcp->lock while initializing mcp->rxbuf_idx
in mcp_i2c_smbus_read():

mcp_i2c_smbus_read() {
    ...
    mcp->rxbuf_idx = 0;
    ...
}

However, mcp2221_raw_event() runs asynchronously in softirq context. Can a
spurious input report race with the process-context operations, corrupting
the buffer bounds or indices?

[ ... ]
> @@ -958,6 +974,8 @@ static int mcp2221_raw_event(struct hid_device *hdev,
>  		break;
>  
>  	case MCP2221_GPIO_SET:
> +		if (size < sizeof(struct mcp_set_gpio))
> +			return 1;
>  		switch (data[1]) {
>  		case MCP2221_SUCCESS:
>  			if ((data[mcp->gp_idx] == MCP2221_ALT_F_NOT_GPIOV) ||

[Severity: High]
This is a pre-existing issue, but does this code perform an out-of-bounds
access if a device sends an unsolicited report before any GPIO operations
occur?

If mcp->gp_idx is still 0, evaluating data[mcp->gp_idx - 1] in the
MCP2221_GPIO_SET handler will read from data[-1].

Similarly, if mcp->gp_idx is legitimately set to a high value like 17,
could an unsolicited MCP2221_GPIO_GET report with a valid size of 10
access data[18], reading past the bounds of the short packet? The newly
added check for the overall packet size does not prevent this since it
doesn't validate the independently modified mcp->gp_idx.

[ ... ]

[Severity: High]
This is a pre-existing issue outside the modified hunks, but is the
retries counter in mcp_init_work() susceptible to data races across
multiple devices?

mcp_init_work() defines retries as a static variable:

mcp_init_work() {
    static int retries = 5;
    ...
    if (!retries--)
        return;
    ...
    schedule_delayed_work(&mcp->init_work, msecs_to_jiffies(100));
}

If a device fails IIO initialization multiple times, the static counter
drops to -1. Once negative, !retries-- evaluates to false, causing an
infinite rescheduling loop for any subsequent devices or replugs that
fail. Should this retry state be tracked per-device in the mcp2221
struct instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714131611.269929-1-yaojiale02@163.com?part=3