Re: [PATCH 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 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