Re: [PATCH] hwmon: (aspeed-g6-pwm-tach) Validate the number of tach channels

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Input validation is performed mid-loop during hardware state mutation, leaving previously processed fan channels permanently enabled in hardware if a later channel fails validation.

Pre-existing issues:
- [High] A single global `priv->tach_divisor` variable is incorrectly shared across all fan channels, corrupting RPM calculations for independent channels.
- [High] Integer overflow and lack of zero-checks in `aspeed_tach_val_to_rpm` can lead to division by zero panics or wildly incorrect RPM values.
- [Medium] Probe error handling returns `0` when fan monitor creation fails, silently abandoning hardware monitoring registration while leaving the device successfully bound.
- [Medium] Duty cycle calculations in `aspeed_pwm_apply` can overflow a 64-bit integer, resulting in a near-zero duty cycle for very large requested periods.
--

commit 1d7e6c15019c0fdc2af8d78d76b900c9f2bb4d7e
Author: Guenter Roeck <linux@roeck-us.net>
Subject: hwmon: (aspeed-g6-pwm-tach) Validate the number of tach channels

This commit adds validation for the number of tachometer channels parsed
from the device tree to prevent out-of-bounds array accesses. It also
validates individual channel numbers before enabling hardware features.

Closes: https://sashiko.dev/#/patchset/20260629232305.2013406-1-linux%40roeck-us.net

> diff --git a/drivers/hwmon/aspeed-g6-pwm-tach.c b/drivers/hwmon/aspeed-g6-pwm-tach.c
> --- a/drivers/hwmon/aspeed-g6-pwm-tach.c
> +++ b/drivers/hwmon/aspeed-g6-pwm-tach.c

[ ... ]

> @@ -399,13 +399,16 @@ static const struct hwmon_chip_info aspeed_tach_chip_info = {
>  	.info = aspeed_tach_info,
>  };
>  
> -static void aspeed_present_fan_tach(struct aspeed_pwm_tach_data *priv, u8 *tach_ch, int count)
> +static int aspeed_present_fan_tach(struct aspeed_pwm_tach_data *priv, u8 *tach_ch, int count)
>  {
>  	u8 ch, index;
>  	u32 val;
>  
>  	for (index = 0; index < count; index++) {
>  		ch = tach_ch[index];
> +		if (ch >= TACH_ASPEED_NR_TACHS)
> +			return -EINVAL;
> +

[Severity: Medium]
If the device tree array contains a valid channel followed by an invalid
channel, does returning early here leave the previously processed valid
channels permanently enabled in hardware? 

It looks like the earlier iterations would have already called
aspeed_tach_ch_enable(priv, ch, true) without disabling them on this error
path.

>  		priv->tach_present[ch] = true;
>  		priv->tach_divisor = DEFAULT_TACH_DIV;

[Severity: High]
This is a pre-existing issue, but is priv->tach_divisor a single global
variable shared across all fan channels? 

Looking at aspeed_tach_hwmon_write():

drivers/hwmon/aspeed-g6-pwm-tach.c:aspeed_tach_hwmon_write() {
    ...
    priv->tach_divisor = val;
    reg_val = readl(priv->base + TACH_ASPEED_CTRL(channel));
    ...
}

Does this mean modifying the divisor for one fan channel overwrites the
global state, corrupting RPM calculations in aspeed_tach_val_to_rpm() for
other independent channels?

[ ... ]

[Severity: Medium]
This isn't a bug introduced by this patch, but does the probe function
silently mask failures when fan monitor creation fails?

drivers/hwmon/aspeed-g6-pwm-tach.c:aspeed_pwm_tach_probe() {
    ...
    ret = aspeed_create_fan_monitor(dev, child, priv);
    if (ret) {
        dev_warn(dev, "Failed to create fan %d", ret);
        return 0;
    }
    ...
}

If the fan channel configuration in the device tree is invalid, this returns
0 instead of the error code. Could this leave the driver in a partially
functional state where hardware monitoring is completely missing without
signaling a probe failure to the driver core?

[Severity: High]
This is a pre-existing issue, but can the RPM calculations in
aspeed_tach_val_to_rpm() lead to a division by zero panic?

drivers/hwmon/aspeed-g6-pwm-tach.c:aspeed_tach_val_to_rpm() {
    ...
    tach_div = tach_val * priv->tach_divisor * DEFAULT_FAN_PULSE_PR;
    ...
    do_div(rpm, tach_div);
    ...
}

If the hardware legitimately reports 0 for tach_val, or if the 32-bit
multiplication overflows and wraps to 0, wouldn't the subsequent do_div()
trigger a division by zero?

[Severity: Medium]
This is a pre-existing issue, but can the duty cycle calculations in
aspeed_pwm_apply() overflow a 64-bit integer?

drivers/hwmon/aspeed-g6-pwm-tach.c:aspeed_pwm_apply() {
    ...
    duty_pt = div64_u64(state->duty_cycle * priv->clk_rate,
                        (u64)NSEC_PER_SEC * (div_l + 1) << div_h);
    ...
}

When a very large PWM state is requested, the period is limited to avoid
overflow, but duty_cycle doesn't appear to be capped. Could the
state->duty_cycle * priv->clk_rate evaluation overflow u64, resulting in a
near-zero hardware duty cycle instead of the requested value?

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