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

[sashiko-bot@kernel.org]

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