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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Dynamic sysfs IDs (new_id) binding will cause a NULL pointer dereference because `id->driver_data` is unchecked.

Pre-existing issues:
- [High] Missing bounds check on HID report size before accessing payload offsets in `rog_ryujin_raw_event`, leading to an out-of-bounds read and information leak.
- [High] The `status_report_request_lock` fails to protect `reinit_completion()` from concurrent `complete_all()` calls in `rog_ryujin_raw_event`.
- [High] The device is exposed to HID raw events before its completions and locks are initialized, risking a NULL pointer dereference and kernel crash.
- [High] The driver allocates a DMA buffer using `devm_kzalloc()`, which breaks cacheline alignment and causes memory corruption on non-coherent DMA architectures.
--

--- Patch [1]: [PATCH 1/3] hwmon: (asus_rog_ryujin) Add per-device configuration ---
commit 532faa30887394da7e60da86be21849f6a4f86c4
Author: Arie Miller <renari@arimil.com>

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: High]
This is a pre-existing issue, but does this code lack a bounds check on the
incoming HID report size?

If a malicious device sends a truncated report that is smaller than the
expected payload, the size parameter is ignored. Accessing offsets like
data[priv->info->temp_offset] could result in an out-of-bounds read.

Can we add a check against the size parameter before accessing the payload?

> +		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 is a pre-existing issue, but is there a data race between this
complete_all() call and reinit_completion()?

In rog_ryujin_execute_cmd(), the status_report_request_lock is used:

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

However, complete_all() is called here in the raw event handler without
acquiring status_report_request_lock. If the device sends an unsolicited
report or commands are interleaved, could this lead to corrupted completion
state or missed wakeups?

[ ... ]
> @@ -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: High]
When binding the driver dynamically via the new_id sysfs interface,
id->driver_data can be NULL if the data field is omitted. 

If priv->info is assigned NULL here, won't this cause a panic when the hwmon
core subsequently queries attribute visibility during device registration?

hwmon_device_register_with_info()
   rog_ryujin_is_visible()
      if (channel >= 2 && !priv->info->has_controller)
                                 ^^^^

Could we add a check to validate id->driver_data before assignment?

>  	hid_set_drvdata(hdev, priv);
>  
[ ... ]
> 	ret = hid_parse(hdev);
> 	if (ret) {
> 		hid_err(hdev, "hid parse failed with %d\n", ret);
> 		return ret;
> 	}
> 
> 	/* Enable hidraw so existing user-space tools can continue to work */
> 	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> 	if (ret) {
> 		hid_err(hdev, "hid hw start failed with %d\n", ret);
> 		return ret;
> 	}
> 
> 	ret = hid_hw_open(hdev);

[Severity: High]
This is a pre-existing issue, but is the device exposed to HID raw events
before its completion structures are initialized?

If hid_hw_start() and hid_hw_open() are called before init_completion()
initializes queues like cooler_status_received, and the device sends an
immediate interrupt report:

rog_ryujin_raw_event()
   complete_all(&priv->cooler_status_received)

Could this dereference a NULL pointer since the completions were only just
allocated with devm_kzalloc() and have not yet been initialized?

> 	if (ret) {
> 		hid_err(hdev, "hid hw open failed with %d\n", ret);
> 		goto fail_and_stop;
> 	}
> 
> 	priv->buffer = devm_kzalloc(&hdev->dev, MAX_REPORT_LENGTH, GFP_KERNEL);

[Severity: High]
This is a pre-existing issue, but is it safe to use devm_kzalloc() for a DMA
buffer?

When this buffer is later passed to hid_hw_output_report() in
rog_ryujin_write_expanded(), it is inherently mapped for DMA.

Because devm_kzalloc() embeds a devres header, the returned pointer is not
cacheline aligned. On non-coherent architectures, cache invalidation during
DMA operations could corrupt the adjacent devres header or preceding memory.

Should this buffer be allocated with a non-devm allocator like kzalloc()
to guarantee proper alignment?

> 	if (!priv->buffer) {
> 		ret = -ENOMEM;
> 		goto fail_and_close;
> 	}
> 
> 	spin_lock_init(&priv->status_report_request_lock);
> 	init_completion(&priv->cooler_status_received);

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