Re: [PATCH] HID: mcp2221: stop device IO before hid_hw_stop

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

New issues:
- [Medium] Unconditional call to `hid_device_io_stop()` causes spurious "io already stopped" kernel warnings on normal device removal.

Pre-existing issues:
- [High] The driver leaves `mcp->rxbuf` pointing to freed memory after an I2C/SMBus transfer completes or times out, leading to a write use-after-free on spurious USB reports.
- [High] The `mcp2221_raw_event` function fails to validate the `size` parameter of incoming HID reports, causing an out-of-bounds read and kernel information leak.
--

--- Patch [1]: [PATCH] HID: mcp2221: stop device IO before hid_hw_stop ---
commit 17aa5ce1c914b6c625bef7c6ef52e3e63205f24d
Author: Jiangshan Yi <[email protected]>

HID: mcp2221: stop device IO before hid_hw_stop
    
This commit adds a call to hid_device_io_stop() at the start of the devm
cleanup callback mcp2221_hid_unregister(). This ensures device IO is
quiesced before the hardware is stopped, addressing a potential
use-after-free during probe failure or device removal.

> diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
> index e4ddd8e9293b6..83857ec1d61ca 100644
> --- a/drivers/hid/hid-mcp2221.c
> +++ b/drivers/hid/hid-mcp2221.c
> @@ -1049,6 +1049,7 @@ static void mcp2221_hid_unregister(void *ptr)
>  {
>  	struct hid_device *hdev = ptr;
>  
> +	hid_device_io_stop(hdev);
[Severity: Medium]
Will this unconditional call trigger a spurious "io already stopped" kernel
warning on normal device removal?

During normal device removal, hid_device_remove() sets hdev->io_started = false
before tearing down the devres group. When devres subsequently invokes
mcp2221_hid_unregister(), hid_device_io_stop() will hit the !io_started
condition and print a dev_warn.

>  	hid_hw_close(hdev);
>  	hid_hw_stop(hdev);
>  }

[Severity: High]
This is a pre-existing issue, but does this driver leave mcp->rxbuf pointing
to freed memory if an I2C/SMBus transfer times out?

In drivers/hid/hid-mcp2221.c:mcp_i2c_smbus_read(), the buffer pointer is set:

    mcp->rxbuf = smbus_buf;
    mcp->rxbuf_size = smbus_len;

If the transfer times out, the function returns without clearing mcp->rxbuf.
The caller may then free or pop the buffer. If a delayed or spurious report
later arrives, mcp2221_raw_event() will blindly copy data into the stale
pointer:

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

Can a malicious or slow USB device send a delayed MCP2221_I2C_GET_DATA packet
and cause a write use-after-free here?


[Severity: High]
This is a pre-existing issue, but does mcp2221_raw_event() fail to validate
the size of incoming HID reports?

When processing raw USB input reports, the function trusts data[3] for the
copy length without validating it against the actual received report size:

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

If a malicious USB device sends a short report (e.g., 4 bytes) but specifies
a payload length of 60 in data[3], will this cause an out-of-bounds read
from the HID core's transfer_buffer and leak uninitialized memory to
userspace?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1