Re: [PATCH v4 05/10] HID: steelseries: Refactor Arctis driver to use device_info framework

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing report ID signature validation for Arctis 1 causes arbitrary raw events to corrupt headset state.
- [High] Battery status requests are incorrectly sent as Feature reports instead of Output reports, breaking battery polling.
- [Medium] Dropped logic for marking Arctis 9 headset as disconnected when it reports an unknown status.

Pre-existing issues:
- [High] The raw_event interrupt handler can dereference an ERR_PTR due to a TOCTOU race window when battery registration fails.
--

commit 0576cf59ec529dc31ed68f59a8d598bdb4ca4a82
Author: Sriman Achanta <srimanachanta@gmail.com>

HID: steelseries: Refactor Arctis driver to use device_info framework

Replace the per-product if/else quirk bitmap with a steelseries_device_info
struct. Each model provides its capabilities, sync_interface, and
request_status/parse_status callbacks.

> diff --git a/drivers/hid/hid-steelseries-arctis.c b/drivers/hid/hid-steelseries-arctis.c
> index 079504e6932a1..f00f4c5e6d9e4 100644
> --- a/drivers/hid/hid-steelseries-arctis.c
> +++ b/drivers/hid/hid-steelseries-arctis.c
[ ... ]
> @@ -15,70 +16,95 @@
[ ... ]
> +static int steelseries_arctis_1_request_status(struct hid_device *hdev)
> +{
> +	const u8 data[] = { 0x06, 0x12 };
> +
> +	return steelseries_send_feature_report(hdev, data, sizeof(data));
> +}
> +
> +static int steelseries_arctis_9_request_status(struct hid_device *hdev)
> +{
> +	const u8 data[] = { 0x00, 0x20 };
> +
> +	return steelseries_send_feature_report(hdev, data, sizeof(data));
> +}

[Severity: High]
Do these devices expect feature reports instead of output reports for status
requests?

The original code sent the periodic battery status request to the device
using an output report via hid_hw_raw_request(..., HID_OUTPUT_REPORT, ...).
This refactoring switches it to use steelseries_send_feature_report()
instead.

Sending a feature report to a device endpoint expecting an output report
might result in a USB stall, which would break battery polling.

[ ... ]
> @@ -88,30 +114,96 @@ static int battery_capacity_to_level(int capacity)
[ ... ]
> +static void steelseries_arctis_1_parse_status(struct steelseries_device *sd,
> +					      u8 *data, int size)
> +{
> +	if (size < 4)
> +		return;
> +
> +	sd->headset_connected = (data[2] != 0x01);
> +	sd->battery_capacity = data[3];
> +}

[Severity: High]
Will this corrupt the battery state if other reports are received?

The original raw event handler verified that the incoming Arctis 1 report
matched the expected battery status signature using memcmp().

Since steelseries_arctis_raw_event() now invokes parse_status()
unconditionally for all incoming raw events, could a different report
(such as volume control) with a payload of at least 4 bytes be erroneously
parsed as battery data?

> +static void steelseries_arctis_9_parse_status(struct steelseries_device *sd,
> +					      u8 *data, int size)
> +{
> +	if (size < 5)
> +		return;
> +
> +	if (data[0] == 0xaa) {
> +		sd->headset_connected = (data[1] == 0x01);
> +		sd->battery_charging = (data[4] == 0x01);
> +		sd->battery_capacity = steelseries_map_capacity(data[3], 0x68, 0x9d);
> +	}
> +}

[Severity: Medium]
What happens if the device is turned off and sends the "no known status"
packet?

The original code explicitly handled the 0x55 response and used it to mark
the headset as disconnected.

By only processing 0xaa and dropping other responses without taking any
action, will we fail to update the connection state and leave the device
incorrectly reported as connected?

[ ... ]
> @@ -168,7 +245,26 @@ static enum power_supply_property steelseries_headset_battery_props[] = {
[ ... ]
> +static int steelseries_battery_register(struct steelseries_device *sd)
>  {
[ ... ]
>  	sd->battery = devm_power_supply_register(&sd->hdev->dev,
>  			&sd->battery_desc, &battery_cfg);
>  	if (IS_ERR(sd->battery)) {
>  		ret = PTR_ERR(sd->battery);
> +		sd->battery = NULL;

[Severity: High]
This isn't a bug introduced by this patch, but does assigning the return
value directly to sd->battery before checking for an error create a race
condition?

Since hid_hw_start() and hid_hw_open() are already called in the probe
function, hardware interrupts are active. If power_supply_register() fails,
sd->battery temporarily holds an error pointer.

If steelseries_arctis_raw_event() triggers during this small window, the
following check would evaluate to true because an error pointer is not NULL:

steelseries_arctis_raw_event() {
    ...
    if (sd->battery)
        power_supply_changed(sd->battery);
}

Could this lead to an invalid pointer dereference and a kernel panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623172310.272708-1-srimanachanta@gmail.com?part=5