Re: [PATCH v6 2/9] HID: steelseries: Split Arctis headset driver into separate module

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] If `steelseries_headset_battery_register()` fails, the probe function ignores the error and continues. This leaves `sd->battery` as an `ERR_PTR` and `sd->battery_work` uninitialized. Later, incoming events in `steelseries_arctis_raw_event()` call `power_supply_changed(sd->battery)` and `schedule_delayed_work(&sd->battery_work, ...)`, which will dereference the error pointer and operate on an uninitialized work structure, causing a kernel panic. Similarly, `steelseries_arctis_remove()` calls `cancel_delayed_work_sync(&sd->battery_work)` which will also crash.
- [High] The driver calls `hid_hw_start()` and `hid_hw_open()` before calling `steelseries_headset_battery_register()`. `hid_hw_open()` starts the URB polling, enabling asynchronous hardware events to trigger `steelseries_arctis_raw_event()` immediately. At this point, `sd->battery` is still `NULL` and `sd->battery_work` is uninitialized. If a report is received during this race window, the event handler will pass a `NULL` pointer to `power_supply_changed()` and call `schedule_delayed_work()` on uninitialized work, resulting in an immediate kernel panic.
- [High] Missing length check on the report descriptor before accessing payload offsets in `steelseries_is_vendor_usage_page()`. The function unconditionally accesses `hdev->rdesc[0]`, `hdev->rdesc[1]`, and `hdev->rdesc[2]`. Although `usbhid_parse()` guarantees `rsize > 0`, it does not guarantee `rsize >= 3`. If a malformed USB device presents a 1- or 2-byte descriptor, this reads out of bounds of the kmalloc'd `rdesc` buffer.
--

--- Patch [2]: [PATCH v6 2/9] HID: steelseries: Split Arctis headset driver into separate module ---
commit 5114a40b6cbe9d3f2ea95a54f76bb50da5fb5d42
Author: Sriman Achanta <srimanachanta@gmail.com>

HID: steelseries: Split Arctis headset driver into separate module

This patch moves the Arctis headset code from hid-steelseries.c into a
new hid-steelseries-arctis.c module. It separates the headset handling
from the SRWS1 racing wheel code.

> diff --git a/drivers/hid/hid-steelseries-arctis.c b/drivers/hid/hid-steelseries-arctis.c
> new file mode 100644
> index 0000000000000..079504e6932a1
> --- /dev/null
> +++ b/drivers/hid/hid-steelseries-arctis.c

[ ... ]

> +static bool steelseries_is_vendor_usage_page(struct hid_device *hdev, uint8_t usage_page)
> +{
> +	return hdev->rdesc[0] == 0x06 &&
> +		hdev->rdesc[1] == usage_page &&
> +		hdev->rdesc[2] == 0xff;
> +}

[Severity: High]
This is a pre-existing issue, but does this function properly validate
the size of the report descriptor before accessing it?

The function unconditionally accesses the first 3 bytes of hdev->rdesc.
Although hid_parse() guarantees rsize > 0, if a malformed USB device
presents a 1- or 2-byte descriptor, wouldn't this read out of bounds
of the allocated rdesc buffer?

[ ... ]

> +static int steelseries_arctis_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{

[ ... ]

> +	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> +	if (ret)
> +		return ret;
> +
> +	ret = hid_hw_open(hdev);
> +	if (ret)
> +		return ret;

[Severity: High]
This isn't a bug introduced by this patch, but does calling hid_hw_open()
before steelseries_headset_battery_register() introduce a race condition?

Calling hid_hw_open() starts the hardware polling, which enables
asynchronous events to immediately trigger steelseries_arctis_raw_event().
At this point, sd->battery is still NULL and sd->battery_work is
uninitialized.

If a report is received during this race window, would the event handler
pass a NULL pointer to power_supply_changed() and schedule uninitialized
work, resulting in a crash?

> +
> +	if (steelseries_headset_battery_register(sd) < 0)
> +		hid_err(sd->hdev,
> +			"Failed to register battery for headset\n");
> +
> +	return ret;
> +}

[Severity: High]
This is a pre-existing issue, but is it safe to ignore the return value
of steelseries_headset_battery_register() here?

If registration fails (for example, due to a duplicate name), the probe
function prints an error but still succeeds. This leaves sd->battery as
an ERR_PTR and sd->battery_work uninitialized.

If a subsequent event is processed by steelseries_arctis_raw_event(),
it will call power_supply_changed() and dereference the error pointer.
Furthermore, steelseries_arctis_remove() will call
cancel_delayed_work_sync() on an uninitialized structure, which can
crash the workqueue.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709032316.1908460-1-68172138+srimanachanta@users.noreply.github.com?part=2