[PATCH] hwmon: (aspeed-g6-pwm-tach) Guard fan RPM calculation against divide-by-zero

[PATCH] hwmon: (aspeed-g6-pwm-tach) Guard fan RPM calculation against divide-by-zero

[Guenter Roeck]

Sashiko reports:

In the aspeed-g6-pwm-tacho driver, the aspeed_tach_val_to_rpm() function
calculates the fan RPM using the tachometer value. However, it does not
check if the tachometer value is zero before performing the division.

If the hardware reports a tachometer value of 0 (which can happen due to
an extremely fast pulse, a stuck edge, or a hardware glitch), the
calculated tach_div evaluates to 0. The subsequent call to do_div() with
tach_div as the divisor triggers a divide-by-zero exception, leading to
a kernel panic.

Check the divisor against zero to fix the problem.

Fixes: 7e1449cd15d1 ("hwmon: (aspeed-g6-pwm-tacho): Support for ASPEED g6 PWM/Fan tach")
Cc: Billy Tsai <billy_tsai@aspeedtech.com>
Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/aspeed-g6-pwm-tach.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/aspeed-g6-pwm-tach.c b/drivers/hwmon/aspeed-g6-pwm-tach.c
index 4f6e6d440dd4..5d611a8e5269 100644
--- a/drivers/hwmon/aspeed-g6-pwm-tach.c
+++ b/drivers/hwmon/aspeed-g6-pwm-tach.c
@@ -293,7 +293,10 @@ static int aspeed_tach_val_to_rpm(struct aspeed_pwm_tach_data *priv, u32 tach_va
 		priv->clk_rate, tach_val, tach_div);
 
 	rpm = (u64)priv->clk_rate * 60;
-	do_div(rpm, tach_div);
+	if (tach_div)
+		do_div(rpm, tach_div);
+	else
+		rpm = 0;
 
 	return (int)rpm;
 }
-- 
2.45.2

Re: [PATCH] hwmon: (aspeed-g6-pwm-tach) Guard fan RPM calculation against divide-by-zero

[sashiko-bot]

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

Pre-existing issues:
- [High] The driver maintains a single global `tach_divisor` variable shared across all fan channels, causing cross-channel corruption of RPM calculations.
- [High] Out-of-bounds array write and MMIO access when `tach-ch` values from the device tree exceed the maximum allowed channels.
--

commit f0b2d4a2512939274908261753aaf7016a48ac2e
Author: Guenter Roeck <linux@roeck-us.net>

hwmon: (aspeed-g6-pwm-tach) Guard fan RPM calculation against divide-by-zero

This commit fixes a potential divide-by-zero exception in the
aspeed-g6-pwm-tacho driver. It checks if the calculated tach_div is zero
before calling do_div() and sets the rpm to zero instead, preventing a
kernel panic on invalid tachometer values.

> diff --git a/drivers/hwmon/aspeed-g6-pwm-tach.c b/drivers/hwmon/aspeed-g6-pwm-tach.c
> index 4f6e6d440dd40..5d611a8e52690 100644
> --- a/drivers/hwmon/aspeed-g6-pwm-tach.c
> +++ b/drivers/hwmon/aspeed-g6-pwm-tach.c
> @@ -293,7 +293,10 @@ static int aspeed_tach_val_to_rpm(struct aspeed_pwm_tach_data *priv, u32 tach_va
>  		priv->clk_rate, tach_val, tach_div);
>  
>  	rpm = (u64)priv->clk_rate * 60;
> -	do_div(rpm, tach_div);
> +	if (tach_div)

[Severity: High]
This is a pre-existing issue, but does this driver maintain a single global
tach_divisor shared across all fan channels?

Looking at the calculation of tach_div earlier in aspeed_tach_val_to_rpm():

tach_div = tach_val * priv->tach_divisor * DEFAULT_FAN_PULSE_PR;

It appears priv->tach_divisor is a single scalar variable in
aspeed_pwm_tach_data rather than a per-channel array. If a user or thermal
daemon writes to hwmon_fan_div for one channel in aspeed_tach_hwmon_write(),
it overwrites this global priv->tach_divisor.

Subsequent RPM reads for all other channels would then use this overwritten
value instead of their respective hardware-configured divisors, potentially
misleading thermal management daemons with incorrect fan speeds.

> +		do_div(rpm, tach_div);
> +	else
> +		rpm = 0;
>  
>  	return (int)rpm;
>  }

[Severity: High]
This is another pre-existing issue in this driver, but is there a risk of
out-of-bounds array writes and MMIO accesses when reading tach-ch values
from the device tree?

In aspeed_create_fan_monitor(), the driver reads raw channel numbers:

ret = of_property_read_u8_array(child, "tach-ch", tach_ch, count);

Then in aspeed_present_fan_tach(), it uses the unvalidated channel value
to index into the statically-sized tach_present array:

priv->tach_present[ch] = true;

It also uses the unvalidated channel value to calculate MMIO offsets:

val = readl(priv->base + TACH_ASPEED_CTRL(ch));

If the device tree provides a channel number that exceeds the maximum
allowed channels, could this corrupt adjacent memory and trigger
out-of-bounds MMIO accesses?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629232305.2013406-1-linux@roeck-us.net?part=1