Re: [PATCH v2 1/4] hwmon: (asus_rog_ryujin) Add per-device configuration

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

New issues:
- [Critical] NULL pointer dereference when device is bound via sysfs `new_id`.

Pre-existing issues:
- [Critical] Out-of-bounds read in raw event handler due to missing size validation.
- [High] Data race on completion structure leading to missed wakeups.
--

--- Patch [1]: [PATCH v2 1/4] hwmon: (asus_rog_ryujin) Add per-device configuration ---
commit 471ae8f11e4ed856f2112cef2beadd2e4b019b6e
Author: Arie Miller <[email protected]>

hwmon: (asus_rog_ryujin) Add per-device configuration

Move model-specific report offsets and capabilities into a device
information structure. This prepares the driver for coolers which use
a different report layout or do not include the external fan
controller, while preserving the existing Ryujin II 360 behavior.

> diff --git a/drivers/hwmon/asus_rog_ryujin.c b/drivers/hwmon/asus_rog_ryujin.c
> index 10a1f5aca988d..b86b87e33615d 100644
> --- a/drivers/hwmon/asus_rog_ryujin.c
> +++ b/drivers/hwmon/asus_rog_ryujin.c
[ ... ]
> @@ -394,10 +416,12 @@ static int rog_ryujin_raw_event(struct hid_device *hdev, struct hid_report *repo
>  
>  	if (data[1] == RYUJIN_GET_COOLER_STATUS_CMD_RESPONSE) {
>  		/* Received coolant temp and speeds of pump and internal fan */
> -		priv->temp_input[0] =
> -		    data[RYUJIN_TEMP_SENSOR_1] * 1000 + data[RYUJIN_TEMP_SENSOR_2] * 100;
> -		priv->speed_input[0] = get_unaligned_le16(data + RYUJIN_PUMP_SPEED);
> -		priv->speed_input[1] = get_unaligned_le16(data + RYUJIN_INTERNAL_FAN_SPEED);
> +		priv->temp_input[0] = data[priv->info->temp_offset] * 1000 +
> +			data[priv->info->temp_offset + 1] * 100;

[Severity: Critical]
This is a pre-existing issue, but the driver appears to access data[0] and
other offsets without verifying the size of the buffer.

If a malicious device sends a truncated HID report, could this read past
the end of the allocated heap buffer since rog_ryujin_raw_event() extracts
values at offsets like priv->info->temp_offset without verifying that the
size parameter covers these bounds?

> +		priv->speed_input[0] =
> +			get_unaligned_le16(data + priv->info->pump_speed_offset);
> +		priv->speed_input[1] =
> +			get_unaligned_le16(data + priv->info->fan_speed_offset);
>  
>  		if (!completion_done(&priv->cooler_status_received))
>  			complete_all(&priv->cooler_status_received);

[Severity: High]
This isn't a bug introduced by this patch, but there seems to be a data
race on the completion structure between rog_ryujin_execute_cmd() and
rog_ryujin_raw_event().

In rog_ryujin_execute_cmd(), the driver attempts to protect the
reinit_completion() call using spin_lock_bh(&priv->status_report_request_lock).
However, spin_lock_bh() only disables softirqs on the local CPU.

Since rog_ryujin_raw_event() (running in softirq context on another CPU)
calls complete_all() without acquiring status_report_request_lock, could
this lockless write race directly with the locked increment, leading to
state corruption and sysfs read timeouts?

[ ... ]
> @@ -476,6 +500,7 @@ static int rog_ryujin_probe(struct hid_device *hdev, const struct hid_device_id
>  		return -ENOMEM;
>  
>  	priv->hdev = hdev;
> +	priv->info = (const struct rog_ryujin_device_info *)id->driver_data;

[Severity: Critical]
If a user binds a new USB device ID using the sysfs new_id interface, the
HID subsystem creates a hid_device_id struct with a zero-initialized
driver_data field.

Does this mean id->driver_data can be NULL here? If so, when the hwmon
core calls rog_ryujin_is_visible() during registration, will it trigger a
NULL pointer dereference when evaluating !priv->info->has_controller?

>  	hid_set_drvdata(hdev, priv);
>  
>  	/*

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