[PATCH] HID: cp2112: validate raw report lengths before parsing

[PATCH] HID: cp2112: validate raw report lengths before parsing

[Pengpeng Hou]

cp2112_raw_event() receives raw HID reports and switches on data[0],
then parses transfer-status and data-read response fields at fixed
offsets. Malformed short reports can therefore be dereferenced before
the handler verifies that the fixed fields are present.

Check the report length before parsing each response type. Short
transfer-status reports complete the pending transfer with -EINVAL,
while short or truncated read responses complete as a zero-length read
so the existing short-read handling can reject them.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/hid/hid-cp2112.c |   18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 803b883a..7527bb08 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -1387,8 +1387,17 @@ static int cp2112_raw_event(struct hid_device *hdev, struct hid_report *report,
 	struct cp2112_device *dev = hid_get_drvdata(hdev);
 	struct cp2112_xfer_status_report *xfer = (void *)data;
 
+	if (size < 1)
+		return 0;
+
 	switch (data[0]) {
 	case CP2112_TRANSFER_STATUS_RESPONSE:
+		if (size < sizeof(*xfer)) {
+			dev->xfer_status = -EINVAL;
+			atomic_set(&dev->xfer_avail, 1);
+			break;
+		}
+
 		hid_dbg(hdev, "xfer status: %02x %02x %04x %04x\n",
 			xfer->status0, xfer->status1,
 			be16_to_cpu(xfer->retries), be16_to_cpu(xfer->length));
@@ -1422,11 +1431,19 @@ static int cp2112_raw_event(struct hid_device *hdev, struct hid_report *report,
 		atomic_set(&dev->xfer_avail, 1);
 		break;
 	case CP2112_DATA_READ_RESPONSE:
+		if (size < 3) {
+			dev->read_length = 0;
+			atomic_set(&dev->read_avail, 1);
+			break;
+		}
+
 		hid_dbg(hdev, "read response: %02x %02x\n", data[1], data[2]);
 
 		dev->read_length = data[2];
 		if (dev->read_length > sizeof(dev->read_data))
 			dev->read_length = sizeof(dev->read_data);
+		if (dev->read_length > size - 3)
+			dev->read_length = 0;
 
 		memcpy(dev->read_data, &data[3], dev->read_length);
 		atomic_set(&dev->read_avail, 1);
@@ -1453,4 +1470,3 @@ module_hid_driver(cp2112_driver);
 MODULE_DESCRIPTION("Silicon Labs HID USB to SMBus master bridge");
 MODULE_AUTHOR("David Barksdale <dbarksdale@uplogix.com>");
 MODULE_LICENSE("GPL");
-

Re: [PATCH] HID: cp2112: validate raw report lengths before parsing

[sashiko-bot]

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

Pre-existing issues:
- [High] Potential data race due to missing memory barriers between payload updates and availability flags, leading to stale reads.
- [High] Use-After-Free due to incorrect teardown sequence of GPIO poll worker in remove.
--

commit 1bac7c472f6cc9dcb9d59a5d99910a2876f7382f
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

HID: cp2112: validate raw report lengths before parsing

This commit adds bounds checking to cp2112_raw_event() to ensure raw HID
reports are large enough before parsing their contents. Short transfer-status
reports now complete with -EINVAL, while short read responses complete as a
zero-length read.

> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
> index 04379db93571b..3f5be199842b1 100644
> --- a/drivers/hid/hid-cp2112.c
> +++ b/drivers/hid/hid-cp2112.c
> @@ -1428,8 +1428,17 @@ static int cp2112_raw_event(struct hid_device *hdev, struct hid_report *report,
>  	struct cp2112_device *dev = hid_get_drvdata(hdev);
>  	struct cp2112_xfer_status_report *xfer = (void *)data;
>  
> +	if (size < 1)
> +		return 0;
> +
>  	switch (data[0]) {
>  	case CP2112_TRANSFER_STATUS_RESPONSE:
> +		if (size < sizeof(*xfer)) {
> +			dev->xfer_status = -EINVAL;
> +			atomic_set(&dev->xfer_avail, 1);

[Severity: High]
This is a pre-existing issue, but could this atomic_set() lead to stale
payload reads on weakly ordered architectures?

Since atomic_set() doesn't provide a memory barrier, is it possible for the
processor to reorder the dev->xfer_status update with this dev->xfer_avail
update?

If so, a reader checking the condition locklessly via the fast path in
wait_event_interruptible_timeout() in cp2112_wait() might observe the flag
as true and return before the new payload is visible.

> +			break;
> +		}
> +

[ ... ]

> @@ -1463,11 +1472,19 @@ static int cp2112_raw_event(struct hid_device *hdev, struct hid_report *report,
>  		atomic_set(&dev->xfer_avail, 1);
>  		break;
>  	case CP2112_DATA_READ_RESPONSE:
> +		if (size < 3) {
> +			dev->read_length = 0;
> +			atomic_set(&dev->read_avail, 1);

[Severity: High]
This is a pre-existing issue, but similar to the above, does this missing
memory barrier allow the dev->read_length update to be reordered with the
availability flag update?

> +			break;
> +		}
> +

[ ... ]

[Severity: High]
This is a pre-existing issue, but looking at cp2112_remove(), is there a
potential use-after-free race during teardown?

drivers/hid/hid-cp2112.c:cp2112_remove() {
    ...
    cancel_delayed_work_sync(&dev->gpio_poll_worker);
    ...
    gpiochip_remove(&dev->gc);
    ...
}

During the window between these two calls, the gpiochip is still registered.
Could a concurrent irq_startup request call cp2112_gpio_irq_startup(), which
sets dev->gpio_poll = true and reschedules the delayed work?

If the delayed work fires after cp2112_remove() returns and dev is freed via
devres, would cp2112_gpio_poll_callback() dereference the freed dev?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701054023.59015-1-pengpeng@iscas.ac.cn?part=1