Re: [PATCH v2 3/4] hwmon: (nzxt-smart2) 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:
- [Critical] The patch fixes a UAF vulnerability in `nzxt-smart2.c` by calling `hid_device_io_stop()` on probe failure after `hid_device_io_start()`, but fails to apply this same necessary fix to multiple other HID drivers that suffer from the exact same vulnerability.
- [Critical] The output buffer embedded in `struct drvdata` is not cacheline aligned, violating DMA API rules.
- [High] Device unplug causes a deadlock in hwmon_device_unregister() due to sysfs readers stuck in indefinite wait.
- [High] The return value of `init_device()` in `nzxt_smart2_hid_probe()` is completely ignored, which can lead to indefinite hangs during sysfs reads.
- [High] Potential deadlock between process context and hardware interrupt context due to incorrect spinlock variants.
--

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

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

This commit addresses a race condition between hid_input_report and the
probe function. It calls hid_device_io_stop before hid_hw_stop in the error
path to prevent a use-after-free vulnerability.

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

[Severity: Critical]
This isn't a bug introduced by this patch, but while addressing the
use-after-free vulnerability here, are there other drivers that need this
same fix? 

For example, in drivers/hwmon/corsair-psu.c:corsairpsu_probe():

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

And similarly in drivers/hwmon/nzxt-kraken3.c:kraken3_probe(), 
drivers/hid/hid-nintendo.c:nintendo_hid_probe(), and 
drivers/hid/hid-mcp2221.c:mcp2221_hid_unregister(). They also appear to omit 
hid_device_io_stop() in their cleanup paths after hid_device_io_start() has 
been called. Could they be vulnerable to the exact same issue?

>  out_hw_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 indefinitely?

In nzxt_smart2_hid_probe():

    hid_device_io_start(hdev);
    init_device(drvdata, UPDATE_INTERVAL_DEFAULT_MS);
    drvdata->hwmon = hwmon_device_register_with_info(...)

If init_device() fails, the device won't send the expected FAN_CONFIG input 
report. When a user process later attempts to read from sysfs, it triggers 
nzxt_smart2_hwmon_read():

    res = wait_event_interruptible_locked_irq(drvdata->wq,
                                              drvdata->fan_config_received);

Will this wait loop block forever waiting for a configuration report that
will never arrive?

[Severity: High]
This isn't a bug introduced by this patch, but can the driver deadlock if 
the device is physically unplugged while a sysfs read is pending?

If a sysfs read is waiting in nzxt_smart2_hwmon_read() as shown above, and 
the device is disconnected, nzxt_smart2_hid_remove() is called:

    static void nzxt_smart2_hid_remove(struct hid_device *hdev)
    {
        struct drvdata *drvdata = hid_get_drvdata(hdev);
        hwmon_device_unregister(drvdata->hwmon);

Because the sysfs read is blocked indefinitely waiting for an input report 
from the unplugged device, will hwmon_device_unregister() hang in 
kernfs_drain() waiting for the active sysfs operation to finish?

[Severity: High]
This is a pre-existing issue, but is there a potential deadlock between 
process context and hardware interrupt context here?

In set_pwm(), the lock is acquired with softirqs disabled but hardirqs 
enabled:

    spin_lock_bh(&drvdata->wq.lock);
    drvdata->fan_duty_percent[channel] = duty_percent;
    spin_unlock_bh(&drvdata->wq.lock);

If a URB completion hardware interrupt fires on the same CPU while this 
lock is held, it can call handle_fan_status_report():

    spin_lock(&drvdata->wq.lock);

Will the CPU spin forever attempting to acquire the lock that the 
interrupted process already holds?

[Severity: Critical]
This isn't a bug introduced by this patch, but does passing an embedded 
buffer to the HID core violate DMA API rules?

In struct drvdata:

    struct mutex mutex;
    long update_interval;
    u8 output_buffer[OUTPUT_REPORT_SIZE];

In send_output_report():

    ret = hid_hw_output_report(drvdata->hid, drvdata->output_buffer,
                               sizeof(drvdata->output_buffer));

Since output_buffer shares a cacheline with the mutex and update_interval,
could the underlying USB DMA mapping operations corrupt these adjacent
variables on systems with non-coherent caches?

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