Re: [PATCH AUTOSEL 7.0-5.10] HID: mcp2221: fix OOB write in mcp2221_raw_event()

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch introduces `mcp->rxbuf_size` to prevent Out-of-Bounds (OOB) writes but fails to initialize it in `mcp_smbus_xfer()` for block operations.

Pre-existing issues:
- [High] The `mcp->rxbuf` pointer is never cleared after an I2C/SMBus transfer completes or times out, leaving a dangling pointer that can be exploited.
- [High] The `mcp2221_raw_event()` handler ignores the `size` parameter provided by the HID subsystem, leading to out-of-bounds reads on the kernel heap.
- [High] A data race exists between the I2C transfer context and the HID raw event context, which accesses `rxbuf` state variables without synchronization.
--

commit 161d8d13687380796daedd1f35b98269619cd589
Author: Florian Pradines <florian.pradines@gmail.com>

HID: mcp2221: fix OOB write in mcp2221_raw_event()

mcp2221_raw_event() copies device-supplied data into mcp->rxbuf at offset
rxbuf_idx without verifying that the copy fits within the destination buffer.
A device responding with up to 60 bytes to a small I2C/SMBus read can overflow
this buffer. This patch adds an rxbuf_size field to struct mcp2221 and checks
rxbuf_idx + data[3] <= rxbuf_size before performing the copy.

> diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
> index ef3b5c77c38e3..badd4958bc12c 100644
> --- a/drivers/hid/hid-mcp2221.c
> +++ b/drivers/hid/hid-mcp2221.c
[ ... ]
> @@ -323,12 +324,14 @@ static int mcp_i2c_smbus_read(struct mcp2221 *mcp,
>  		mcp->txbuf[3] = (u8)(msg->addr << 1);
>  		total_len = msg->len;
>  		mcp->rxbuf = msg->buf;
> +		mcp->rxbuf_size = msg->len;
>  	} else {
>  		mcp->txbuf[1] = smbus_len;
>  		mcp->txbuf[2] = 0;
>  		mcp->txbuf[3] = (u8)(smbus_addr << 1);
>  		total_len = smbus_len;
>  		mcp->rxbuf = smbus_buf;
> +		mcp->rxbuf_size = smbus_len;

[Severity: High]
Does this overlook updating mcp->rxbuf_size for block operations in
mcp_smbus_xfer()?

In mcp_smbus_xfer(), mcp->rxbuf is manually assigned for block reads without
updating mcp->rxbuf_size:

drivers/hid/hid-mcp2221.c:mcp_smbus_xfer() {
...
	case I2C_SMBUS_BLOCK_DATA:
		if (read_write == I2C_SMBUS_READ) {
...
			mcp->rxbuf_idx = 0;
			mcp->rxbuf = data->block;
...
}

If rxbuf_size is 0, the new bounds check in mcp2221_raw_event() will always
fail, breaking legitimate SMBus block reads. If rxbuf_size holds a large stale
value from a prior transfer, the check might pass and still allow an overflow
on the data->block stack buffer.

>  	}
>  
>  	ret = mcp_send_data_req_status(mcp, mcp->txbuf, 4);
[ ... ]
> @@ -912,6 +915,10 @@ static int mcp2221_raw_event(struct hid_device *hdev,
>  					mcp->status = -EINVAL;
>  					break;
>  				}
> +				if (mcp->rxbuf_idx + data[3] > mcp->rxbuf_size) {
> +					mcp->status = -EINVAL;
> +					break;
> +				}

[Severity: High]
This is a pre-existing issue, but does mcp2221_raw_event() ignore the size
parameter provided by the HID subsystem?

The driver unconditionally accesses data bytes like data[3], and later copies
data[3] bytes starting from &data[4], without verifying the actual length of
the received HID report buffer. If a malicious device provides a small report
size, could reading or copying these bytes result in out-of-bounds reads on
the kernel heap?


[Severity: High]
This isn't a bug introduced by this patch, but is there a data race between
the I2C transfer context and the HID raw event context?

While mcp_i2c_smbus_read() and mcp_smbus_xfer() update mcp->rxbuf,
mcp->rxbuf_size, and mcp->rxbuf_idx while holding mcp->lock,
mcp2221_raw_event() runs asynchronously in the HID input report context and
accesses these variables without acquiring the lock.

If a stray or malicious USB report arrives concurrently with a new I2C read
request, could the state become desynchronized and lead to an out-of-bounds
write?

>  				buf = mcp->rxbuf;

[Severity: High]
This is a pre-existing issue, but is the mcp->rxbuf pointer left dangling
after a transfer completes or times out?

During transfers, mcp->rxbuf points to a caller-provided buffer which is often
allocated on the kernel stack. When a transfer times out, the stack frame is
popped, but mcp->rxbuf still points to the now-invalid stack address.

If a delayed or spurious USB report arrives later, could mcp2221_raw_event()
copy device-controlled data into this stale pointer and corrupt the kernel
stack?

>  				memcpy(&buf[mcp->rxbuf_idx], &data[4], data[3]);
>  				mcp->rxbuf_idx = mcp->rxbuf_idx + data[3];
>  				mcp->status = 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520111944.3424570-25-sashal@kernel.org?part=1