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

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

Pre-existing issues:
- [High] Missing size validation in `rog_ryujin_raw_event` allows out-of-bounds memory reads.
- [High] The `status_report_request_lock` spinlock is ignored in `rog_ryujin_raw_event`, causing data races on completion states.
--

--- Patch [2]: [PATCH v2 2/4] hwmon: (asus_rog_ryujin) Add ROG Ryujin III support ---
commit b5b4a6af3fd216324c49f933284fb63343c21174
Author: Arie Miller <[email protected]>

hwmon: (asus_rog_ryujin) Add ROG Ryujin III support

This commit adds model data, USB IDs, and report offsets for the ROG Ryujin
III Extreme and EVA Edition variants. It also skips controller commands
and hides unavailable controller channels for these devices.

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 b86b87e33615d..5bc60c6036de3 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,
> +	.duty_channel = 1,
> +	.has_controller = false,
> +};
> +

[Severity: High]
This is a pre-existing issue, but does rog_ryujin_raw_event() adequately
validate the report size before using these offsets?

If a malicious USB device provides a descriptor with a small max report size
but sends a valid-looking command prefix, the driver will read offsets up to
priv->info->fan_speed_offset without checking if the report size is
sufficient.

Could this cause an out-of-bounds read when accessing the payload fields?

drivers/hwmon/asus_rog_ryujin.c:rog_ryujin_raw_event() {
    ...
    priv->temp_input[0] = data[priv->info->temp_offset] * 1000 +
                          data[priv->info->temp_offset + 1] * 100;
    priv->speed_input[0] =
            get_unaligned_le16(data + priv->info->pump_speed_offset);
    ...
}

[Severity: High]
This is also a pre-existing issue, but is the status_report_request_lock
effective if it is ignored in rog_ryujin_raw_event()?

rog_ryujin_execute_cmd() acquires status_report_request_lock to protect
reinit_completion():

drivers/hwmon/asus_rog_ryujin.c:rog_ryujin_execute_cmd() {
    ...
    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() ignores this lock when calling complete_all():

drivers/hwmon/asus_rog_ryujin.c:rog_ryujin_raw_event() {
    ...
    if (!completion_done(&priv->cooler_status_received))
        complete_all(&priv->cooler_status_received);
    ...
}

Could this lead to data races on the completion states and spurious sysfs
read/write timeouts when a USB raw event arrives concurrently with a
driver sysfs command?

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