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

[sashiko-bot@kernel.org]

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