[PATCH] HID: mcp2200: reject short read-all responses

[PATCH] HID: mcp2200: reject short read-all responses

[Yousef Alhouseen]

mcp2200_raw_event() casts every READ_ALL response to the full 16-byte
response structure and reads fields through byte 10 without checking the
received report size. A malformed USB device can therefore trigger
out-of-bounds reads from the input buffer.

Complete the pending command with -EMSGSIZE when the response is short.

Fixes: 740329d7120f ("HID: mcp2200: added driver for GPIOs of MCP2200")
Cc: stable@vger.kernel.org
Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
 drivers/hid/hid-mcp2200.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/hid/hid-mcp2200.c b/drivers/hid/hid-mcp2200.c
index dafdd5b4a079..d49f3aa44448 100644
--- a/drivers/hid/hid-mcp2200.c
+++ b/drivers/hid/hid-mcp2200.c
@@ -302,6 +302,10 @@ static int mcp2200_raw_event(struct hid_device *hdev, struct hid_report *report,
 	switch (data[0]) {
 	case READ_ALL:
 		all_resp = (struct mcp_read_all_resp *) data;
+		if (size < sizeof(*all_resp)) {
+			mcp->status = -EMSGSIZE;
+			break;
+		}
 		mcp->status = 0;
 		mcp->gpio_inval = all_resp->io_port_val_bmap;
 		mcp->baud_h = all_resp->baud_h;
-- 
2.54.0

Re: [PATCH] HID: mcp2200: reject short read-all responses

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing synchronization in `mcp_cmd_read_all` and lockless shared state access in `mcp2200_raw_event` lead to race conditions, timeouts, and data corruption.
- [High] `mcp_get` silently ignores negative error codes returned by `mcp_get_multiple`, converting hardware or communication errors into a valid 'low' state.
- [High] The driver fails to synchronize its internal state cache (`mcp->gpio_dir` and `mcp->gpio_val`) with the actual hardware configuration during probe.
- [High] Device unbind race condition leading to Use-After-Free due to improper devm_ teardown order.
- [High] The `hid_report` buffer is embedded directly within `struct mcp2200` and passed to DMA mapping functions, violating DMA cache-line alignment requirements.
--

commit 3df6b9c6c4d5763dc3316160e66ba44dfef6c606
Author: Yousef Alhouseen <alhouseenyousef@gmail.com>

HID: mcp2200: reject short read-all responses

This commit adds a size check for READ_ALL responses in the MCP2200
driver. If the response size is smaller than the expected struct size,
it sets the status to -EMSGSIZE and aborts processing to prevent
out-of-bounds reads.

> diff --git a/drivers/hid/hid-mcp2200.c b/drivers/hid/hid-mcp2200.c
> index dafdd5b4a0797..d49f3aa44448e 100644
> --- a/drivers/hid/hid-mcp2200.c
> +++ b/drivers/hid/hid-mcp2200.c
> @@ -302,6 +302,10 @@ static int mcp2200_raw_event(struct hid_device *hdev, struct hid_report *report,
>  	switch (data[0]) {
>  	case READ_ALL:
>  		all_resp = (struct mcp_read_all_resp *) data;
> +		if (size < sizeof(*all_resp)) {
> +			mcp->status = -EMSGSIZE;

[Severity: High]
This is a pre-existing issue, but won't this new -EMSGSIZE error be silently
swallowed by mcp_get()?

Looking at mcp_get(), it calls mcp_get_multiple() which propagates this
negative error code. However, mcp_get() discards the return value:

drivers/hid/hid-mcp2200.c:mcp_get() {
    ...
    mcp_get_multiple(gc, &mask, &bits);
    return bits > 0;
}

Since bits remains uninitialized or zeroed upon error, this converts the
-EMSGSIZE error (or any other communication failure) into a valid logical
low state.

Does this need to be handled so downstream drivers don't receive false
logical states when the device errors out?

> +			break;
> +		}
>  		mcp->status = 0;
>  		mcp->gpio_inval = all_resp->io_port_val_bmap;

[Severity: High]
This is a pre-existing issue, but does mcp2200_raw_event() update shared
device state locklessly?

mcp_cmd_read_all() orchestrates a transaction but drops mcp->lock before
waiting for a completion, and calls reinit_completion() unprotected:

drivers/hid/hid-mcp2200.c:mcp_cmd_read_all() {
    ...
    reinit_completion(&mcp->wait_in_report);
    
    mutex_lock(&mcp->lock);
    ...
    mutex_unlock(&mcp->lock);
    
    ...
    t = wait_for_completion_timeout(&mcp->wait_in_report, ...);
}

If multiple threads concurrently invoke mcp_cmd_read_all(), or unrelated
hardware reports arrive while waiting, their wait queues and completion
states can collide.

Furthermore, mcp2200_raw_event() updates mcp->status and mcp->gpio_inval
locklessly in interrupt context, and unconditionally completes the wait queue
for any unexpected report.

Should a single lock serialize the entire command-response wait cycle?

[Severity: High]
This is also a pre-existing issue, but why is the io_bmap hardware direction
state provided in READ_ALL responses completely ignored here instead of being
saved to mcp->gpio_dir?

In mcp2200_probe(), mcp->gpio_dir defaults to 0 (all outputs) without
querying the initial state. Because io_bmap is ignored here, when
mcp_set_direction() is later called to modify a specific pin, it masks
against the uninitialized local cache:

drivers/hid/hid-mcp2200.c:mcp_set_direction() {
    ...
    conf->io_bmap = (mcp->gpio_dir & ~(1 << gpio_nr))
        | (io_direction << gpio_nr);
}

Doesn't this clobber the direction of all other pins, potentially causing
electrical short-circuits by driving input pins as outputs?

>  		mcp->baud_h = all_resp->baud_h;

[ ... ]

Additionally, there are a couple of other pre-existing issues in this driver
that might be worth addressing:

[Severity: High]
This is a pre-existing issue, but is there a device unbind race condition
leading to a use-after-free due to the devm_ teardown order?

mcp2200_probe() uses devm_gpiochip_add_data() to register the gpiochip:

drivers/hid/hid-mcp2200.c:mcp2200_probe() {
    ...
    ret = devm_gpiochip_add_data(&hdev->dev, &mcp->gc, mcp);
    ...
}

However, mcp2200_remove() calls hid_hw_stop(hdev) synchronously, freeing
underlying URBs:

drivers/hid/hid-mcp2200.c:mcp2200_remove() {
    ...
    hid_hw_stop(hdev);
}

Because devm_ cleanup callbacks run after mcp2200_remove() completes, the
gpiochip remains fully active.

If userspace invokes a GPIO operation during this window, mcp_cmd_read_all()
will call hid_hw_output_report(), triggering a use-after-free on the freed
urbout. 

Should the driver use manual teardown in remove() instead of devm_?

[Severity: High]
This is a pre-existing issue, but does the hid_report buffer violate DMA
cache-line alignment requirements?

The hid_report buffer is embedded directly within struct mcp2200:

drivers/hid/hid-mcp2200.c:
struct mcp2200 {
    ...
    struct gpio_chip gc;
    u8 hid_report[16];
};

This buffer is placed at the end of the struct without a
____cacheline_aligned attribute.

When hid_hw_output_report() is called, the USB HID layer receives this
buffer and maps it for DMA. On non-coherent architectures, mapping a buffer
that shares cache lines with other actively modified struct members (like gc
or status) can cause cache invalidation or flush operations to overwrite
concurrent CPU writes. 

Could this buffer be allocated independently or aligned to the cacheline size?

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