[PATCH 0/2] HID: roccat: bound device-supplied profile index

[PATCH 0/2] HID: roccat: bound device-supplied profile index

[Michael Bommarito]

The Roccat Kone driver uses an 8-bit value taken straight from a USB HID
interrupt report as an index into a fixed 5-element profiles[] array,
without any range check. A malicious or counterfeit device that claims
the Roccat Kone VID/PID can send a "switch profile" report with an
out-of-range value and make the driver read out of bounds; the same
unbounded index is also reachable at probe time from a device-supplied
startup_profile field. The read result is stored in actual_dpi and
exposed to user space through the actual_dpi sysfs attribute.

Michael Bommarito (2):
  HID: roccat: bound device-supplied profile index
  HID: roccat: add KUnit test for kone profile-index bounds

 drivers/hid/Kconfig           |  9 +++++
 drivers/hid/hid-roccat-kone.c | 65 +++++++++++++++++++++++++++++++++--
 2 files changed, 72 insertions(+), 2 deletions(-)

-- 
2.53.0

[PATCH 1/2] HID: roccat: bound device-supplied profile index

[Michael Bommarito]

kone_keep_values_up_to_date() and kone_profile_activated() use an
8-bit, device-supplied profile value as an index into the 5-element
kone->profiles[] array without a range check. A malicious USB device
claiming the Roccat Kone id can send a switch-profile event (or a
startup_profile read at probe) with an out-of-range value and make the
driver read out of bounds; the result is exposed via the actual_dpi
sysfs attribute.

Reject out-of-range indices in both paths.

This was found with static analysis and confirmed with the KUnit test
added in the following patch (KASAN: slab-out-of-bounds).

Fixes: 14bf62cde7942 ("HID: add driver for Roccat Kone gaming mouse")
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 drivers/hid/hid-roccat-kone.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat-kone.c
index 58654cf78f0df..17495fcc8b7da 100644
--- a/drivers/hid/hid-roccat-kone.c
+++ b/drivers/hid/hid-roccat-kone.c
@@ -36,6 +36,8 @@ static uint profile_numbers[5] = {0, 1, 2, 3, 4};
 
 static void kone_profile_activated(struct kone_device *kone, uint new_profile)
 {
+	if (new_profile < 1 || new_profile > ARRAY_SIZE(kone->profiles))
+		new_profile = 1;
 	kone->actual_profile = new_profile;
 	kone->actual_dpi = kone->profiles[new_profile - 1].startup_dpi;
 }
@@ -793,8 +795,10 @@ static void kone_keep_values_up_to_date(struct kone_device *kone,
 {
 	switch (event->event) {
 	case kone_mouse_event_switch_profile:
-		kone->actual_dpi = kone->profiles[event->value - 1].
-				startup_dpi;
+		if (event->value >= 1 &&
+		    event->value <= ARRAY_SIZE(kone->profiles))
+			kone->actual_dpi =
+				kone->profiles[event->value - 1].startup_dpi;
 		fallthrough;
 	case kone_mouse_event_osd_profile:
 		kone->actual_profile = event->value;
-- 
2.53.0

[PATCH 2/2] HID: roccat: add KUnit test for kone profile-index bounds

[Michael Bommarito]

Drive kone_keep_values_up_to_date() with a crafted switch-profile event;
an out-of-range value reads past profiles[] (KASAN slab-out-of-bounds on
an unpatched tree). A benign control with an in-range value exercises the
same path. The test object is sized to end at profiles[] so the over-read
lands in the KASAN redzone.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 drivers/hid/Kconfig           |  9 ++++++
 drivers/hid/hid-roccat-kone.c | 57 +++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index ff2f580b660ba..3c6bc918aeb54 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -1057,6 +1057,15 @@ config HID_ROCCAT
 	Say Y here if you have a Roccat mouse or keyboard and want
 	support for its special functionalities.
 
+config HID_ROCCAT_KONE_KUNIT_TEST
+	bool "KUnit tests for the Roccat Kone driver" if !KUNIT_ALL_TESTS
+	depends on HID_ROCCAT=y && KUNIT=y
+	default KUNIT_ALL_TESTS
+	help
+	  Enable the KUnit regression tests for the Roccat Kone driver,
+	  covering bounds checking of device-supplied profile indices.
+	  If unsure, say N.
+
 config HID_SAITEK
 	tristate "Saitek (Mad Catz) non-fully HID-compliant devices"
 	help
diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat-kone.c
index 17495fcc8b7da..3dae9eaa0b6f4 100644
--- a/drivers/hid/hid-roccat-kone.c
+++ b/drivers/hid/hid-roccat-kone.c
@@ -919,3 +919,60 @@ module_exit(kone_exit);
 MODULE_AUTHOR("Stefan Achatz");
 MODULE_DESCRIPTION("USB Roccat Kone driver");
 MODULE_LICENSE("GPL v2");
+
+#if IS_ENABLED(CONFIG_HID_ROCCAT_KONE_KUNIT_TEST)
+#include <kunit/test.h>
+
+/*
+ * Regression test for the out-of-bounds read in
+ * kone_keep_values_up_to_date(): a malicious USB device sends a
+ * "switch profile" HID event (event == kone_mouse_event_switch_profile)
+ * with an attacker-chosen value in 0..255, which is used unbounded as
+ * profiles[value - 1].  On an unpatched kernel the attack case triggers a
+ * KASAN slab-out-of-bounds read; the fix must leave actual_dpi unchanged.
+ */
+static void kone_profile_index_oob_test(struct kunit *test)
+{
+	struct kone_device *kone;
+	struct kone_mouse_event ev = {};
+	/*
+	 * Allocate only up to the end of profiles[] so that any index past
+	 * the 5-element array is IMMEDIATELY out of bounds and lands in the
+	 * KASAN redzone (a far over-read would hit unrelated valid memory and
+	 * escape KASAN).
+	 */
+	size_t sz = offsetof(struct kone_device, profiles) +
+		    sizeof(kone->profiles);
+
+	kone = kunit_kzalloc(test, sz, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, kone);
+	kone->profiles[0].startup_dpi = 0x42;
+
+	/* benign control: a valid in-range value drives the SAME path and
+	 * must succeed (proves the trigger reaches the real code).
+	 */
+	ev.event = kone_mouse_event_switch_profile;
+	ev.value = 1;
+	kone_keep_values_up_to_date(kone, &ev);
+	KUNIT_EXPECT_EQ(test, kone->actual_dpi, 0x42);
+
+	/* attack: value == ARRAY_SIZE(profiles) + 1 reads profiles[5], one
+	 * element past the array end -> KASAN slab-out-of-bounds read on an
+	 * unpatched kernel. The fix must reject it (actual_dpi unchanged).
+	 */
+	ev.value = ARRAY_SIZE(kone->profiles) + 1;
+	kone_keep_values_up_to_date(kone, &ev);
+	KUNIT_EXPECT_EQ(test, kone->actual_dpi, 0x42);
+}
+
+static struct kunit_case kone_test_cases[] = {
+	KUNIT_CASE(kone_profile_index_oob_test),
+	{}
+};
+
+static struct kunit_suite kone_test_suite = {
+	.name = "hid-roccat-kone",
+	.test_cases = kone_test_cases,
+};
+kunit_test_suite(kone_test_suite);
+#endif /* CONFIG_HID_ROCCAT_KONE_KUNIT_TEST */
-- 
2.53.0

Re: [PATCH 2/2] HID: roccat: add KUnit test for kone profile-index bounds

[sashiko-bot]

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

Pre-existing issues:
- [High] A Use-After-Free vulnerability occurs because the `kone_device` structure is freed while hardware interrupts are still active, leaving a dangling pointer in `hid_drvdata`.
- [High] Uninitialized kernel memory is copied to the persistent `kone->settings` structure on a failed USB control message, leading to a heap information leak via sysfs.
--

commit e8aee21a8be54aaf776b4d76c32eeaefd255b5aa
Author: Michael Bommarito <michael.bommarito@gmail.com>

HID: roccat: add KUnit test for kone profile-index bounds

This commit adds a KUnit test for the Roccat Kone driver to verify bounds
checking on profile indices. It simulates a crafted switch-profile event to
ensure the driver does not read out-of-bounds.

> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index ff2f580b660ba..3c6bc918aeb54 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -1057,6 +1057,15 @@ config HID_ROCCAT
>  	Say Y here if you have a Roccat mouse or keyboard and want
>  	support for its special functionalities.
>  
> +config HID_ROCCAT_KONE_KUNIT_TEST
> +	bool "KUnit tests for the Roccat Kone driver" if !KUNIT_ALL_TESTS
> +	depends on HID_ROCCAT=y && KUNIT=y
> +	default KUNIT_ALL_TESTS
> +	help
> +	  Enable the KUnit regression tests for the Roccat Kone driver,
> +	  covering bounds checking of device-supplied profile indices.
> +	  If unsure, say N.
> +
>  config HID_SAITEK
>  	tristate "Saitek (Mad Catz) non-fully HID-compliant devices"
>  	help
> diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat-kone.c
> index 17495fcc8b7da..3dae9eaa0b6f4 100644
> --- a/drivers/hid/hid-roccat-kone.c
> +++ b/drivers/hid/hid-roccat-kone.c

[ ... ]

> @@ -919,3 +919,60 @@ module_exit(kone_exit);
>  MODULE_AUTHOR("Stefan Achatz");
>  MODULE_DESCRIPTION("USB Roccat Kone driver");
>  MODULE_LICENSE("GPL v2");
> +
> +#if IS_ENABLED(CONFIG_HID_ROCCAT_KONE_KUNIT_TEST)
> +#include <kunit/test.h>
> +
> +/*
> + * Regression test for the out-of-bounds read in
> + * kone_keep_values_up_to_date(): a malicious USB device sends a
> + * "switch profile" HID event (event == kone_mouse_event_switch_profile)
> + * with an attacker-chosen value in 0..255, which is used unbounded as
> + * profiles[value - 1].  On an unpatched kernel the attack case triggers a
> + * KASAN slab-out-of-bounds read; the fix must leave actual_dpi unchanged.
> + */
> +static void kone_profile_index_oob_test(struct kunit *test)
> +{
> +	struct kone_device *kone;
> +	struct kone_mouse_event ev = {};

[ ... ]

> +	/* attack: value == ARRAY_SIZE(profiles) + 1 reads profiles[5], one
> +	 * element past the array end -> KASAN slab-out-of-bounds read on an
> +	 * unpatched kernel. The fix must reject it (actual_dpi unchanged).
> +	 */
> +	ev.value = ARRAY_SIZE(kone->profiles) + 1;
> +	kone_keep_values_up_to_date(kone, &ev);
> +	KUNIT_EXPECT_EQ(test, kone->actual_dpi, 0x42);
> +}
> +
> +static struct kunit_case kone_test_cases[] = {
> +	KUNIT_CASE(kone_profile_index_oob_test),
> +	{}
> +};
> +
> +static struct kunit_suite kone_test_suite = {
> +	.name = "hid-roccat-kone",
> +	.test_cases = kone_test_cases,
> +};
> +kunit_test_suite(kone_test_suite);
> +#endif /* CONFIG_HID_ROCCAT_KONE_KUNIT_TEST */

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing I noticed a
potential use-after-free vulnerability in the driver initialization and
teardown paths.

In kone_probe(), hid_hw_start() is called before kone_init_specials(),
starting the hardware and enabling USB interrupts. If initialization fails,
the error path frees the driver data but does not clear the pointer:

drivers/hid/hid-roccat-kone.c:kone_init_specials() {
...
exit_free:
	kfree(kone);
	return retval;
}

Between kfree() and hid_hw_stop(), an incoming USB interrupt will trigger
kone_raw_event(), which dereferences the dangling pointer:

drivers/hid/hid-roccat-kone.c:kone_raw_event() {
...
	struct kone_device *kone = hid_get_drvdata(hdev);
...
}

The same window exists in the teardown path, where the driver state is freed
before stopping the hardware:

drivers/hid/hid-roccat-kone.c:kone_remove() {
...
	kone_remove_specials(hdev);
	hid_hw_stop(hdev);
}

Is it possible for an attacker to trigger this use-after-free by
disconnecting the device or simulating a device error while hardware
interrupts are active?

[Severity: High]
This is a pre-existing issue, but there appears to be a potential heap
information leak when copying uninitialized memory after a failed USB
transfer.

In kone_receive(), a bounce buffer is allocated with kmalloc() and
unconditionally copied to the target even if the USB transfer fails:

drivers/hid/hid-roccat-kone.c:kone_receive() {
	buf = kmalloc(size, GFP_KERNEL);
...
	retval = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0), ...
...
	memcpy(data, buf, size);
	kfree(buf);
	return retval;
}

When called via kone_get_settings(), the uninitialized kernel heap memory
is copied to the persistent settings structure. If this occurs during a
sysfs operation where the error is handled, the corrupted structure remains
intact:

drivers/hid/hid-roccat-kone.c:kone_sysfs_set_tcu() {
...
	retval = kone_get_settings(usb_dev, &kone->settings);
	if (retval)
		goto exit_no_settings;
...
}

A subsequent read of the settings sysfs attribute exposes this memory
directly to userspace:

drivers/hid/hid-roccat-kone.c:kone_sysfs_read_settings() {
...
	memcpy(buf, ((char const *)&kone->settings) + off, count);
...
}

Could a malicious USB device intentionally fail the transfer to exploit
this leak? Should we check the return value of usb_control_msg() before
calling memcpy() in kone_receive()?

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

Re: [PATCH 1/2] HID: roccat: bound device-supplied profile index

[sashiko-bot]

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

Pre-existing issues:
- [High] Closing a stale roccat character device file descriptor after device removal causes a memory leak, state corruption and Use-After-Free.
- [High] `kone_get_profile()` uses an embedded, unaligned struct field (`&kone->profiles[i]`) as a DMA buffer for inbound USB control messages, leading to cache-line sharing corruption.
--

commit cda04d8cb7b50c97cbcee76f405a2121b1b4c56d
Author: Michael Bommarito <michael.bommarito@gmail.com>

HID: roccat: bound device-supplied profile index

kone_keep_values_up_to_date() and kone_profile_activated() use an
8-bit, device-supplied profile value as an index into the 5-element
kone->profiles[] array without a range check. A malicious USB device
claiming the Roccat Kone id can send a switch-profile event (or a
startup_profile read at probe) with an out-of-range value and make the
driver read out of bounds; the result is exposed via the actual_dpi
sysfs attribute.

> diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat-kone.c
> index 58654cf78f0df..17495fcc8b7da 100644
> --- a/drivers/hid/hid-roccat-kone.c
> +++ b/drivers/hid/hid-roccat-kone.c

[ ... ]

> @@ -793,8 +795,10 @@ static void kone_keep_values_up_to_date(struct kone_device *kone,
>  {
>  	switch (event->event) {
>  	case kone_mouse_event_switch_profile:
> -		kone->actual_dpi = kone->profiles[event->value - 1].
> -				startup_dpi;
> +		if (event->value >= 1 &&
> +		    event->value <= ARRAY_SIZE(kone->profiles))
> +			kone->actual_dpi =
> +				kone->profiles[event->value - 1].startup_dpi;
>  		fallthrough;
>  	case kone_mouse_event_osd_profile:
>  		kone->actual_profile = event->value;

[Severity: High]
This is a pre-existing issue, but does kone_get_profile() use an embedded,
unaligned array element as a DMA buffer?

In kone_init_kone_device_struct(), we call kone_get_profile() passing a
pointer to the embedded array element:

drivers/hid/hid-roccat-kone.c:kone_init_kone_device_struct() {
    ...
    retval = kone_get_profile(usb_dev, &kone->profiles[i], i + 1);
    ...
}

Which then passes the unaligned buffer directly to usb_control_msg() for a
USB_DIR_IN transfer:

drivers/hid/hid-roccat-kone.c:kone_get_profile() {
    ...
    len = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
            USB_REQ_CLEAR_FEATURE,
            USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN,
            kone_command_profile, number, buf,
            sizeof(struct kone_profile), USB_CTRL_SET_TIMEOUT);
    ...
}

Since sizeof(struct kone_profile) is 975 bytes, the array elements are not
cache-line aligned and share cache lines with other struct kone_device
members. Because hid_hw_start() is called earlier, the device can generate
interrupts and concurrent writes to these shared fields may occur.

On architectures with non-coherent DMA caches, won't the cache invalidation
required for the inbound USB DMA wipe out these concurrent CPU writes,
leading to state corruption?


[Severity: High]
This is a pre-existing issue, but can roccat_release() cause a memory leak,
list corruption, and Use-After-Free if a stale file descriptor is closed
after device removal?

When closing the file descriptor, roccat_release() retrieves the device from
the global array instead of using the local reader->device:

drivers/hid/hid-roccat.c:roccat_release() {
    ...
    device = devices[minor];
    if (!device) {
        mutex_unlock(&devices_lock);
        pr_emerg("roccat device with minor %d doesn't exist\n", minor);
        return -ENODEV;
    }
    ...
}

If the original device was disconnected, devices[minor] is cleared, causing
roccat_release() to return -ENODEV and permanently leak the roccat_reader
allocation and the original roccat_device.

If a new device is plugged in before the stale file descriptor is closed,
devices[minor] resolves to the new device. roccat_release() then operates on
the new device, locking its readers_lock while modifying the old device's
list:

drivers/hid/hid-roccat.c:roccat_release() {
    ...
    list_del(&reader->node);
    ...
    if (!--device->open) {
    ...
}

Could this lead to list corruption and incorrectly decrement the new device's
open count, causing premature shutdown and a Use-After-Free of the new device
when legitimate users close their descriptors?

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