Re: [PATCH v3] hwmon: (aquacomputer_d5next) valid the data size before reading the sensor data

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

Pre-existing issues:
- [High] The newly added validation function `aqc_raw_data_valid` is incomplete and leaves multiple out-of-bounds reads unhandled.
- [High] NULL pointer dereference when a malicious USB device spoofing an `aquastreamxt` device sends a STATUS_REPORT_ID (0x01) input report.
--

--- Patch [1]: [PATCH v3] hwmon: (aquacomputer_d5next) valid the data size before reading the sensor data ---
commit f2f8bc6cd48d08e98637bc3e14b8b66f1ed8bcd4
Author: Edward Adam Davis <[email protected]>
Subject: hwmon: (aquacomputer_d5next) valid the data size before reading the sensor data

This commit adds a data size check function, aqc_raw_data_valid(), to prevent
out-of-bounds reads when a device sends a truncated HID report. It aborts the
sensor data read operation if the received data size is smaller than required.

Link: https://syzkaller.appspot.com/bug?extid=9ee5f5dc18673d6b2f37

> diff --git a/drivers/hwmon/aquacomputer_d5next.c b/drivers/hwmon/aquacomputer_d5next.c
> index 1ca70e7262988..2381a74eb4a48 100644
> --- a/drivers/hwmon/aquacomputer_d5next.c
> +++ b/drivers/hwmon/aquacomputer_d5next.c
[ ... ]
> +	/* Fan speed and related readings data size check */
> +	for (i = 0; i < priv->num_fans; i++) {
> +		fan_off = priv->fan_sensor_offsets[i] + 1;
> +		off = fan_off + priv->fan_structure->power;

[Severity: High]
This is a pre-existing issue, but does this code lead to a NULL pointer
dereference for aquastreamxt devices?

For legacy aquastreamxt devices, priv->num_fans is initialized to 2,
but priv->fan_structure is left unassigned (NULL). Legacy devices
normally communicate using report ID 0x04. If a malicious USB device
spoofs an aquastreamxt device and sends a report with
STATUS_REPORT_ID (0x01), would aqc_raw_data_valid() and
aqc_raw_event() blindly dereference priv->fan_structure->power?

Additionally, while this patch attempts to fix the out-of-bounds
reads, are we missing bounds checking for priv->fan_structure->speed
here?

The speed attribute often has the largest offset
(e.g., AQC_FAN_SPEED_OFFSET is 0x08, while power is 0x06). Could a
truncated report still cause an out-of-bounds read when accessing
the speed offset later in aqc_raw_event()?

> +		if (off >= size) {
> +			msg = "fan power offset";
> +			goto invalid;
> +		}
> +
> +		off = fan_off + priv->fan_structure->voltage;
> +		if (off >= size) {
> +			msg = "fan voltage offset";
> +			goto invalid;
> +		}
> +
> +		off = fan_off + priv->fan_structure->curr;
> +		if (off >= size) {
> +			msg = "fan curr offset";
> +			goto invalid;
> +		}
> +	}
> +

[Severity: High]
This is a pre-existing issue, but does this function leave several
out-of-bounds reads unprotected?

The new aqc_raw_data_valid() function skips checking
priv->flow_sensors_start_offset, priv->power_cycle_count_offset, and
device-specific sensors (like LEAKSHIELD_RESERVOIR_VOLUME). If a
malicious USB device sends a short HID report with STATUS_REPORT_ID,
could these reads still trigger a slab-out-of-bounds access inside
aqc_raw_event()?

> +	return true;
> +invalid:
> +	pr_debug("data size (%d) is less than the %s, %s\n",
> +		 size, msg, __func__);
> +	return false;
> +}

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