[PATCH 0/2] hwmon: Fix ASUS ROG Ryujin HID report handling

[PATCH 0/2] hwmon: Fix ASUS ROG Ryujin HID report handling

[Arie Miller]

A Sashiko review found two pre-existing issues in the ASUS ROG Ryujin
driver: missing HID report length validation and a race between completion
updates and reinitialization.

Reject undersized reports and serialize both paths with the existing
spinlock.

Tested on an ASUS ROG Ryujin III EVA using a DKMS-built module with 16,000
concurrent hwmon reads and 50 liquidctl status requests over hidraw. No
failures, timeouts, lock warnings, or HID errors were observed.

Arie Miller (2):
  hwmon: (asus_rog_ryujin) Validate HID report lengths
  hwmon: (asus_rog_ryujin) Synchronize HID command and report handling

 drivers/hwmon/asus_rog_ryujin.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

-- 
2.55.0

[PATCH 1/2] hwmon: (asus_rog_ryujin) Validate HID report lengths

[Arie Miller]

rog_ryujin_raw_event() parses response headers and payload fields without
first checking that they are present in the received report. A short report
can therefore make the driver consume uninitialized bytes from the HID
transport buffer and expose them as sensor values through sysfs.

Validate the response header and the fields used by each response type
before parsing them.

Fixes: ed3e03790c5c ("hwmon: Add driver for ASUS ROG RYUJIN II 360 AIO cooler")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-hwmon/20260812104617.858D01F000E9@smtp.kernel.org/
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.6-sol sparse
Signed-off-by: Arie Miller <renari@arimil.com>
---
 drivers/hwmon/asus_rog_ryujin.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/asus_rog_ryujin.c b/drivers/hwmon/asus_rog_ryujin.c
index 702edb831394..f4d99c510369 100644
--- a/drivers/hwmon/asus_rog_ryujin.c
+++ b/drivers/hwmon/asus_rog_ryujin.c
@@ -422,10 +422,15 @@ static int rog_ryujin_raw_event(struct hid_device *hdev, struct hid_report *repo
 {
 	struct rog_ryujin_data *priv = hid_get_drvdata(hdev);
 
-	if (data[0] != RYUJIN_CMD_PREFIX)
+	if (size < 2 || data[0] != RYUJIN_CMD_PREFIX)
 		return 0;
 
 	if (data[1] == RYUJIN_GET_COOLER_STATUS_CMD_RESPONSE) {
+		if (size <= priv->info->temp_offset + 1 ||
+		    size <= priv->info->pump_speed_offset + 1 ||
+		    size <= priv->info->fan_speed_offset + 1)
+			return 0;
+
 		/* Received coolant temp and speeds of pump and internal fan */
 		priv->temp_input[0] = data[priv->info->temp_offset] * 1000 +
 			data[priv->info->temp_offset + 1] * 100;
@@ -437,6 +442,9 @@ static int rog_ryujin_raw_event(struct hid_device *hdev, struct hid_report *repo
 		if (!completion_done(&priv->cooler_status_received))
 			complete_all(&priv->cooler_status_received);
 	} else if (data[1] == RYUJIN_GET_CONTROLLER_SPEED_CMD_RESPONSE) {
+		if (size <= RYUJIN_CONTROLLER_SPEED_3 + 1)
+			return 0;
+
 		/* Received speeds of four fans attached to the controller */
 		priv->speed_input[2] = get_unaligned_le16(data + RYUJIN_CONTROLLER_SPEED_1);
 		priv->speed_input[3] = get_unaligned_le16(data + RYUJIN_CONTROLLER_SPEED_2);
@@ -446,6 +454,9 @@ static int rog_ryujin_raw_event(struct hid_device *hdev, struct hid_report *repo
 		if (!completion_done(&priv->controller_status_received))
 			complete_all(&priv->controller_status_received);
 	} else if (data[1] == RYUJIN_GET_COOLER_DUTY_CMD_RESPONSE) {
+		if (size <= RYUJIN_INTERNAL_FAN_DUTY)
+			return 0;
+
 		/* Received report for pump and internal fan duties (in %) */
 		if (data[RYUJIN_PUMP_DUTY] == 0 && data[RYUJIN_INTERNAL_FAN_DUTY] == 0) {
 			/*
@@ -472,6 +483,9 @@ static int rog_ryujin_raw_event(struct hid_device *hdev, struct hid_report *repo
 		if (!completion_done(&priv->cooler_duty_received))
 			complete_all(&priv->cooler_duty_received);
 	} else if (data[1] == RYUJIN_GET_CONTROLLER_DUTY_CMD_RESPONSE) {
+		if (size <= RYUJIN_CONTROLLER_DUTY)
+			return 0;
+
 		/* Received report for controller duty for fans (in PWM) */
 		if (data[RYUJIN_CONTROLLER_DUTY] == 0) {
 			/*
-- 
2.55.0

[PATCH 2/2] hwmon: (asus_rog_ryujin) Synchronize HID command and report handling

[Arie Miller]

rog_ryujin_execute_cmd() holds status_report_request_lock while
reinitializing a completion, intending to exclude raw-event handling.
However, rog_ryujin_raw_event() does not acquire the lock when it updates
the completion. A response can therefore race with reinit_completion() and
be lost, leaving the command to time out.

Hold the lock while parsing reports and updating their completions. Use the
irqsave variants in both paths because raw-event handling may run in
interrupt context.

Fixes: ed3e03790c5c ("hwmon: Add driver for ASUS ROG RYUJIN II 360 AIO cooler")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-hwmon/20260812104617.858D01F000E9@smtp.kernel.org/
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.6-sol sparse
Signed-off-by: Arie Miller <renari@arimil.com>
---
 drivers/hwmon/asus_rog_ryujin.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/asus_rog_ryujin.c b/drivers/hwmon/asus_rog_ryujin.c
index f4d99c510369..e297557ca346 100644
--- a/drivers/hwmon/asus_rog_ryujin.c
+++ b/drivers/hwmon/asus_rog_ryujin.c
@@ -184,6 +184,7 @@ static int rog_ryujin_write_expanded(struct rog_ryujin_data *priv, const u8 *cmd
 static int rog_ryujin_execute_cmd(struct rog_ryujin_data *priv, const u8 *cmd, int cmd_length,
 				  struct completion *status_completion)
 {
+	unsigned long flags;
 	int ret;
 
 	/*
@@ -191,9 +192,9 @@ static int rog_ryujin_execute_cmd(struct rog_ryujin_data *priv, const u8 *cmd, i
 	 * completion. Reinit is done because hidraw could have triggered
 	 * the raw event parsing and marked the passed in completion as done.
 	 */
-	spin_lock_bh(&priv->status_report_request_lock);
+	spin_lock_irqsave(&priv->status_report_request_lock, flags);
 	reinit_completion(status_completion);
-	spin_unlock_bh(&priv->status_report_request_lock);
+	spin_unlock_irqrestore(&priv->status_report_request_lock, flags);
 
 	/* Send command for getting data */
 	ret = rog_ryujin_write_expanded(priv, cmd, cmd_length);
@@ -421,15 +422,18 @@ static int rog_ryujin_raw_event(struct hid_device *hdev, struct hid_report *repo
 				int size)
 {
 	struct rog_ryujin_data *priv = hid_get_drvdata(hdev);
+	unsigned long flags;
 
 	if (size < 2 || data[0] != RYUJIN_CMD_PREFIX)
 		return 0;
 
+	spin_lock_irqsave(&priv->status_report_request_lock, flags);
+
 	if (data[1] == RYUJIN_GET_COOLER_STATUS_CMD_RESPONSE) {
 		if (size <= priv->info->temp_offset + 1 ||
 		    size <= priv->info->pump_speed_offset + 1 ||
 		    size <= priv->info->fan_speed_offset + 1)
-			return 0;
+			goto unlock;
 
 		/* Received coolant temp and speeds of pump and internal fan */
 		priv->temp_input[0] = data[priv->info->temp_offset] * 1000 +
@@ -443,7 +447,7 @@ static int rog_ryujin_raw_event(struct hid_device *hdev, struct hid_report *repo
 			complete_all(&priv->cooler_status_received);
 	} else if (data[1] == RYUJIN_GET_CONTROLLER_SPEED_CMD_RESPONSE) {
 		if (size <= RYUJIN_CONTROLLER_SPEED_3 + 1)
-			return 0;
+			goto unlock;
 
 		/* Received speeds of four fans attached to the controller */
 		priv->speed_input[2] = get_unaligned_le16(data + RYUJIN_CONTROLLER_SPEED_1);
@@ -455,7 +459,7 @@ static int rog_ryujin_raw_event(struct hid_device *hdev, struct hid_report *repo
 			complete_all(&priv->controller_status_received);
 	} else if (data[1] == RYUJIN_GET_COOLER_DUTY_CMD_RESPONSE) {
 		if (size <= RYUJIN_INTERNAL_FAN_DUTY)
-			return 0;
+			goto unlock;
 
 		/* Received report for pump and internal fan duties (in %) */
 		if (data[RYUJIN_PUMP_DUTY] == 0 && data[RYUJIN_INTERNAL_FAN_DUTY] == 0) {
@@ -474,7 +478,7 @@ static int rog_ryujin_raw_event(struct hid_device *hdev, struct hid_report *repo
 				 * We're expecting a report, so parse it.
 				 */
 				goto read_cooler_duty;
-			return 0;
+			goto unlock;
 		}
 read_cooler_duty:
 		priv->duty_input[0] = rog_ryujin_percent_to_pwm(data[RYUJIN_PUMP_DUTY]);
@@ -484,7 +488,7 @@ static int rog_ryujin_raw_event(struct hid_device *hdev, struct hid_report *repo
 			complete_all(&priv->cooler_duty_received);
 	} else if (data[1] == RYUJIN_GET_CONTROLLER_DUTY_CMD_RESPONSE) {
 		if (size <= RYUJIN_CONTROLLER_DUTY)
-			return 0;
+			goto unlock;
 
 		/* Received report for controller duty for fans (in PWM) */
 		if (data[RYUJIN_CONTROLLER_DUTY] == 0) {
@@ -503,7 +507,7 @@ static int rog_ryujin_raw_event(struct hid_device *hdev, struct hid_report *repo
 				 * We're expecting a report, so parse it.
 				 */
 				goto read_controller_duty;
-			return 0;
+			goto unlock;
 		}
 read_controller_duty:
 		priv->duty_input[2] = data[RYUJIN_CONTROLLER_DUTY];
@@ -512,6 +516,8 @@ static int rog_ryujin_raw_event(struct hid_device *hdev, struct hid_report *repo
 			complete_all(&priv->controller_duty_received);
 	}
 
+unlock:
+	spin_unlock_irqrestore(&priv->status_report_request_lock, flags);
 	return 0;
 }
 
-- 
2.55.0

Re: [PATCH 2/2] hwmon: (asus_rog_ryujin) Synchronize HID command and report handling

[sashiko-bot]

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

Pre-existing issues:
- [High] The driver starts the hardware and registers hidraw before initializing the driver's spinlock and completions, leading to a potential NULL pointer dereference panic if a report arrives early.
--

commit 3c28258aa778caf1385fb4cb2e13b3f6eaf844f5
Author: Arie Miller <renari@arimil.com>

    hwmon: (asus_rog_ryujin) Synchronize HID command and report handling

    This commit fixes a race condition where a HID response can be lost because
    the event handler did not acquire the status lock before updating the
    completion. It modifies both the command execution and raw event paths to
    use irqsave spinlock variants to prevent the command from timing out.

    Closes: https://lore.kernel.org/linux-hwmon/20260812104617.858D01F000E9@smtp.kernel.org/

> diff --git a/drivers/hwmon/asus_rog_ryujin.c b/drivers/hwmon/asus_rog_ryujin.c
> index f4d99c5103691..e297557ca3463 100644
> --- a/drivers/hwmon/asus_rog_ryujin.c
> +++ b/drivers/hwmon/asus_rog_ryujin.c
[ ... ]
> @@ -421,15 +422,18 @@ static int rog_ryujin_raw_event(struct hid_device *hdev, struct hid_report *repo
>  				int size)
>  {
>  	struct rog_ryujin_data *priv = hid_get_drvdata(hdev);
> +	unsigned long flags;
>  
>  	if (size < 2 || data[0] != RYUJIN_CMD_PREFIX)
>  		return 0;
>  
> +	spin_lock_irqsave(&priv->status_report_request_lock, flags);

[Severity: High]
This is a pre-existing issue, but I am wondering if this could lead to a NULL
pointer dereference during initialization?

Looking at rog_ryujin_probe() in drivers/hwmon/asus_rog_ryujin.c, the hardware
data flow is started before the synchronization primitives used by the
interrupt handler are initialized:

rog_ryujin_probe() {
    ...
    ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
    ...
    ret = hid_hw_open(hdev);
    ...
    spin_lock_init(&priv->status_report_request_lock);
    init_completion(&priv->cooler_status_received);
    ...
}

If a USB device sends a HID report immediately after hid_hw_open() but before
probe() completes initialization of the spinlock and completions, the event
handler rog_ryujin_raw_event() will attempt to lock an uninitialized spinlock
and trigger completion logic on zero-allocated struct completion objects.

Would this result in a NULL pointer dereference in the waitqueue list handling
if a report arrives early?

> +
>  	if (data[1] == RYUJIN_GET_COOLER_STATUS_CMD_RESPONSE) {
>  		if (size <= priv->info->temp_offset + 1 ||
>  		    size <= priv->info->pump_speed_offset + 1 ||
>  		    size <= priv->info->fan_speed_offset + 1)
> -			return 0;
> +			goto unlock;

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

Re: [PATCH 1/2] hwmon: (asus_rog_ryujin) Validate HID report lengths

[sashiko-bot]

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

Pre-existing issues:
- [Medium] Missing synchronization when reading sensor data in sysfs callbacks, violating hwmon subsystem guidelines.
--

commit 4aa6deb585949a68bf55aaa6ef4f88906594c6f3
Author: Arie Miller <renari@arimil.com>

hwmon: (asus_rog_ryujin) Validate HID report lengths

This commit validates response headers and payload fields in the HID raw
event handler before parsing them. This prevents the driver from consuming
uninitialized bytes from short reports.

Link: https://lore.kernel.org/linux-hwmon/20260812104617.858D01F000E9@smtp.kernel.org/

> diff --git a/drivers/hwmon/asus_rog_ryujin.c b/drivers/hwmon/asus_rog_ryujin.c
> index 702edb8313945..f4d99c5103691 100644
> --- a/drivers/hwmon/asus_rog_ryujin.c
> +++ b/drivers/hwmon/asus_rog_ryujin.c
[ ... ]
> @@ -422,10 +422,15 @@ static int rog_ryujin_raw_event(struct hid_device *hdev, struct hid_report *repo
>  {
>  	struct rog_ryujin_data *priv = hid_get_drvdata(hdev);
>  
> -	if (data[0] != RYUJIN_CMD_PREFIX)
> +	if (size < 2 || data[0] != RYUJIN_CMD_PREFIX)
>  		return 0;
>  
>  	if (data[1] == RYUJIN_GET_COOLER_STATUS_CMD_RESPONSE) {
> +		if (size <= priv->info->temp_offset + 1 ||
> +		    size <= priv->info->pump_speed_offset + 1 ||
> +		    size <= priv->info->fan_speed_offset + 1)
> +			return 0;
> +
>  		/* Received coolant temp and speeds of pump and internal fan */
>  		priv->temp_input[0] = data[priv->info->temp_offset] * 1000 +
>  			data[priv->info->temp_offset + 1] * 100;

[Severity: Medium]
This isn't a bug introduced by this patch, but is it possible for a data
race to occur on the sensor data arrays like priv->temp_input between
this interrupt handler and the sysfs read callbacks?

While the sensor arrays are written to from the HID raw event handler
here in interrupt context while holding the status_report_request_lock
spinlock, the sysfs read callback appears to read the sensor values
locklessly in process context:

drivers/hwmon/asus_rog_ryujin.c:rog_ryujin_read() {
    ...
	case hwmon_temp:
		*val = priv->temp_input[channel];
		break;
    ...
}

Concurrently, an unsolicited hardware report or a report triggered by
another hidraw client can be processed by the HID interrupt handler in
rog_ryujin_raw_event(), overwriting the sensor values while they are
being read. This could lead to torn reads or undefined behavior.

The hwmon subsystem guidelines mandate that drivers must implement locking
required for interrupt handling. Could the driver take its
status_report_request_lock spinlock during rog_ryujin_read() to safely
synchronize with the raw event handler? This pre-existing issue remains
unfixed in the final state of the series.

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

Re: [PATCH 2/2] hwmon: (asus_rog_ryujin) Synchronize HID command and report handling

[Arimil]

On Thu, Sep 3, 2026 at 10:34 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] The driver starts the hardware and registers hidraw before initializing the driver's spinlock and completions, leading to a potential NULL pointer dereference panic if a report arrives early.
> --

This is a false positive, this driver does not call
hid_device_io_start() so there will be no early reports.

Re: [PATCH 1/2] hwmon: (asus_rog_ryujin) Validate HID report lengths

[Guenter Roeck]

On Thu, Sep 03, 2026 at 10:21:28PM -0400, Arie Miller wrote:
> rog_ryujin_raw_event() parses response headers and payload fields without
> first checking that they are present in the received report. A short report
> can therefore make the driver consume uninitialized bytes from the HID
> transport buffer and expose them as sensor values through sysfs.
> 
> Validate the response header and the fields used by each response type
> before parsing them.
> 
> Fixes: ed3e03790c5c ("hwmon: Add driver for ASUS ROG RYUJIN II 360 AIO cooler")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/linux-hwmon/20260812104617.858D01F000E9@smtp.kernel.org/
> Cc: stable@vger.kernel.org
> Assisted-by: Codex:gpt-5.6-sol sparse
> Signed-off-by: Arie Miller <renari@arimil.com>

Applied.

Thanks,
Guenter

Re: [PATCH 2/2] hwmon: (asus_rog_ryujin) Synchronize HID command and report handling

[Guenter Roeck]

On Thu, Sep 03, 2026 at 10:21:29PM -0400, Arie Miller wrote:
> rog_ryujin_execute_cmd() holds status_report_request_lock while
> reinitializing a completion, intending to exclude raw-event handling.
> However, rog_ryujin_raw_event() does not acquire the lock when it updates
> the completion. A response can therefore race with reinit_completion() and
> be lost, leaving the command to time out.
> 
> Hold the lock while parsing reports and updating their completions. Use the
> irqsave variants in both paths because raw-event handling may run in
> interrupt context.
> 
> Fixes: ed3e03790c5c ("hwmon: Add driver for ASUS ROG RYUJIN II 360 AIO cooler")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/linux-hwmon/20260812104617.858D01F000E9@smtp.kernel.org/
> Cc: stable@vger.kernel.org
> Assisted-by: Codex:gpt-5.6-sol sparse
> Signed-off-by: Arie Miller <renari@arimil.com>

Applied.

Thanks,
Guenter