[PATCH 1/4] HID: elo: ignore short touch reports

[PATCH 1/4] HID: elo: ignore short touch reports

[Yousef Alhouseen]

elo_process_data() reads coordinates, flags, and pressure through data[7].
The raw-event callback only checks the packet marker, so a malformed USB
device can submit a one-byte marker report and trigger out-of-bounds
reads from the input buffer.

Only process touch packets that contain all eight protocol bytes.

Fixes: d23efc19478a ("HID: add driver for ELO 4000/4500")
Cc: stable@vger.kernel.org
Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
 drivers/hid/hid-elo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-elo.c b/drivers/hid/hid-elo.c
index b8f5f3eb53a4..1aeec712c67b 100644
--- a/drivers/hid/hid-elo.c
+++ b/drivers/hid/hid-elo.c
@@ -89,7 +89,7 @@ static int elo_raw_event(struct hid_device *hdev, struct hid_report *report,
 
 	switch (report->id) {
 	case 0:
-		if (data[0] == 'T') {	/* Mandatory ELO packet marker */
+		if (size >= 8 && data[0] == 'T') { /* Mandatory ELO packet marker */
 			elo_process_data(hidinput->input, data, size);
 			return 1;
 		}
-- 
2.54.0

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

[Yousef Alhouseen]

cougar_raw_event() reads the key code and action from bytes one and two
of every enabled vendor-interface report. A malformed USB device can
send a shorter report and make both accesses run beyond the received
buffer.

Consume short vendor reports without parsing them.

Fixes: b8e759b8f6da ("HID: cougar: Add support for the Cougar 500k Gaming Keyboard")
Cc: stable@vger.kernel.org
Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
 drivers/hid/hid-cougar.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/hid-cougar.c b/drivers/hid/hid-cougar.c
index ad027c45f162..7156658166f5 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;
 
+	if (size <= COUGAR_FIELD_ACTION)
+		return -EPERM;
+
 	code = data[COUGAR_FIELD_CODE];
 	action = data[COUGAR_FIELD_ACTION];
 	for (i = 0; cougar_mapping[i][0]; i++) {
-- 
2.54.0

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

[sashiko-bot]

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

[PATCH 3/4] HID: zydacron: reject short key reports

[Yousef Alhouseen]

The raw-event handler reads data[1] for report IDs 2 and 3 without
checking that the report includes a payload byte. A malformed USB device
can submit a report containing only the report ID and trigger an
out-of-bounds read.

Ignore the key value when the payload byte is missing.

Fixes: d0742abaa1c3 ("HID: add omitted hid-zydacron.c file")
Cc: stable@vger.kernel.org
Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
 drivers/hid/hid-zydacron.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/hid-zydacron.c b/drivers/hid/hid-zydacron.c
index 1aae80f848f5..b882d2a0ba1a 100644
--- a/drivers/hid/hid-zydacron.c
+++ b/drivers/hid/hid-zydacron.c
@@ -129,6 +129,9 @@ static int zc_raw_event(struct hid_device *hdev, struct hid_report *report,
 		switch (report->id) {
 		case 0x02:
 		case 0x03:
+			if (size < 2)
+				break;
+
 			switch (data[1]) {
 			case 0x10:
 				key = KEY_MODE;
-- 
2.54.0

[PATCH 4/4] HID: roccat-savu: reject short special reports

[Yousef Alhouseen]

savu_report_to_chrdev() casts special reports to a five-byte structure and
reads all of its payload fields without checking the received size. A
malformed USB device can therefore trigger out-of-bounds reads from the
input buffer when the character device is claimed.

Pass the report size into the helper and require the complete structure.

Fixes: 6a2a6390cf09 ("HID: roccat: add support for Roccat Savu")
Cc: stable@vger.kernel.org
Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
 drivers/hid/hid-roccat-savu.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-roccat-savu.c b/drivers/hid/hid-roccat-savu.c
index 679136933560..04fa4c50cfa4 100644
--- a/drivers/hid/hid-roccat-savu.c
+++ b/drivers/hid/hid-roccat-savu.c
@@ -152,12 +152,13 @@ static void savu_remove(struct hid_device *hdev)
 }
 
 static void savu_report_to_chrdev(struct roccat_common2_device const *savu,
-		u8 const *data)
+		u8 const *data, int size)
 {
 	struct savu_roccat_report roccat_report;
 	struct savu_mouse_report_special const *special_report;
 
-	if (data[0] != SAVU_MOUSE_REPORT_NUMBER_SPECIAL)
+	if (data[0] != SAVU_MOUSE_REPORT_NUMBER_SPECIAL ||
+	    size < sizeof(*special_report))
 		return;
 
 	special_report = (struct savu_mouse_report_special const *)data;
@@ -183,7 +184,7 @@ static int savu_raw_event(struct hid_device *hdev,
 		return 0;
 
 	if (savu->roccat_claimed)
-		savu_report_to_chrdev(savu, data);
+		savu_report_to_chrdev(savu, data, size);
 
 	return 0;
 }
-- 
2.54.0

Re: [PATCH 4/4] HID: roccat-savu: reject short special reports

[sashiko-bot]

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

Pre-existing issues:
- [High] The driver acquires a mutex in the raw_event handler path, which executes in atomic (softIRQ) context, leading to a scheduling-while-atomic bug or deadlock.
- [Medium] The roccat driver leaks the memory allocated for circular buffer reports when the device is destroyed.
--

commit 60d1ec57aac96f7ad83f249887da4a3aa3a3f7c4
Author: Yousef Alhouseen <alhouseenyousef@gmail.com>

HID: roccat-savu: reject short special reports

savu_report_to_chrdev() casts special reports to a five-byte structure and
reads all of its payload fields without checking the received size. A
malformed USB device can therefore trigger out-of-bounds reads from the
input buffer when the character device is claimed. Pass the report size
into the helper and require the complete structure.

> diff --git a/drivers/hid/hid-roccat-savu.c b/drivers/hid/hid-roccat-savu.c
> index 6791369335601..04fa4c50cfa4b 100644
> --- a/drivers/hid/hid-roccat-savu.c
> +++ b/drivers/hid/hid-roccat-savu.c

[ ... ]

> @@ -183,7 +184,7 @@ static int savu_raw_event(struct hid_device *hdev,
>  		return 0;
>  
>  	if (savu->roccat_claimed)
> -		savu_report_to_chrdev(savu, data);
> +		savu_report_to_chrdev(savu, data, size);

[Severity: High]
This is a pre-existing issue, but can this lead to a scheduling-while-atomic
bug?

The savu_raw_event() callback runs in atomic context (softIRQ). When it
calls savu_report_to_chrdev(), the execution path proceeds to
roccat_report_event() in drivers/hid/hid-roccat.c, which acquires
sleeping locks:

drivers/hid/hid-roccat.c:roccat_report_event() {
    ...
    mutex_lock(&device->readers_lock);
    mutex_lock(&device->cbuf_lock);
    ...
}


[Severity: Medium]
This is also a pre-existing issue, but does this code leak the memory
allocated for circular buffer reports when the device is destroyed?

In drivers/hid/hid-roccat.c:roccat_report_event(), kmemdup() dynamically
allocates memory for the report:

    new_value = kmemdup(data, device->report_size, GFP_ATOMIC);

However, when the device is freed in roccat_release() or
roccat_disconnect() using kfree(device), the dynamically allocated report
values in the circular buffer do not appear to be cleaned up.

>  
>  	return 0;
>  }

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