Re: [PATCH v2 4/4] hwmon: (asus_rog_ryujin) Handle missing driver data

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

Pre-existing issues:
- [High] Missing size validation of incoming HID reports in `rog_ryujin_raw_event` leads to out-of-bounds heap reads.
- [Medium] Early sysfs interaction causes spurious `-ETIMEDOUT` errors because HID I/O is not started.
- [Medium] The `status_report_request_lock` spinlock provides no mutual exclusion against `rog_ryujin_raw_event`, leading to a race condition on completions.
--

--- Patch [4]: [PATCH v2 4/4] hwmon: (asus_rog_ryujin) Handle missing driver data ---
commit ec4154bc4d0c91156d0d07b375b65f694f8c0199
Author: Arie Miller <[email protected]>

hwmon: (asus_rog_ryujin) Handle missing driver data

Handles an issue reported by Sashiko where an id could
be missing driver_data.

Link: https://lore.kernel.org/r/[email protected]

> diff --git a/drivers/hwmon/asus_rog_ryujin.c b/drivers/hwmon/asus_rog_ryujin.c
> index 9b34c6e06bf78..702edb8313945 100644
> --- a/drivers/hwmon/asus_rog_ryujin.c
> +++ b/drivers/hwmon/asus_rog_ryujin.c
> @@ -506,6 +506,9 @@ static int rog_ryujin_probe(struct hid_device *hdev, const struct hid_device_id
>  	struct rog_ryujin_data *priv;
>  	int ret;
>  
> +	if (!id->driver_data)
> +		return -EINVAL;
> +
>  	priv = devm_kzalloc(&hdev->dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;

[Severity: High]
This isn't a bug introduced by this patch, but is there a missing size
validation in rog_ryujin_raw_event()?

If a malicious or malfunctioning HID device sends a short report matching the
expected prefix but lacking the required data fields, the size parameter
doesn't seem to be validated:

drivers/hwmon/asus_rog_ryujin.c:rog_ryujin_raw_event() {
    ...
	if (data[1] == RYUJIN_GET_COOLER_STATUS_CMD_RESPONSE) {
		/* Received coolant temp and speeds of pump and internal fan */
		priv->temp_input[0] = data[priv->info->temp_offset] * 1000 +
    ...
}

Could accessing fixed and dynamic offsets here lead to an out-of-bounds heap
read, potentially resulting in a KASAN crash or leaking adjacent heap memory
contents?

[Severity: Medium]
This is a pre-existing issue, but can early sysfs interaction cause
spurious -ETIMEDOUT errors during device initialization?

The hwmon device is registered and exposed to userspace here:

drivers/hwmon/asus_rog_ryujin.c:rog_ryujin_probe() {
    ...
	priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "rog_ryujin",
							  priv, &rog_ryujin_chip_info, NULL);
    ...
}

If userspace accesses the hwmon sysfs attributes immediately after they are
created, but before the probe() callback returns, will incoming hardware
reports be rejected with -EBUSY because __hid_device_probe() holds
driver_input_lock?

Since HID I/O is not explicitly started before registering the hwmon device,
might this race condition lead to dropped HID events?

[Severity: Medium]
This isn't a bug introduced by this patch, but does the
status_report_request_lock spinlock actually protect the completion structures?

The spinlock is acquired 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, the event handler accesses and modifies the completion structures
without acquiring status_report_request_lock:

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

If complete_all() is called concurrently with reinit_completion() from a
userspace sysfs read/write, could this race condition corrupt the completion
done count and cause sysfs operations to fail with -ETIMEDOUT or return stale
data?

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