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

[sashiko-bot@kernel.org]

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