Re: [PATCH] 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:
- [Critical] The patch adds a size bound but leaves a dangling pointer in `mcp->rxbuf` when a read times out, resulting in a Use-After-Free.
- [High] Unsynchronized updates to `mcp->rxbuf` and `mcp->rxbuf_size` allow an attacker to bypass the new bounds check completely, causing an Out-Of-Bounds (OOB) write.
- [High] The patch fails to prevent stack buffer overflows for `I2C_SMBUS_BLOCK_DATA` reads because it bases `mcp->rxbuf_size` on uninitialized stack memory.
- [High] The raw event handler blindly accesses payload data without validating against the actual received HID report size, leading to out-of-bounds kernel heap reads.
--

commit 2cea5cadcf6c54079354cc3c519d5621e3b06d74
Author: Florian Pradines <florian.pradines@gmail.com>

HID: mcp2221: fix OOB write in mcp2221_raw_event()

This commit attempts to fix an out-of-bounds write in mcp2221_raw_event()
by adding a rxbuf_size field. It sets this field alongside rxbuf in
mcp_i2c_smbus_read() and validates the incoming data length against it
before copying.

> diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
> index be80970ab48e2..e4ddd8e9293b6 100644
> --- a/drivers/hid/hid-mcp2221.c
> +++ b/drivers/hid/hid-mcp2221.c
[ ... ]
> @@ -330,12 +331,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;

Does this code fail to prevent stack buffer overflows for
I2C_SMBUS_BLOCK_DATA reads?

In i2c_smbus_read_block_data(), the i2c_smbus_data union is allocated
on the stack but its first byte (data.block[0]) is not initialized before
the read. In mcp_smbus_xfer(), the driver uses data->block[0] + 1 to
determine the read length, which is then assigned here to mcp->rxbuf_size.

If this uninitialized stack memory contains a large value (e.g., 255), the
new bounds check will pass for large responses, and memcpy can write past
the 34-byte data->block array, overflowing the stack.

Also, are these updates to mcp->rxbuf and mcp->rxbuf_size safe from
concurrent access?

These are updated non-atomically in process context, while
mcp2221_raw_event() runs asynchronously.

If a previous read timed out leaving rxbuf_idx = 0 and rxbuf_size = 60,
could a late 60-byte response arrive just after mcp->rxbuf is assigned a new
2-byte buffer but before mcp->rxbuf_size is updated to 2?

This would cause the event handler to read the old rxbuf_size (60), pass the
check, and copy 60 bytes into the 2-byte buffer, bypassing the protection.

>  	}
>  
>  	ret = mcp_send_data_req_status(mcp, mcp->txbuf, 4);

If mcp_send_data_req_status() times out, it returns -ETIMEDOUT without
setting mcp->rxbuf to NULL.

If the caller then frees or pops the buffer, and a delayed or malicious
MCP2221_I2C_GET_DATA report is processed by mcp2221_raw_event(), could this
result in a use-after-free?

Since rxbuf is not NULL and rxbuf_idx < rxbuf_size, the new check passes,
and memcpy would write data directly into the freed memory.

[ ... ]
> @@ -919,6 +922,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;
> +				}
>  				buf = mcp->rxbuf;
>  				memcpy(&buf[mcp->rxbuf_idx], &data[4], data[3]);

This isn't a regression from this patch, but does the raw event handler
blindly access payload data without validating against the actual received
HID report size?

The mcp2221_raw_event() function receives a size parameter representing the
length of the HID report but ignores it.

If a malicious USB device presents a HID report descriptor with a small
maximum report size (e.g., 4 bytes), the HID core will allocate a 4-byte
buffer.

If the device then sends data[3] = 60, this memcpy will read 60 bytes past
the end of the 4-byte kernel heap buffer, potentially leaking adjacent
kernel memory to user-space.

>  				mcp->rxbuf_idx = mcp->rxbuf_idx + data[3];

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260509094517.2691246-1-florian.pradines@gmail.com?part=1