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

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch's attempt to fix the out-of-bounds read is incomplete. The newly added `aqc_raw_data_valid()` deliberately skips bounds checking for special-case sensors (like `leakshield`, `aquastreamult`, and `highflownext`). An attacker can supply a HID report that passes the initial generic length checks, but is too small for the special-case offsets (e.g., `LEAKSHIELD_PRESSURE_ADJUSTED` at offset 285). The switch statement in `aqc_raw_event()` then unconditionally reads these out-of-bounds offsets.
- [High] This problem wasn't introduced by this patch, but a malicious HID device mimicking `aquastreamxt` can trigger a NULL pointer dereference in `aqc_raw_event()`. For `aquastreamxt`, `priv->fan_structure` is deliberately left uninitialized (NULL) during `probe()`. The patch author added `if (!priv->fan_structure) goto flow;` in `aqc_raw_data_valid()` to skip checking it, but missed that the loop in `aqc_raw_event()` will still execute because `priv->num_fans` is 2. The loop unconditionally dereferences `priv->fan_structure->speed`, causing an immediate kernel panic.
- [High] This problem wasn't introduced by this patch, but there is a missing bounds check on `hdev->maxcollection` before accessing `hdev->collection[1]` in `aqc_probe()`. A malicious USB device can present a HID report descriptor that defines 0 or 1 collections, leading to an out-of-bounds array read when the driver checks `hdev->collection[1].type`.
--

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

hwmon: (aquacomputer_d5next) valid the data size before reading the sensor data

The user-forged sensor data is only 65 bytes long; however, aqc_raw_event()
fails to handle cases where the sensor data length is too small when reading
the data, resulting in [1] during the read process.

Add a data size check, if the size is less than that required for the
specific data item to be read, abort the sensor data read operation.

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

> diff --git a/drivers/hwmon/aquacomputer_d5next.c b/drivers/hwmon/aquacomputer_d5next.c
> index 1ca70e7262988..89c9fc0c77e52 100644
> --- a/drivers/hwmon/aquacomputer_d5next.c
> +++ b/drivers/hwmon/aquacomputer_d5next.c
> @@ -1324,6 +1324,107 @@ static const struct hwmon_chip_info aqc_chip_info = {
>  	.info = aqc_info,
>  };
>  
> +/* aqc_raw_data_valid()
> + * Does not support special-case sensor readings data size check
> + */

[Severity: High]
This problem wasn't introduced by this patch, but the patch's attempt to fix
the out-of-bounds read seems incomplete. The newly added aqc_raw_data_valid()
deliberately skips bounds checking for special-case sensors (like leakshield,
aquastreamult, and highflownext).

Could an attacker supply a HID report that passes the initial generic length
checks, but is too small for the special-case offsets?

For example, LEAKSHIELD_PRESSURE_ADJUSTED is at offset 285. The switch
statement in aqc_raw_event() then unconditionally reads these out-of-bounds
offsets:

	case leakshield:
		priv->speed_input[0] =
		    ((s16)get_unaligned_be16(data + LEAKSHIELD_PRESSURE_ADJUSTED)) * 100;

> +static bool aqc_raw_data_valid(struct aqc_data *priv, int size)
> +{
[ ... ]
> +	/* Fan speed and related readings data size check */
> +	if (!priv->fan_structure)
> +		goto flow;

[Severity: High]
This isn't a bug introduced by this patch, but could this cause a NULL pointer
dereference?

For aquastreamxt, priv->fan_structure is deliberately left uninitialized
(NULL) during probe(). The check above skips bounds checking when
fan_structure is NULL, but misses that the loop in aqc_raw_event() will still
execute because priv->num_fans is 2.

The loop in aqc_raw_event() unconditionally dereferences priv->fan_structure:

	for (i = 0; i < priv->num_fans; i++) {
		priv->speed_input[i] =
		    get_unaligned_be16(data + priv->fan_sensor_offsets[i] +
				       priv->fan_structure->speed);

Could a malicious HID device mimicking aquastreamxt trigger a kernel panic
here?

[ ... ]

[Severity: High]
This is a pre-existing issue, but is there a missing bounds check on
hdev->maxcollection before accessing hdev->collection[1] in aqc_probe()?

If a malicious USB device presents a HID report descriptor that defines 0 or 1
collections, could the driver perform an out-of-bounds array read when it
checks hdev->collection[1].type:

drivers/hwmon/aquacomputer_d5next.c:aqc_probe() {
	...
	if (hdev->collection[1].type != 0) {
		ret = -ENODEV;
		goto fail_and_close;
	}
	...
}

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