Re: [PATCH 2/3] hwmon: (asus_rog_ryujin) Add ROG Ryujin III support

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing bounds check on the incoming HID report size allows out-of-bounds memory reads.
- [High] Completions and spinlocks are initialized after enabling HID hardware interrupts, creating a race condition that leads to a NULL pointer dereference.
- [Medium] Ineffective locking when reinitializing completions causes a data race on SMP systems.
- [Medium] Incorrect `time_after` calculation allows uninitialized sensor data to be returned to userspace if read in the same jiffy as probe.
- [Medium] The driver violates the hwmon subsystem requirement to use hwmon_lock() for serializing interrupt updates.
--

--- Patch [2]: [PATCH 2/3] hwmon: (asus_rog_ryujin) Add ROG Ryujin III support ---
commit 3d0c1a58d5a805ca4eebf7afcc2b51a8194835be
Author: Arie Miller <renari@arimil.com>

hwmon: (asus_rog_ryujin) Add ROG Ryujin III support
    
The ROG Ryujin III uses different report offsets and a different
cooler-duty channel from the Ryujin II. It also lacks the separate
external fan controller supplied with the older model.

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

> diff --git a/drivers/hwmon/asus_rog_ryujin.c b/drivers/hwmon/asus_rog_ryujin.c
> index b86b87e33615d..5bc60c6036de3 100644
> --- a/drivers/hwmon/asus_rog_ryujin.c
> +++ b/drivers/hwmon/asus_rog_ryujin.c

[ ... ]

> @@ -34,6 +36,14 @@ static const struct rog_ryujin_device_info rog_ryujin_ii_360_info = {
>  	.has_controller = true,
>  };
>  
> +static const struct rog_ryujin_device_info rog_ryujin_iii_info = {
> +	.temp_offset = 5,
> +	.pump_speed_offset = 7,
> +	.fan_speed_offset = 10,

[Severity: High]
This is a pre-existing issue, but does rog_ryujin_raw_event() verify if the
incoming HID report size is sufficiently large before accessing dynamic
payload offsets?

For example, it unconditionally reads:

priv->speed_input[1] =
    get_unaligned_le16(data + priv->info->fan_speed_offset);

Since this patch adds new devices with larger offsets, could a truncated HID
report cause an out-of-bounds memory read here?

> +	.duty_channel = 1,
> +	.has_controller = false,
> +};
> +
>  #define STATUS_VALIDITY		1500	/* ms */

[Severity: Medium]
This isn't a bug introduced by this patch, but does rog_ryujin_get_status()
correctly handle being called in the exact same jiffy as the probe?

If a thermal governor or monitoring tool polls the device immediately upon
registration, and it evaluates the time check:

if (!time_after(jiffies, priv->updated + msecs_to_jiffies(STATUS_VALIDITY))) {
    /* Data is up to date */
    return 0;
}

Since time_after(jiffies, jiffies) is false, the negation makes this
condition true. Would this incorrectly skip hardware polling and return
uninitialized sensor data to userspace?

[ ... ]

> @@ -573,6 +583,10 @@ static void rog_ryujin_remove(struct hid_device *hdev)
>  static const struct hid_device_id rog_ryujin_table[] = {
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUS_ROG, USB_PRODUCT_ID_RYUJIN_AIO),
>  	  .driver_data = (kernel_ulong_t)&rog_ryujin_ii_360_info },
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUS_ROG, USB_PRODUCT_ID_RYUJIN_III_EXTREME),
> +	  .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 },
>  	{ }
>  };

[Severity: High]
This isn't a bug introduced by this patch, but does rog_ryujin_probe() enable
hardware interrupts before initializing the synchronization primitives?

If hid_hw_open() is called before init_completion() and spin_lock_init(),
and the device sends a report immediately:

rog_ryujin_raw_event()
    if (!completion_done(&priv->cooler_status_received))
        complete_all(&priv->cooler_status_received);

Could this attempt to complete an uninitialized structure and cause a NULL
pointer dereference?


[Severity: Medium]
This is a pre-existing issue, but is the locking around completions effective
against concurrent raw events?

rog_ryujin_execute_cmd() takes a lock to protect the completion:

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

However, rog_ryujin_raw_event() signals this completion without acquiring the
corresponding lock:

if (!completion_done(&priv->cooler_status_received))
    complete_all(&priv->cooler_status_received);

Could this result in a data race on SMP systems, leading to corrupted
completion state or premature timeouts?


[Severity: Medium]
This isn't a bug introduced by this patch, but does this driver adhere to the
hardware monitoring subsystem guidelines for locking?

The hwmon guidelines state that drivers must use hwmon_lock() and
hwmon_unlock() for serializing attributes registered outside the info
parameter, and for interrupt handling.

In rog_ryujin_raw_event(), the driver asynchronously modifies shared state in
interrupt context without the hwmon lock:

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

Could this lead to inconsistent or torn reads when rog_ryujin_read() accesses
these arrays concurrently from process context?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807000107.1786892-1-renari@arimil.com?part=2