Re: [PATCH v2 2/4] hwmon: (gigabyte_waterforce) Stop device IO before calling hid_hw_stop

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Systemic Preexisting UAF in other hwmon HID drivers.
- [High] Architecture Violation and Data Race in hwmon sensor reading.
- [High] Potential Deadlock between process and hardirq context.
--

--- Patch [2]: [PATCH v2 2/4] hwmon: (gigabyte_waterforce) Stop device IO before calling hid_hw_stop ---
commit 1ac1936efdffad20d37444790d9aa7b430d348cd
Author: Guenter Roeck <linux@roeck-us.net>

hwmon: (gigabyte_waterforce) Stop device IO before calling hid_hw_stop

Calling hid_hw_stop() does not stop the device IO. This results in a race
condition between hid_input_report() and the point immediately following the
execution of hid_device_io_start() within the driver probe function. Fix the
problem by calling hid_device_io_stop() before calling hid_hw_stop().

> diff --git a/drivers/hwmon/gigabyte_waterforce.c b/drivers/hwmon/gigabyte_waterforce.c
> index 27487e215bddf..4eea05f8b569c 100644
> --- a/drivers/hwmon/gigabyte_waterforce.c
> +++ b/drivers/hwmon/gigabyte_waterforce.c
> @@ -371,13 +371,15 @@ static int waterforce_probe(struct hid_device *hdev, const struct hid_device_id
>  	if (IS_ERR(priv->hwmon_dev)) {
>  		ret = PTR_ERR(priv->hwmon_dev);
>  		hid_err(hdev, "hwmon registration failed with %d\n", ret);
> -		goto fail_and_close;
> +		goto fail_and_io_stop;
>  	}
>  
>  	waterforce_debugfs_init(priv);
>  
>  	return 0;
>  
> +fail_and_io_stop:
> +	hid_device_io_stop(hdev);
>  fail_and_close:
>  	hid_hw_close(hdev);
>  fail_and_stop:

[Severity: Critical]
This isn't a bug introduced by this patch, but does this same UAF
vulnerability exist in other hwmon HID drivers?

Looking at corsairpsu_probe() in drivers/hwmon/corsair-psu.c, kraken3_probe()
in drivers/hwmon/nzxt-kraken3.c, and nzxt_smart2_hid_probe() in
drivers/hwmon/nzxt-smart2.c, the error paths jump directly to hid_hw_close()
and hid_hw_stop() without calling hid_device_io_stop(). For example, in
corsairpsu_probe:

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

Since hid_device_io_start() unlocked the driver input lock, asynchronous HID
events could continue firing and triggering the raw_event handler while the
driver state is freed. Should these other drivers be fixed as well?

[Severity: High]
This is a pre-existing issue, but does the lack of locking when reading sensor
state cause torn reads?

In drivers/hwmon/gigabyte_waterforce.c, waterforce_read() reads the
priv->temp_input[channel] array directly from process context:

    *val = priv->temp_input[channel];

Concurrently, waterforce_raw_event() runs in the HID input/interrupt context
and modifies these identical arrays:

    priv->temp_input[0] = data[WATERFORCE_TEMP_SENSOR] * 1000;
    priv->speed_input[0] = get_unaligned_le16(data + WATERFORCE_FAN_SPEED);

According to the subsystem guidelines in subsystem/hwmon.md, drivers must
implement locking required for interrupt handling (e.g., using hwmon_lock()
and hwmon_unlock()). Could the lack of synchronization here allow the
interrupt handler to tear the data being read by sysfs?

[Severity: High]
This isn't a bug introduced by this patch, but could the lock usage around
priv->status_report_request_lock lead to a deadlock?

In drivers/hwmon/gigabyte_waterforce.c, waterforce_get_status() acquires the
lock using spin_lock_bh(), which disables softirqs but leaves hardirqs
enabled:

    spin_lock_bh(&priv->status_report_request_lock);
    reinit_completion(&priv->status_report_received);
    spin_unlock_bh(&priv->status_report_request_lock);

If a USB URB completion fires in hardirq context while this lock is held, it
will call waterforce_raw_event(), which attempts to acquire the same lock
using plain spin_lock():

    spin_lock(&priv->status_report_request_lock);
    if (!completion_done(&priv->status_report_received))
        complete_all(&priv->status_report_received);
    spin_unlock(&priv->status_report_request_lock);

Could this cause a recursive spinlock deadlock? Would it be safer to use
spin_lock_irqsave() and spin_lock_irqrestore() in both paths to properly
synchronize between process context and the HID raw event handler?

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