Re: [PATCH] HID: cp2112: validate input response lengths

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The irqchip callback `irq_shutdown` causes a system panic by sleeping in atomic context.
- [High] Concurrent masking and unmasking corrupts `dev->irq_mask`.
- [High] Stale state reads occur due to missing memory ordering between data updates and the availability flag.
- [High] Late USB responses from aborted I2C transfers wrongly satisfy new transfer wait conditions.
--

commit 4d019b9aa500a3c35cd141dda658a1f88ad1c46a
Author: Yousef Alhouseen <alhouseenyousef@gmail.com>

HID: cp2112: validate input response lengths

cp2112_raw_event() parses transfer-status responses as a fixed seven-byte
structure without checking the report size. It also trusts the length in
data-read responses and copies that many bytes even when the received
report is shorter. A malformed USB device can use either path to trigger
out-of-bounds reads from the HID input buffer.

Reject short status responses with -EMSGSIZE. Treat truncated data
responses as zero-length reads so the waiting transfer fails instead of
timing out or copying beyond the report.

Fixes: e932d8178667 ("HID: add hid-cp2112 driver")
Cc: stable@vger.kernel.org

> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
> index 04379db93571b..f2988659a5cbb 100644
> --- a/drivers/hid/hid-cp2112.c
> +++ b/drivers/hid/hid-cp2112.c
> @@ -1430,6 +1430,12 @@ static int cp2112_raw_event(struct hid_device *hdev, struct hid_report *report,
>  
>  	switch (data[0]) {
>  	case CP2112_TRANSFER_STATUS_RESPONSE:
> +		if (size < sizeof(*xfer)) {
> +			dev->xfer_status = -EMSGSIZE;
> +			atomic_set(&dev->xfer_avail, 1);

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to use atomic_set()
here without a write memory barrier?

If a thread waiting on another CPU reads the flag locklessly via
wait_event_interruptible_timeout() in cp2112_wait(), could the CPU reorder
the data stores and loads? For example:

cp2112_wait()
    ret = wait_event_interruptible_timeout(dev->wait,
            atomic_read(avail), msecs_to_jiffies(RESPONSE_TIMEOUT));

Could this cause the reader to see the flag as set but read stale data?

[Severity: High]
This is also a pre-existing issue, but does setting the availability flag
unconditionally here leave us vulnerable to late USB responses?

If an I2C transfer times out and a new transfer is initiated, could a delayed
response from the aborted transfer arrive here and incorrectly wake up the new
transfer since there's no sequence number checking?

> +			break;
> +		}
> +

[ ... ]

>  	case CP2112_DATA_READ_RESPONSE:
> +		if (size < 3 || data[2] > size - 3) {
> +			dev->read_length = 0;
> +			atomic_set(&dev->read_avail, 1);
> +			break;
> +		}
> +

While reviewing this file, I noticed a couple of other pre-existing issues:

[Severity: High]
This is a pre-existing issue, but does disabling a GPIO IRQ cause a system
panic by sleeping in atomic context?

The irqchip callback irq_shutdown is called by the IRQ core with the
per-IRQ desc->lock raw spinlock held. In cp2112_gpio_irq_shutdown():

cp2112_gpio_irq_shutdown()
    cancel_delayed_work_sync(&dev->gpio_poll_worker);

Could calling this sleeping function inside a raw spinlock trigger a BUG
scheduling while atomic?

[Severity: High]
This is another pre-existing issue, but can concurrent masking and unmasking
corrupt dev->irq_mask?

The irq_mask and irq_unmask callbacks are called with the per-pin desc->lock
held, so they can execute concurrently on different CPUs for different pins.

In cp2112_gpio_irq_mask():
    __clear_bit(hwirq, &dev->irq_mask);

And in cp2112_gpio_irq_unmask():
    __set_bit(hwirq, &dev->irq_mask);

Do these non-atomic bitwise updates create a read-modify-write data race on
the shared dev->irq_mask, leading to lost or stuck interrupts?

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