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

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

[Guenter Roeck]

Sashiko reports:

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?

Fix the problem by validating the number of tachometer channels reported
from of_property_count_u8_elems() and return an error if the number of
tachometer channels exceeds the number of supported channels. Also, in
aspeed_present_fan_tach(), validate the actual channel number in
aspeed_present_fan_tach() and return an error if it exceeds the number
of channels supported by the driver.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260629232305.2013406-1-linux%40roeck-us.net
Fixes: 7e1449cd15d10 ("hwmon: (aspeed-g6-pwm-tacho): Support for ASPEED g6 PWM/Fan tach")
Cc: Billy Tsai <billy_tsai@aspeedtech.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/aspeed-g6-pwm-tach.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/aspeed-g6-pwm-tach.c b/drivers/hwmon/aspeed-g6-pwm-tach.c
index 5d611a8e5269..2cb3f2e1aeb2 100644
--- a/drivers/hwmon/aspeed-g6-pwm-tach.c
+++ b/drivers/hwmon/aspeed-g6-pwm-tach.c
@@ -402,13 +402,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;
+
 		priv->tach_present[ch] = true;
 		priv->tach_divisor = DEFAULT_TACH_DIV;
 
@@ -424,6 +427,7 @@ static void aspeed_present_fan_tach(struct aspeed_pwm_tach_data *priv, u8 *tach_
 
 		aspeed_tach_ch_enable(priv, ch, true);
 	}
+	return 0;
 }
 
 static int aspeed_create_fan_monitor(struct device *dev,
@@ -434,7 +438,7 @@ static int aspeed_create_fan_monitor(struct device *dev,
 	u8 *tach_ch;
 
 	count = of_property_count_u8_elems(child, "tach-ch");
-	if (count < 1)
+	if (count < 1 || count > TACH_ASPEED_NR_TACHS)
 		return -EINVAL;
 	tach_ch = devm_kcalloc(dev, count, sizeof(*tach_ch), GFP_KERNEL);
 	if (!tach_ch)
@@ -443,9 +447,7 @@ static int aspeed_create_fan_monitor(struct device *dev,
 	if (ret)
 		return ret;
 
-	aspeed_present_fan_tach(priv, tach_ch, count);
-
-	return 0;
+	return aspeed_present_fan_tach(priv, tach_ch, count);
 }
 
 static void aspeed_pwm_tach_reset_assert(void *data)
-- 
2.45.2

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

[sashiko-bot]

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