* Re: [PATCH] HID: mcp2221: fix OOB write in mcp2221_raw_event()
2026-05-09 9:45 [PATCH] HID: mcp2221: fix OOB write in mcp2221_raw_event() Florian Pradines
@ 2026-05-09 10:36 ` sashiko-bot
2026-05-12 15:48 ` Jiri Kosina
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-05-09 10:36 UTC (permalink / raw)
To: Florian Pradines; +Cc: dmitry.torokhov, linux-input
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
^ permalink raw reply [flat|nested] 3+ messages in thread