Re: [PATCH v5] 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] Incomplete bounds check leaves special-case sensor readings vulnerable to out-of-bounds reads.
- [High] Malicious HID report can trigger a NULL pointer dereference for `aquastreamxt` devices.
--

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

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

This commit adds a data size check for sensor readings in the
aquacomputer_d5next driver to prevent out-of-bounds memory accesses when
processing truncated HID reports.

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

> diff --git a/drivers/hwmon/aquacomputer_d5next.c b/drivers/hwmon/aquacomputer_d5next.c
> index 1ca70e7262988..352b227c30159 100644
> --- a/drivers/hwmon/aquacomputer_d5next.c
> +++ b/drivers/hwmon/aquacomputer_d5next.c
> @@ -1324,6 +1324,84 @@ 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 is a pre-existing issue, but since this patch aims to fix out-of-bounds
reads during report processing, is it safe to leave special-case sensor
readings unvalidated?

If a malicious USB device presenting as leakshield, highflownext, or
aquastreamult sends a truncated HID report, it could pass this new generic
validation but still be too small for the special-case fields accessed later.

For example, in aqc_raw_event():

drivers/hwmon/aquacomputer_d5next.c:aqc_raw_event() {
    ...
    switch (priv->kind) {
    ...
    case leakshield:
        priv->speed_input[0] =
            ((s16)get_unaligned_be16(data + LEAKSHIELD_PRESSURE_ADJUSTED)) * 100;
    ...
}

Could this lead to another slab-out-of-bounds read if the report is too small
to contain these offsets?

> +static bool aqc_raw_data_valid(struct aqc_data *priv, int size)
> +{
> +	int off, fan_off, i;
> +
> +	if (!priv)
> +		return false;

[ ... ]

> +	/* Virtual temperature sensor */
> +	for (i = 0; i < priv->num_virtual_temp_sensors; i++) {
> +		off = priv->virtual_temp_sensor_start_offset +
> +		      i * AQC_SENSOR_SIZE + 1;
> +
> +		if (off >= size)
> +			goto invalid;
> +	}
> +
> +	/* Fan speed and related */
> +	if (!priv->fan_structure)
> +		goto flow;

[Severity: High]
This is a pre-existing issue, but does skipping this validation when
priv->fan_structure is NULL expose a NULL pointer dereference later in
aqc_raw_event()?

For devices like aquastreamxt, priv->fan_structure is NULL but priv->num_fans
is greater than 0. Later in aqc_raw_event(), the code loops over num_fans and
unconditionally dereferences fan_structure:

drivers/hwmon/aquacomputer_d5next.c:aqc_raw_event() {
    ...
    /* Fan speed and related readings */
    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 this crash the kernel if a malicious USB device sends a status report
for such a device?

> +
> +	for (i = 0; i < priv->num_fans; i++) {
> +		fan_off = priv->fan_sensor_offsets[i] + 1;
> +		off = fan_off + priv->fan_structure->speed;

[ ... ]

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