Re: [PATCH 3/3] hwmon: (asus_rog_ryujin) Add ROG Ryujin III White Edition

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

Pre-existing issues:
- [High] Hardware and input processing are started before wait queue completions are initialized, leading to potential NULL pointer dereferences.
- [High] The spinlock `status_report_request_lock` provides no mutual exclusion against the raw event handler, leading to race conditions with completions.
- [High] The raw event handler fails to validate the size of incoming HID reports.
- [High] The driver updates and reads shared sensor data fields concurrently between `rog_ryujin_raw_event` and sysfs operations without using the mandated `hwmon_lock()`.
--

--- Patch [3]: [PATCH 3/3] hwmon: (asus_rog_ryujin) Add ROG Ryujin III White Edition ---
commit 7de73211f5a55165f52d68bf9aae14fa4ccb23f0
Author: Will Smith <[email protected]>

hwmon: (asus_rog_ryujin) Add ROG Ryujin III White Edition

This commit adds support for the ROG Ryujin III White Edition liquid cooler.
It adds the USB device ID to the hardware monitoring driver and updates the
documentation.

Link: https://github.com/aleksamagicka/asus_rog_ryujin-hwmon/pull/10

> diff --git a/drivers/hwmon/asus_rog_ryujin.c b/drivers/hwmon/asus_rog_ryujin.c
> --- a/drivers/hwmon/asus_rog_ryujin.c
> +++ b/drivers/hwmon/asus_rog_ryujin.c

[ ... ]

> @@ -587,6 +588,8 @@ static const struct hid_device_id rog_ryujin_table[] = {
>  	  .driver_data = (kernel_ulong_t)&rog_ryujin_iii_info },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUS_ROG, USB_PRODUCT_ID_RYUJIN_III_EVA),
>  	  .driver_data = (kernel_ulong_t)&rog_ryujin_iii_info },
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUS_ROG, USB_PRODUCT_ID_RYUJIN_III_WHITE),
> +	  .driver_data = (kernel_ulong_t)&rog_ryujin_iii_info },
>  	{ }
>  };

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this driver I 
noticed a pre-existing issue with hardware initialization.

In rog_ryujin_probe(), event polling is started before the wait queue
completions are initialized:

    ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
    ...
    init_completion(&priv->cooler_status_received);

Could this lead to a NULL pointer dereference or list corruption if the
device sends an early report and rog_ryujin_raw_event() calls complete_all()
on the uninitialized completion?

[Severity: High]
This is a pre-existing issue, but does the status_report_request_lock provide
adequate mutual exclusion?

In rog_ryujin_execute_cmd(), the lock is taken when reinitializing completions:

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

However, in rog_ryujin_raw_event(), complete_all() is called without
acquiring this lock. Can this lead to race conditions with the completion
state being clobbered, potentially causing sysfs reads to hang and return
-ETIMEDOUT?

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for an
out-of-bounds read to occur when parsing the HID report?

In rog_ryujin_raw_event(), the incoming report is parsed directly:

    if (data[1] == RYUJIN_GET_COOLER_STATUS_CMD_RESPONSE) {
        priv->temp_input[0] = data[priv->info->temp_offset] * 1000 +

Should the size parameter be validated before accessing data offsets to
prevent exposing stale or uninitialized memory if a truncated packet is
received?

[Severity: High]
This is a pre-existing issue, but does this driver violate the hwmon
subsystem locking guidelines?

The driver updates and reads shared sensor data fields concurrently between
rog_ryujin_raw_event() and sysfs operations:

    priv->temp_input[0] = data[priv->info->temp_offset] * 1000 +
        data[priv->info->temp_offset + 1] * 100;

And in rog_ryujin_read():

    *val = priv->temp_input[channel];

Are hwmon_lock() and hwmon_unlock() required here to synchronize the
attribute access, as mandated for hardware monitoring drivers?

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