[PATCH 0/4] Fix error handling in HID device probe functions

[PATCH 0/4] Fix error handling in HID device probe functions

[Guenter Roeck]

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
driver probe functions. If the probe operation fails after "io start"
has been initiated, this race condition will result in a UAF vulnerability.

Fix the problem by calling hid_device_io_stop() before calling
hid_hw_stop() in affected hardware monitoring drivers.

----------------------------------------------------------------
Guenter Roeck (4):
      hwmon: (corsair-cpro) Stop device IO before calling hid_hw_stop
      hwmon: (gigabyte_waterforce) Stop device IO before calling hid_hw_stop
      hwmon: (nzxt-smart2) Stop device IO before calling hid_hw_stop
      hwmon: (nzxt-kraken3) Stop device IO before calling hid_hw_stop

 drivers/hwmon/corsair-cpro.c        | 1 +
 drivers/hwmon/gigabyte_waterforce.c | 1 +
 drivers/hwmon/nzxt-kraken3.c        | 1 +
 drivers/hwmon/nzxt-smart2.c         | 1 +
 4 files changed, 4 insertions(+)

[PATCH 1/4] hwmon: (corsair-cpro) Stop device IO before calling hid_hw_stop

[Guenter Roeck]

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. If the probe operation fails after "io start"
has been initiated, this race condition will result in a UAF vulnerability.

Fix the problem by calling hid_device_io_stop() before calling
hid_hw_stop().

Reported-by: Sashiko <sashiko-bot@kernel.org>
Fixes: 40c3a44542257 ("hwmon: add Corsair Commander Pro driver")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/corsair-cpro.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
index b6e508e43fa1..89bd1ccb324c 100644
--- a/drivers/hwmon/corsair-cpro.c
+++ b/drivers/hwmon/corsair-cpro.c
@@ -646,6 +646,7 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 out_hw_close:
 	hid_hw_close(hdev);
 out_hw_stop:
+	hid_device_io_stop(hdev);
 	hid_hw_stop(hdev);
 	return ret;
 }
-- 
2.45.2

Re: [PATCH 1/4] hwmon: (corsair-cpro) Stop device IO before calling hid_hw_stop

[sashiko-bot]

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

New issues:
- [Low] The patch introduces a spurious kernel warning in the probe error path when `hid_hw_open()` fails by unconditionally calling `hid_device_io_stop()`.

Pre-existing issues:
- [High] The driver leaks a debugfs directory on probe failure, creating a Use-After-Free vulnerability.
- [High] A timeout during a USB command causes the driver to consume stale delayed responses for subsequent commands, permanently desynchronizing the protocol.
--

--- Patch [1]: [PATCH 1/4] hwmon: (corsair-cpro) Stop device IO before calling hid_hw_stop ---
commit 0cdcb75d39b8efee212d1a27b0eb0c56aaafb5e9
Author: Guenter Roeck <linux@roeck-us.net>

hwmon: (corsair-cpro) 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 driver probe failure. This commit
calls hid_device_io_stop() before hid_hw_stop() to fix the issue.

> diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
> --- a/drivers/hwmon/corsair-cpro.c
> +++ b/drivers/hwmon/corsair-cpro.c
[ ... ]
> @@ -646,6 +646,7 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  out_hw_close:
>  	hid_hw_close(hdev);
>  out_hw_stop:
> +	hid_device_io_stop(hdev);

[Severity: Low]
Will this cause a spurious kernel warning if hid_hw_open() fails? 

If hid_hw_open() fails earlier in ccp_probe(), the code jumps directly to 
the out_hw_stop label:

ccp_probe() {
    ...
    ret = hid_hw_open(hdev);
    if (ret)
        goto out_hw_stop;
    ...
    hid_device_io_start(hdev);
    ...
}

Since hid_device_io_start() hasn't been called yet, hdev->io_started will 
be false. Calling hid_device_io_stop() here will trigger the dev_warn() 
"io already stopped" from the HID core. Could we conditionally check if IO 
was started, or introduce a new error label to avoid this?

>  	hid_hw_stop(hdev);
>  	return ret;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does the driver leak a 
debugfs directory if hwmon registration fails?

If hwmon_device_register_with_info() fails in ccp_probe(), the code jumps 
to out_hw_close:

ccp_probe() {
    ...
    ccp_debugfs_init(ccp);

    ccp->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, ...);
    if (IS_ERR(ccp->hwmon_dev)) {
        ret = PTR_ERR(ccp->hwmon_dev);
        goto out_hw_close;
    }
    ...
}

Because ccp is allocated with devm_kzalloc(), devres will free it when the 
probe fails. However, without a call to debugfs_remove_recursive(), the 
debugfs files remain. If userspace later reads a leaked file like 
firmware_version, firmware_show() will dereference the freed ccp pointer.

[Severity: High]
This is also a pre-existing issue, but does a timeout in send_usb_cmd() 
cause permanent protocol desynchronization?

If wait_for_completion_timeout() times out, send_usb_cmd() returns 
-ETIMEDOUT without marking the completion as done:

send_usb_cmd() {
    ...
    t = wait_for_completion_timeout(&ccp->wait_input_report, ...);
    if (!t)
        return -ETIMEDOUT;
    ...
}

If the delayed hardware response arrives later, after a subsequent command 
has called reinit_completion(), the interrupt handler will accept the stale 
response:

ccp_raw_event() {
    ...
    if (!completion_done(&ccp->wait_input_report)) {
        memcpy(ccp->buffer, data, min(IN_BUFFER_SIZE, size));
        ...
        complete_all(&ccp->wait_input_report);
    }
    ...
}

This causes the new command to consume the stale response, leaving its own 
response pending for the next command, permanently shifting the responses.

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

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

[Guenter Roeck]

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. If the probe operation fails after "io start"
has been initiated, this race condition will result in a UAF vulnerability.

Fix the problem by calling hid_device_io_stop() before calling
hid_hw_stop().

Reported-by: Sashiko <sashiko-bot@kernel.org>
Fixes: 42ac68e3d4ba0 ("hwmon: Add driver for Gigabyte AORUS Waterforce AIO coolers")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/gigabyte_waterforce.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hwmon/gigabyte_waterforce.c b/drivers/hwmon/gigabyte_waterforce.c
index 27487e215bdd..d5b27751147b 100644
--- a/drivers/hwmon/gigabyte_waterforce.c
+++ b/drivers/hwmon/gigabyte_waterforce.c
@@ -381,6 +381,7 @@ static int waterforce_probe(struct hid_device *hdev, const struct hid_device_id
 fail_and_close:
 	hid_hw_close(hdev);
 fail_and_stop:
+	hid_device_io_stop(hdev);
 	hid_hw_stop(hdev);
 	return ret;
 }
-- 
2.45.2

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

[sashiko-bot]

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

New issues:
- [Medium] Unconditional invocation of `hid_device_io_stop()` in the probe error path triggers spurious kernel warnings on early failures and inverts the teardown order.

Pre-existing issues:
- [High] Missing bounds check on the `size` parameter in `waterforce_raw_event()` allows out-of-bounds memory reads.
- [High] Potential deadlock due to using `spin_lock_bh()` in process context while the same lock is acquired in hardware interrupt context.
- [High] Data race and unprotected multi-variable access between `waterforce_raw_event()` and `waterforce_read()`.
--

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

hwmon: (gigabyte_waterforce) Stop device IO before calling hid_hw_stop

This commit fixes a use-after-free vulnerability by calling
hid_device_io_stop() before hid_hw_stop() in the probe error path.

> diff --git a/drivers/hwmon/gigabyte_waterforce.c b/drivers/hwmon/gigabyte_waterforce.c
> --- a/drivers/hwmon/gigabyte_waterforce.c
> +++ b/drivers/hwmon/gigabyte_waterforce.c
> @@ -381,6 +381,7 @@ static int waterforce_probe(struct hid_device *hdev, const struct hid_device_id
>  fail_and_close:
>  	hid_hw_close(hdev);
>  fail_and_stop:
> +	hid_device_io_stop(hdev);
[Severity: Medium]
Will this unconditionally calling hid_device_io_stop() here trigger spurious
warnings on early failures?

If an early failure occurs, such as hid_hw_open() failing, the code jumps to
fail_and_stop. At this point, hid_device_io_start() has not been called, so
hdev->io_started is false, causing hid_device_io_stop() to emit a spurious
warning.

Also, for later failures that jump to fail_and_close, does this result in
an inverted teardown order by calling hid_hw_close() before
hid_device_io_stop()?

>  	hid_hw_stop(hdev);
>  	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but does waterforce_raw_event() lack a bounds
check on the size parameter?

In waterforce_raw_event():

    if (data[0] == get_firmware_ver_cmd[0] && ...

and later:

    priv->temp_input[0] = data[WATERFORCE_TEMP_SENSOR] * 1000;

If a malicious or malfunctioning HID device sends a short report, could
accessing data at WATERFORCE_TEMP_SENSOR result in an out-of-bounds memory
read?

[Severity: High]
This isn't a bug introduced by this patch, but is there a potential deadlock
between process context and hardware interrupt context?

waterforce_get_status() runs in process context and uses spin_lock_bh():

    spin_lock_bh(&priv->status_report_request_lock);

while waterforce_raw_event() can execute in hardirq context and acquires
the same lock:

    spin_lock(&priv->status_report_request_lock);

If waterforce_raw_event() is triggered by an interrupt on the same CPU
while waterforce_get_status() is holding the lock, will the interrupt
handler attempt to acquire the lock and spin forever?

[Severity: High]
This is a pre-existing issue, but does this code have a data race between
the interrupt handler and the sysfs read function?

waterforce_raw_event() executes asynchronously and updates sensor values
without a lock protecting the data arrays:

    priv->speed_input[0] = get_unaligned_le16(data + WATERFORCE_FAN_SPEED);
    priv->speed_input[1] = get_unaligned_le16(data + WATERFORCE_PUMP_SPEED);

waterforce_read() is called from sysfs and reads these fields locklessly
after releasing the status mutex:

    switch (type) {
    case hwmon_temp:
        *val = priv->temp_input[channel];

Can this result in torn reads and bogus sysfs values if the interrupt
handler overwrites the fields concurrently?

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

[PATCH 3/4] hwmon: (nzxt-smart2) Stop device IO before calling hid_hw_stop

[Guenter Roeck]

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. If the probe operation fails after "io start"
has been initiated, this race condition will result in a UAF vulnerability.

Fix the problem by calling hid_device_io_stop() before calling
hid_hw_stop().

Reported-by: Sashiko <sashiko-bot@kernel.org>
Fixes: 53e68c20aeb1e ("hwmon: add driver for NZXT RGB&Fan Controller/Smart Device v2")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/nzxt-smart2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hwmon/nzxt-smart2.c b/drivers/hwmon/nzxt-smart2.c
index 58ef9fa0184b..286de15cedc7 100644
--- a/drivers/hwmon/nzxt-smart2.c
+++ b/drivers/hwmon/nzxt-smart2.c
@@ -770,6 +770,7 @@ static int nzxt_smart2_hid_probe(struct hid_device *hdev,
 	hid_hw_close(hdev);
 
 out_hw_stop:
+	hid_device_io_stop(hdev);
 	hid_hw_stop(hdev);
 	return ret;
 }
-- 
2.45.2

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

[sashiko-bot]

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

New issues:
- [Low] Unconditional call to `hid_device_io_stop()` on the error path triggers a spurious kernel warning when `hid_hw_open()` fails.

Pre-existing issues:
- [Critical] The same Use-After-Free (UAF) vulnerability exists in other HID hwmon drivers (nzxt-kraken3.c and corsair-psu.c) and was missed by this patch series.
- [High] Device removal can hang indefinitely if a user process is blocked reading sysfs attributes.
- [High] The return value of init_device() is silently ignored during probe, leading to permanent hangs on sysfs reads.
--

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

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

Calling hid_hw_stop() does not stop the device IO, causing a race condition
between hid_input_report() and probe failure. Fix this by calling
hid_device_io_stop() before calling hid_hw_stop().

> diff --git a/drivers/hwmon/nzxt-smart2.c b/drivers/hwmon/nzxt-smart2.c
> index 58ef9fa0184be..286de15cedc7d 100644
> --- a/drivers/hwmon/nzxt-smart2.c
> +++ b/drivers/hwmon/nzxt-smart2.c
> @@ -770,6 +770,7 @@ static int nzxt_smart2_hid_probe(struct hid_device *hdev,
>  	hid_hw_close(hdev);
>  
>  out_hw_stop:
> +	hid_device_io_stop(hdev);

[Severity: Low]
If hid_hw_open() fails earlier in the probe function, we jump to out_hw_stop
and call hid_device_io_stop():

drivers/hwmon/nzxt-smart2.c:nzxt_smart2_hid_probe()
        ret = hid_hw_open(hdev);
        if (ret)
                goto out_hw_stop;

Since hid_device_io_start() hasn't been called yet, does this trigger the
dev_warn("io already stopped") in hid_device_io_stop()?

>  	hid_hw_stop(hdev);
>  	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but does ignoring the return value of
init_device() cause sysfs reads to hang?

If init_device() fails to send the 'detect fans' command during
nzxt_smart2_hid_probe(), it appears fan_config_received is never set to true.
Will subsequent sysfs reads block forever in
wait_event_interruptible_locked_irq() waiting for that condition?


[Severity: High]
This is a pre-existing issue, but does device removal hang if a user
process is currently blocked reading sysfs attributes?

When a process accesses sysfs attributes, nzxt_smart2_hwmon_read() blocks:

drivers/hwmon/nzxt-smart2.c:nzxt_smart2_hwmon_read()
        res = wait_event_interruptible_locked_irq(drvdata->wq,
                                                  drvdata->fan_config_received);

If the device is unplugged, nzxt_smart2_hid_remove() calls
hwmon_device_unregister(), which waits for active readers to finish:

drivers/hwmon/nzxt-smart2.c:nzxt_smart2_hid_remove()
        hwmon_device_unregister(drvdata->hwmon);

Since the waitqueue is never woken up during remove(), does the reader
process stay blocked indefinitely, causing the unregister to hang?


[Severity: Critical]
This isn't a bug introduced by this patch, but does the same use-after-free
vulnerability exist in the probe error paths of other HID hwmon drivers?

Both nzxt-kraken3.c and corsair-psu.c appear to call hid_device_io_start()
during probe, but omit hid_device_io_stop() in their fail_and_stop error
paths:

drivers/hwmon/nzxt-kraken3.c:kraken3_probe()
fail_and_close:
        hid_hw_close(hdev);
fail_and_stop:
        hid_hw_stop(hdev);

drivers/hwmon/corsair-psu.c:corsairpsu_probe()
fail_and_close:
        hid_hw_close(hdev);
fail_and_stop:
        hid_hw_stop(hdev);

Do these drivers also need to call hid_device_io_stop(hdev) to prevent
hid_input_report() from executing after device structures are freed?

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

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

[Guenter Roeck]

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. If the probe operation fails after "io start"
has been initiated, this race condition will result in a UAF vulnerability.

Fix the problem by calling hid_device_io_stop() before calling
hid_hw_stop().

Reported-by: Sashiko <sashiko-bot@kernel.org>
Fixes: f3b4b146eb107 ("hwmon: Add driver for NZXT Kraken X and Z series AIO CPU coolers")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/nzxt-kraken3.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hwmon/nzxt-kraken3.c b/drivers/hwmon/nzxt-kraken3.c
index d00409bcab93..cca53252b343 100644
--- a/drivers/hwmon/nzxt-kraken3.c
+++ b/drivers/hwmon/nzxt-kraken3.c
@@ -970,6 +970,7 @@ static int kraken3_probe(struct hid_device *hdev, const struct hid_device_id *id
 fail_and_close:
 	hid_hw_close(hdev);
 fail_and_stop:
+	hid_device_io_stop(hdev);
 	hid_hw_stop(hdev);
 	return ret;
 }
-- 
2.45.2

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unconditional call to `hid_device_io_stop()` in the probe error path triggers a spurious kernel warning and improperly unwinds initialization.
--

--- Patch [4]: [PATCH 4/4] hwmon: (nzxt-kraken3) Stop device IO before calling hid_hw_stop ---
commit 1a6a1a210c8f1b5e86d4cd6021a1f5a8d033e172
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, causing a race condition
if probe fails. This commit fixes the problem by calling
hid_device_io_stop() before calling hid_hw_stop().

> diff --git a/drivers/hwmon/nzxt-kraken3.c b/drivers/hwmon/nzxt-kraken3.c
> index d00409bcab93a..cca53252b343a 100644
> --- a/drivers/hwmon/nzxt-kraken3.c
> +++ b/drivers/hwmon/nzxt-kraken3.c
> @@ -970,6 +970,7 @@ static int kraken3_probe(struct hid_device *hdev, const struct hid_device_id *id
>  fail_and_close:
>  	hid_hw_close(hdev);
>  fail_and_stop:
> +	hid_device_io_stop(hdev);

[Severity: High]
Does this code unconditionally call hid_device_io_stop() on error paths where
hid_device_io_start() has not yet been called?

Looking at kraken3_probe() in drivers/hwmon/nzxt-kraken3.c, if we hit the
default case in the product switch statement or if memory allocation for
priv->buffer fails, we jump to fail_and_close:

        default:
                ret = -ENODEV;
                goto fail_and_close;

At this point, hid_device_io_start() hasn't executed.

Because hid_device_io_stop() checks if io_started is false and emits a
dev_warn() in that case, this will trigger a spurious kernel warning on
these benign error paths. On systems with panic_on_warn enabled, this could
lead to a panic just by plugging in an unsupported device.

Additionally, when failing after hid_device_io_start() has been called
(for example, if kraken3_init_device() fails), the error handling jumps to
fail_and_close. This means hid_hw_close() is called before
hid_device_io_stop().

Should a new fail_and_stop_io label be added to ensure the teardown
happens in the correct LIFO order and to avoid warnings on early errors?

>  	hid_hw_stop(hdev);
>  	return ret;
>  }

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