Re: [PATCH 4/4] HID: cp2112: fix out-of-bounds read in cp2112_raw_event

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing cancel_delayed_work_sync() on the probe error path can lead to Use-After-Free of devm-allocated dev structure.
- [High] Race condition in cp2112_remove() due to premature and conditional cancellation of the delayed work.
- [High] Lockless updates to dev->read_data and dev->read_length lack required memory barriers when signaled via atomic flags.
- [High] A TOCTOU race condition on dev->read_length in cp2112_read() can bypass length checks and return uninitialized kernel memory.
- [High] Using an interruptible wait in cp2112_wait() allows signals to permanently desynchronize the hardware response state.
--

--- Patch [4]: [PATCH 4/4] HID: cp2112: fix out-of-bounds read in cp2112_raw_event ---
commit 1ea70eb400858ee9a4d1aa2528079b6370e4b0a8
Author: Jiale Yao <yaojiale02@163.com>

HID: cp2112: fix out-of-bounds read in cp2112_raw_event

The TRANSFER_STATUS_RESPONSE path dereferences xfer->length at
offset 6, requiring sizeof(*xfer) bytes.  The DATA_READ_RESPONSE
path uses the device-supplied read_length to drive memcpy()
without clamping it to the remaining report size.  Add both
checks, and clamp read_length to size - 3.

> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
> index 04379db93571b..99e31724a4211 100644
> --- a/drivers/hid/hid-cp2112.c
> +++ b/drivers/hid/hid-cp2112.c

[ ... ]

> @@ -1463,12 +1468,18 @@ 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 < 4) {
> +			hid_err(hdev, "short data read response(%d < 4)\n", size);
> +			return 0;
> +		}
>  		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 = size - 3;
>  		memcpy(dev->read_data, &data[3], dev->read_length);
>  		atomic_set(&dev->read_avail, 1);

[Severity: High]
This is a pre-existing issue, but does this lockless update to dev->read_data
and dev->read_length lack required memory barriers?

In cp2112_raw_event(), dev->read_data is updated and then read_avail is
signaled locklessly:

    memcpy(dev->read_data, &data[3], dev->read_length);
    atomic_set(&dev->read_avail, 1);

However, the consumer in cp2112_wait() evaluates atomic_read() locklessly,
and cp2112_read() reads the payload without an acquire barrier:

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

cp2112_read():
    memcpy(data, dev->read_data, size);

On weakly ordered architectures, could the CPU reorder the producer's data
write after the flag set, or the consumer's data read before the flag check,
leading to stale memory reads and I2C data corruption?


[Severity: High]
This is a pre-existing issue, but does the error path in cp2112_probe()
leave the delayed work running?

drivers/hid/hid-cp2112.c:cp2112_probe() {
...
err_gpiochip_remove:
    gpiochip_remove(&dev->gc);
err_free_i2c:
...
}

If gpiochip_add_data() succeeds, the GPIO chip is exposed and its IRQ can be
immediately requested, scheduling the gpio_poll_worker.

If a subsequent initialization step fails, the probe function jumps to
err_gpiochip_remove and removes the gpiochip, but fails to cancel the
delayed work before the dev structure is freed.

Could this lead to a Use-After-Free of the dev structure?


[Severity: High]
This is a pre-existing issue, but is there a race condition in
cp2112_remove() due to premature cancellation of the delayed work?

drivers/hid/hid-cp2112.c:cp2112_remove() {
...
    if (dev->gpio_poll) {
        dev->gpio_poll = false;
        cancel_delayed_work_sync(&dev->gpio_poll_worker);
    }

    gpiochip_remove(&dev->gc);
...
}

Because the GPIO chip is still registered when the cancellation is evaluated,
a concurrent IRQ request can execute cp2112_gpio_irq_startup(), setting
dev->gpio_poll = true and scheduling the work just after the cancellation
check but before the chip is removed.

Once remove() completes and dev is freed, will the scheduled work cause a
Use-After-Free?


[Severity: High]
This is a pre-existing issue, but is there a TOCTOU race on
dev->read_length in cp2112_read()?

drivers/hid/hid-cp2112.c:cp2112_read() {
...
    if (size > dev->read_length)
        size = dev->read_length;

    memcpy(data, dev->read_data, size);
    return dev->read_length;
}

If a spurious or delayed USB CP2112_DATA_READ_RESPONSE packet is processed
concurrently by cp2112_raw_event(), dev->read_length can increase between
the bounds check and the return value.

Since cp2112_read() reads dev->read_length locklessly twice, could it copy
fewer bytes than it returns? This would cause the caller cp2112_i2c_xfer()
to advance its buffer by the larger returned value and leak uninitialized
heap memory to the userspace I2C client.


[Severity: High]
This is a pre-existing issue, but does using an interruptible wait in
cp2112_wait() allow signals to permanently desynchronize the hardware
response state?

drivers/hid/hid-cp2112.c:cp2112_wait() {
...
    ret = wait_event_interruptible_timeout(dev->wait,
        atomic_read(avail), msecs_to_jiffies(RESPONSE_TIMEOUT));
    if (-ERESTARTSYS == ret)
        return ret;
...
}

If a user process executing an I2C transaction receives a signal,
wait_event_interruptible_timeout() aborts with -ERESTARTSYS. The driver
exits, but the hardware will eventually deliver the response for the
aborted transaction.

Because the hardware lacks transaction IDs, will this late response falsely
satisfy the cp2112_wait() of the next I2C transaction, returning garbage
data or false success?

Should the driver use wait_event_timeout() for short hardware
synchronization instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714131611.269929-1-yaojiale02@163.com?part=4