Re: [PATCH] hwmon: (nzxt-smart2) DMA-align output buffer

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

Pre-existing issues:
- [High] Sysfs read paths wait indefinitely for device reports, causing a deadlock during device removal when kernfs_drain waits for sysfs operations to complete.
- [High] Unprotected access to output_buffer and update_interval in the reset_resume path leads to corrupted USB reports.
--

--- Patch [1]: [PATCH] hwmon: (nzxt-smart2) DMA-align output buffer ---
commit 4207ab1b1718833d04744a9d89312688c2cf5221
Author: Guenter Roeck <[email protected]>

hwmon: (nzxt-smart2) DMA-align output buffer

When send_output_report() calls hid_hw_output_report(), the underlying USB
HID core calls usb_interrupt_msg() which maps this buffer directly for DMA.
Align the output buffer to ARCH_DMA_MINALIGN to fix the problem on
non-coherent architectures.

> diff --git a/drivers/hwmon/nzxt-smart2.c b/drivers/hwmon/nzxt-smart2.c
> index e2316c46629d6..ff0c0bee0e839 100644
> --- a/drivers/hwmon/nzxt-smart2.c
> +++ b/drivers/hwmon/nzxt-smart2.c
> @@ -203,7 +203,7 @@ struct drvdata {
>  	 */
>  	struct mutex mutex;
>  	long update_interval;
> -	u8 output_buffer[OUTPUT_REPORT_SIZE];
> +	u8 output_buffer[OUTPUT_REPORT_SIZE] __aligned(ARCH_DMA_MINALIGN);

[Severity: High]
This is a pre-existing issue, but could this output buffer be corrupted
during a USB device reset?

If a userspace process triggers a USBDEVFS_RESET ioctl, the USB core logic
calls nzxt_smart2_hid_reset_resume(), which calls init_device() without
holding drvdata->mutex:

nzxt_smart2_hid_reset_resume()
    return init_device(drvdata, drvdata->update_interval);
    
init_device()
    ret = send_output_report(drvdata, detect_fans_report, ...);
    
send_output_report()
    memcpy(drvdata->output_buffer, data, data_size);

Because userspace sysfs access is not frozen during USBDEVFS_RESET, another
thread could concurrently write to a hwmon sysfs attribute, acquire the
mutex, and call send_output_report(). 

Both threads would then concurrently modify the shared output_buffer via
memcpy() and attempt to submit it, potentially corrupting the USB report and
causing concurrent DMA mapping of the same buffer.

Does init_device() need to acquire the mutex when called from the
reset_resume path?


[Severity: High]
This is another pre-existing issue, but is there a risk of deadlocking
during device removal if a sysfs read is blocked?

In nzxt_smart2_hwmon_read(), the read path blocks waiting for a report:

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

If the device is disconnected before the report arrives, the read might
block indefinitely.

When the USB disconnect triggers nzxt_smart2_hid_remove(), it calls
hwmon_device_unregister():

nzxt_smart2_hid_remove()
    hwmon_device_unregister(drvdata->hwmon);
    
This in turn invokes kernfs_drain to wait for all sysfs operations to
complete.

Since the sysfs read is waiting indefinitely and the remove function does
not wake it up, could kernfs_drain block forever and deadlock the removal?

>  };
>  
>  static long scale_pwm_value(long val, long orig_max, long new_max)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1