Re: [PATCH 2/4] HID: cougar: reject short vendor reports

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] `cougar_raw_event()` accesses `shared->input` without synchronization against `cougar_remove()`, leading to a potential Use-After-Free if the keyboard interface is unbound.
- [High] A deadlock occurs if `devm_add_action_or_reset()` fails during probe.
- [High] A race condition between `kref_put()` and list lookup leads to a Use-After-Free of `cougar_shared`.
--

commit 27f66ec84e2d69ee2a0830cfc45515ced43d2bb5
Author: Yousef Alhouseen <alhouseenyousef@gmail.com>

    HID: cougar: reject short vendor reports

This commit fixes an out-of-bounds read in cougar_raw_event() by ensuring
that vendor-interface reports are large enough before parsing them. Short
reports are now properly rejected with an error.

> diff --git a/drivers/hid/hid-cougar.c b/drivers/hid/hid-cougar.c
> index ad027c45f1628..7156658166f51 100644
> --- a/drivers/hid/hid-cougar.c
> +++ b/drivers/hid/hid-cougar.c
> @@ -270,6 +270,9 @@ 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 cougar_raw_event() accessing shared->input
without synchronization against cougar_remove()?

If a thread is preempted right after checking shared->enabled and shared->input,
and cougar_remove() runs concurrently, it will set shared->enabled to false and
call hid_hw_stop(). This destroys and frees the input_dev structure.

When the first thread resumes, wouldn't it pass the freed shared->input to
input_event(), resulting in a Use-After-Free?

> +	if (size <= COUGAR_FIELD_ACTION)
> +		return -EPERM;
> +
>  	code = data[COUGAR_FIELD_CODE];
>  	action = data[COUGAR_FIELD_ACTION];

[ ... ]

While reviewing this file, I noticed a couple of other pre-existing issues in
functions outside of this patch.

drivers/hid/hid-cougar.c:cougar_bind_shared_data() {
...
	mutex_lock(&cougar_udev_list_lock);
...
	error = devm_add_action_or_reset(&hdev->dev, cougar_remove_shared_data, cougar);
...
}

[Severity: High]
This isn't a bug introduced by this patch, but could this create a deadlock if
devm_add_action_or_reset() fails during probe?

If it fails, it synchronously executes the cleanup callback
cougar_remove_shared_data(), which calls kref_put(). If the refcount drops to 0,
cougar_release_shared_data() is called and attempts to acquire
cougar_udev_list_lock. Since that lock is already held by
cougar_bind_shared_data(), this would cause a deadlock.

drivers/hid/hid-cougar.c:cougar_remove_shared_data() {
...
	if (cougar->shared) {
		kref_put(&cougar->shared->kref, cougar_release_shared_data);
		cougar->shared = NULL;
	}
...
}

[Severity: High]
This is also a pre-existing issue, but might there be a race condition between
kref_put() and the list lookup?

The refcount is decremented to zero without holding cougar_udev_list_lock.
Before the object is removed from the list in cougar_release_shared_data(), a
concurrent call to cougar_get_shared_data() might iterate the list and use
kref_get() on this object while it is pending deletion.

Would using kref_get_unless_zero() during the lookup, or holding the list lock
during kref_put(), prevent this Use-After-Free?

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