Re: [PATCH 1/4] HID: cougar: fix out-of-bounds read in cougar_raw_event

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Unbounded error logging in interrupt context can lead to log flooding and Denial of Service.
- [Low] The newly added `hid_err` log message is missing a terminating newline character.

Pre-existing issues:
- [High] TOCTOU race condition during device unbinding leads to a Use-After-Free of the keyboard's input_dev (`shared->input`).
- [High] Self-deadlock on `cougar_udev_list_lock` during error handling in `cougar_bind_shared_data()`.
- [High] Race condition between `kref_put()` and list lookup causing Use-After-Free of the `cougar_shared` structure.
- [High] The probe function dereferences `hdev->collection` without verifying that any collections were actually parsed.
--

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

HID: cougar: fix out-of-bounds read in cougar_raw_event

This patch adds a bounds check to cougar_raw_event() to prevent out-of-bounds
reads when handling malformed HID reports shorter than 3 bytes.

> --- a/drivers/hid/hid-cougar.c
> +++ b/drivers/hid/hid-cougar.c
> @@ -270,6 +270,12 @@ static int cougar_raw_event(struct hid_device *hdev, struct hid_report *report,
>  	if (!shared->enabled || !shared->input)
>  		return -EPERM;

[Severity: High]
This is a pre-existing issue, but is there a TOCTOU race condition here
during device unbinding?

If a device disconnect is triggered while a USB event is being processed,
cougar_raw_event() checks shared->enabled locklessly on one CPU.
Concurrently, cougar_remove() on another CPU could clear shared->enabled
and call hid_hw_stop(), destroying the input_dev.

Could the interrupt handler then resume and dereference the freed
shared->input in input_event()?

> +	if (size < 3) {
> +		hid_err(hdev, "Received HID report of bad size (%d)",
> +			size);

[Severity: Medium]
Could this un-ratelimited error logging lead to a log flood if a
malfunctioning or malicious device continuously sends reports shorter than
3 bytes?

Since cougar_raw_event() runs in interrupt context, this might cause high
CPU usage and flood the kernel ring buffer.

[Severity: Low]
Also, does this log message need a terminating newline character?
Without the \n, subsequent kernel messages might be incorrectly appended
to the same line.

> +		return -EPERM;
> +	}
> +
>  	code = data[COUGAR_FIELD_CODE];

[ ... ]

[Severity: High]
This is a pre-existing issue, but does cougar_bind_shared_data() have a
self-deadlock on cougar_udev_list_lock during error handling?

If devm_add_action_or_reset() fails, it invokes cougar_remove_shared_data()
which drops the kref to zero, triggering cougar_release_shared_data():

drivers/hid/hid-cougar.c:cougar_release_shared_data() {
    ...
    mutex_lock(&cougar_udev_list_lock);
    ...
}

Since cougar_bind_shared_data() already holds cougar_udev_list_lock when
calling devm_add_action_or_reset(), will this attempt to re-acquire the
already held mutex block all further probes?

[Severity: High]
This is a pre-existing issue, but is there a race condition between
kref_put() and list lookup that could cause a use-after-free of the
cougar_shared structure?

During device unbind, cougar_remove_shared_data() calls
kref_put(&shared->kref) without holding cougar_udev_list_lock. If the
refcount drops to 0, cougar_release_shared_data() blocks on the lock.

Concurrently, a new device probe calling cougar_bind_shared_data() holds
the lock and traverses the list:

drivers/hid/hid-cougar.c:cougar_bind_shared_data() {
    ...
    mutex_lock(&cougar_udev_list_lock);
    list_for_each_entry(shared, &cougar_udev_list, list) {
        ...
        kref_get(&shared->kref);
        ...
    }
}

Could it find the zero-refcount object, increment it, and then after the lock
is dropped, the blocked release function frees it, leaving the newly
probed device with a dangling pointer?

[Severity: High]
This is a pre-existing issue, but does the probe function dereference
hdev->collection without verifying that any collections were actually
parsed?

drivers/hid/hid-cougar.c:cougar_probe() {
    ...
    if (hdev->collection->usage == COUGAR_VENDOR_USAGE) {
    ...
}

If a USB device supplies a HID report descriptor containing no collections,
hid_parse() succeeds but hdev->collection remains NULL. Will this
unconditional access cause a panic on probe?

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