Re: [PATCH v2 4/4] hwmon: (nzxt-kraken3) Stop device IO before calling hid_hw_stop

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing hwmon_lock() serialization for sysfs extra_groups attributes in kraken3_fan_curve_pwm_store.
- [High] Insufficient bounds checking in kraken3_raw_event leads to an out-of-bounds read for Z53 devices.
- [High] Probe error paths in multiple other HID drivers fail to call `hid_device_io_stop(hdev)`, leaving IO enabled and leading to UAF vulnerabilities.
- [Medium] Using `spin_lock_bh()` in process context while the same lock is acquired with `spin_lock()` in a hardirq-capable completion handler leads to deadlocks.
- [Medium] Sensor readings, update timestamps, and firmware version arrays are modified by the interrupt handler locklessly and read by sysfs/debugfs callbacks locklessly.
--

--- Patch [4]: [PATCH v2 4/4] hwmon: (nzxt-kraken3) Stop device IO before calling hid_hw_stop ---
commit 56b3edbc8e234031a68a61227f708d801053d8df
Author: Guenter Roeck <linux@roeck-us.net>

hwmon: (nzxt-kraken3) Stop device IO before calling hid_hw_stop

Calling hid_hw_stop() does not stop the device IO, which can result
in a use-after-free race condition between hid_input_report() and probe
failure. This fix adds hid_device_io_stop() before hid_hw_stop() to
prevent the vulnerability.

> diff --git a/drivers/hwmon/nzxt-kraken3.c b/drivers/hwmon/nzxt-kraken3.c
> index d00409bcab93a..05525406c5fbb 100644
> --- a/drivers/hwmon/nzxt-kraken3.c
> +++ b/drivers/hwmon/nzxt-kraken3.c
> @@ -960,13 +960,15 @@ static int kraken3_probe(struct hid_device *hdev, const struct hid_device_id *id
[ ... ]
>  	return 0;
>  
> +fail_and_stop_io:
> +	hid_device_io_stop(hdev);
>  fail_and_close:
>  	hid_hw_close(hdev);
>  fail_and_stop:
>  	hid_hw_stop(hdev);
>  	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but do other HID drivers share this same
missing cleanup?

Looking at corsairpsu_probe() in drivers/hwmon/corsair-psu.c:

fail_and_close:
	hid_hw_close(hdev);
fail_and_stop:
	hid_hw_stop(hdev);
	return ret;

And mcp2221_probe() in drivers/hid/hid-mcp2221.c:

	ret = mcp_set_i2c_speed(mcp);
	if (ret) {
		hid_err(hdev, "can't set i2c speed: %d\n", ret);
		return ret;
	}

And nintendo_hid_probe() in drivers/hid/hid-nintendo.c:

err_close:
	hid_hw_close(hdev);
err_stop:
	hid_hw_stop(hdev);

Could these error paths also leave IO enabled and trigger a similar UAF
vulnerability if the HID core processes events after devres frees the
private data?

[Severity: High]
This isn't a bug introduced by this patch, but does the custom attribute
store function lack hwmon_lock() serialization?

drivers/hwmon/nzxt-kraken3.c:kraken3_fan_curve_pwm_store() {
...
		ret =
		    kraken3_write_curve(priv,
					priv->channel_info[dev_attr->nr].pwm_points, dev_attr->nr);
...
}

The hwmon core only serializes attributes registered via the info
parameter. For attributes registered via extra_groups, can modifying
the fan curve arrays without hwmon_lock() or priv->buffer_lock allow
concurrent sysfs writes to race with the core hwmon write callback and
corrupt device state?

[Severity: High]
This is a pre-existing issue, but does kraken3_raw_event() have
sufficient bounds checking for Z53 devices?

drivers/hwmon/nzxt-kraken3.c:kraken3_raw_event() {
...
		priv->fan_input[1] = get_unaligned_le16(data + Z53_FAN_SPEED_OFFSET);
		priv->channel_info[1].reported_duty =
		    kraken3_percent_to_pwm(data[Z53_FAN_DUTY_OFFSET]);
...
}

The function checks if the size is at least MIN_REPORT_LENGTH (20
bytes). If a report is exactly 20 bytes, does unconditionally accessing
offsets 23, 24, and 25 cause an out-of-bounds read?

[Severity: Medium]
This isn't a bug introduced by this patch, but is there a deadlock risk
between kraken3_read_z53() and kraken3_raw_event()?

kraken3_read_z53() uses spin_lock_bh():
	spin_lock_bh(&priv->status_completion_lock);

kraken3_raw_event() uses spin_lock():
	spin_lock(&priv->status_completion_lock);

On systems where legacy USB host controllers execute URB givebacks in
hardirq context, could the interrupt handler attempt to acquire the
already held spinlock and deadlock on the local CPU?

[Severity: Medium]
This is a pre-existing issue, but are sensor readings modified
locklessly?

drivers/hwmon/nzxt-kraken3.c:kraken3_raw_event() {
...
	/* Temperature and fan sensor readings */
	priv->temp_input[0] =
	    data[TEMP_SENSOR_START_OFFSET] * 1000 + data[TEMP_SENSOR_END_OFFSET] * 100;
...
}

And read in kraken3_read() without locks:

drivers/hwmon/nzxt-kraken3.c:kraken3_read() {
...
	case hwmon_temp_input:
		*val = priv->temp_input[channel];
...
}

Can the sysfs/debugfs callbacks read these variables concurrently with
the interrupt handler and return torn reads, contrary to the hwmon
subsystem guidelines?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708030507.227193-1-linux@roeck-us.net?part=4