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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Hardware transport is started before synchronization primitives are initialized, leading to a NULL pointer dereference.
- [High] Missing bounds check on HID report size leads to kernel memory information leak.
- [High] Ineffective spinlock usage causes data races and corrupted completion state.
--

--- Patch [2]: [PATCH v3 2/3] hwmon: (asus_rog_ryujin) Add ROG Ryujin III support ---
commit 38401fc658552e2aee59237664fe94f5ef49d39c
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 80563ce38519d..79e5b6d6dae0a 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 isn't a bug introduced by this patch, but does this driver check the
size of the incoming HID report before parsing it?

In rog_ryujin_raw_event(), the driver dynamically accesses payload
offsets without validating against the received report size:

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

If a malicious or buggy USB device sends a HID report shorter than the
expected length (e.g., 1 byte), this code would unconditionally access
memory up to the offset. Since the underlying HID transport buffer is
dynamically allocated to at least 64 bytes (HID_MIN_BUFFER_SIZE), an
out-of-bounds page fault wouldn't occur, but uninitialized buffer bytes
could be parsed as sensor values and exposed to userspace via hwmon
sysfs attributes.

Could we add a size check at the beginning of rog_ryujin_raw_event()
to prevent leaking kernel memory information?

> +	.duty_channel = 1,
> +	.has_controller = false,
> +};
> +
[ ... ]
> @@ -605,4 +619,4 @@ module_exit(rog_ryujin_exit);
>  
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Aleksa Savic <savicaleksa83@gmail.com>");
> -MODULE_DESCRIPTION("Hwmon driver for Asus ROG Ryujin II 360 AIO cooler");
> +MODULE_DESCRIPTION("Hwmon driver for Asus ROG Ryujin AIO coolers");

[Severity: Critical]
This is a pre-existing issue, but is the initialization order safe in
rog_ryujin_probe()?

The hardware transport is enabled and opened before initializing the
completion structures used in the raw event handler:

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

If a report arrives immediately after hid_hw_open(),
rog_ryujin_raw_event() can fire and call complete_all() on a zero-allocated
struct completion. 

The wait.head list_head inside the completion is uninitialized, which could
cause __wake_up_locked() to dereference a NULL pointer and trigger a kernel
panic.

Should init_completion() be moved before the hardware transport is started?

[Severity: High]
This is a pre-existing issue, but does the spinlock usage correctly serialize
against the raw event handler?

In rog_ryujin_execute_cmd(), the spinlock is acquired to serialize
against the raw event handler:

	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() modifies the completion state concurrently
without acquiring this protective spinlock:

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

If a device report arrives on another CPU exactly as execute_cmd() is
calling reinit_completion() (which sets done = 0), this lack of mutual
exclusion allows reinit_completion() to race with complete_all().

Could this corrupt the completion count and cause the driver to sleep
until timeout?

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