Re: [hwmon PATCH v3 3/6] hwmon: (adt7470) Fix busy-loop and I2C flooding in update thread

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

Pre-existing issues:
- [High] The temperature alarm reporting logic uses the raw integer channel index instead of a bitmask, resulting in incorrect alarm reporting.
- [High] In `pwm_auto_temp_store`, user input overflows a hardware bit-field due to missing bounds checks, and is written to the wrong state array, corrupting the PWM automatic enable state.
--

--- Patch [3]: [hwmon PATCH v3 3/6] hwmon: (adt7470) Fix busy-loop and I2C flooding in update thread ---
commit c10c388e55482dcd15df4bd6225052bbff835213
Author: Luiz Angelo Daros de Luca <[email protected]>

hwmon: (adt7470) Fix busy-loop and I2C flooding in update thread

Setting 'auto_update_interval' and 'num_temp_sensors' to 0 forces the
background thread into a tight, unbounded busy-loop, hogging the CPU and
flooding the I2C bus. This patch fixes the vulnerability by raising the
lower limit in auto_update_interval_store() to 500 milliseconds. This
guarantees a reasonable minimum sleep window between sensor updates.

Reported-by: [email protected]
Closes: https://lore.kernel.org/r/[email protected]
Fixes: 89fac11cb3e7 ("adt7470: make automatic fan control really work")
Signed-off-by: Luiz Angelo Daros de Luca <[email protected]>

> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> index 62ec68ea0a406..0b19b0925d1c7 100644
> --- a/drivers/hwmon/adt7470.c
> +++ b/drivers/hwmon/adt7470.c
> @@ -509,7 +509,7 @@ static ssize_t auto_update_interval_store(struct device *dev,
>  	if (kstrtol(buf, 10, &temp))
>  		return -EINVAL;
>  
> -	temp = clamp_val(temp, 0, 60000);
> +	temp = clamp_val(temp, 500, 60000);
>  
>  	mutex_lock(&data->lock);
>  	data->auto_update_interval = temp;

While reviewing the code, I found a few unrelated issues in this driver.

drivers/hwmon/adt7470.c:adt7470_temp_read() {
...
	case hwmon_temp_min:
		*val = 1000 * data->temp_min[channel];
		break;
	case hwmon_temp_max:
		*val = 1000 * data->temp_max[channel];
		break;
	case hwmon_temp_alarm:
		*val = !!(data->alarm & channel);
		break;
...
}

[Severity: High]
This isn't a bug introduced by this patch, but is the temperature alarm
reporting logic using the raw integer channel index instead of a bitmask?

If channel is 0 (temp1_alarm), data->alarm & 0 is always 0. For other
channels, it checks bit 0, 1, 2, or 3 incorrectly, which might lead to
false positives or missed alarms.


drivers/hwmon/adt7470.c:pwm_auto_temp_store() {
...
	temp = cvt_auto_temp(temp);
	if (temp < 0)
		return temp;

	mutex_lock(&data->lock);
	data->pwm_automatic[attr->index] = temp;

	if (!(attr->index % 2)) {
		mask = 0xF0;
		val = (temp << 4) & 0xF0;
	} else {
		mask = 0x0F;
		val = temp & 0x0F;
	}
...
}

[Severity: High]
This is a pre-existing issue, but does this write the parsed value into the
wrong state array, and can the unchecked input overflow the hardware
bit-field?

It appears to overwrite the pwm_automatic enable state instead of updating
data->pwm_auto_temp, corrupting the PWM automatic enable state.

Additionally, since cvt_auto_temp() can return a value up to 32, it can
overflow the 4-bit field mask when constructing the register value.

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