Re: [PATCH v2] hwmon: valid the data size before reading the sensor data

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

New issues:
- [High] The bounds checking logic in `aqc_raw_data_valid` uses an incorrect comparison, resulting in off-by-one errors that allow 1-byte out-of-bounds reads.
- [High] `aqc_raw_data_valid` incorrectly assumes `fan_sensor_offsets` is sorted in ascending order, failing to validate larger offsets at lower array indices.

Pre-existing issues:
- [High] `aqc_raw_data_valid` completely omits bounds checks for several offsets that are subsequently read in `aqc_raw_event`, leaving the driver vulnerable to out-of-bounds reads.
- [High] Processing a crafted report for legacy devices like `aquastreamxt` triggers a NULL pointer dereference on `priv->fan_structure`.
--

--- Patch [1]: [PATCH v2] hwmon: valid the data size before reading the sensor data ---
commit a13b90e2804a6e0d888f292535c33b47d5d8f7fb
Author: Edward Adam Davis <[email protected]>

hwmon: 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.

Reported-by: [email protected]
Closes: https://syzkaller.appspot.com/bug?extid=9ee5f5dc18673d6b2f37
Tested-by: [email protected]

> diff --git a/drivers/hwmon/aquacomputer_d5next.c b/drivers/hwmon/aquacomputer_d5next.c
> index 1ca70e7262988..c6fe36abfe569 100644
> --- a/drivers/hwmon/aquacomputer_d5next.c
> +++ b/drivers/hwmon/aquacomputer_d5next.c

[ ... ]

> @@ -1324,6 +1324,76 @@ static const struct hwmon_chip_info aqc_chip_info = {
>  	.info = aqc_info,
>  };
>  
> +static bool aqc_raw_data_valid(struct aqc_data *priv, int size)
> +{
> +	int off;
> +	char *msg;
> +
> +	if (!priv)
> +		return false;
> +
> +	off = priv->serial_number_start_offset + SERIAL_PART_OFFSET;
> +	if (off >= size) {

[Severity: High]
Does this bounds check adequately protect against multi-byte reads?

The condition if (off >= size) evaluates to false when size == off + 1.
However, aqc_raw_event() uses get_unaligned_be16(), which reads 2 bytes.

If the device sends a report with a size exactly equal to off + 1, won't
reading 2 bytes at offset size - 1 access one byte beyond the buffer?

[ ... ]

> +	/* Fan speed and related readings data size check */
> +	if (priv->num_fans > 0) {
> +		int fan_off = priv->fan_sensor_offsets[priv->num_fans - 1];

[Severity: High]
Does this code assume that the fan_sensor_offsets array is always sorted in
ascending order?

For devices like d5next, the array is initialized as:
{ D5NEXT_PUMP_OFFSET (108), D5NEXT_FAN_OFFSET (95) }

The check uses the last index (95) for bounds validation. This would pass a
report of size 104, but aqc_raw_event() will subsequently access index 0
(108), resulting in an out-of-bounds read.

> +
> +		off = fan_off + priv->fan_structure->power;

[Severity: High]
This isn't a bug introduced by this patch, but can priv->fan_structure be
NULL here?

For legacy devices like aquastreamxt, priv->num_fans is set to 2 during
probe, but priv->fan_structure is never assigned and remains NULL. While
this NULL dereference already existed in aqc_raw_event(), this patch adds
another immediate dereference here.

> +		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) {

[Severity: High]
This is a pre-existing issue, but does this adequately cover all fields
accessed in aqc_raw_event()?

This validation omits checks for several offsets, such as:
- priv->fan_structure->speed
- priv->flow_sensors_start_offset
- priv->power_cycle_count_offset
- Device-specific offsets inside the switch (priv->kind) block (e.g.,
  HIGHFLOWNEXT_POWER).

Since aqc_raw_event() unconditionally reads these fields, an out-of-bounds
read could still occur for truncated reports.

> +			msg = "fan curr offset";
> +			goto invalid;
> +		}
> +	}

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