Re: [RFC PATCH 12/14] hwmon/macsmc: add more types

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] hwmon: 32-bit integer overflow when scaling values in macsmc_hwmon_read_key()
--

commit 8d01f24623ebf973c1f3f366376ae2ac53217c82
Author: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>

hwmon/macsmc: add more types

This commit adds support for reading common integer (ui8, si8, ui16, si16,
ui32, si32) and fixed-point (fp* and sp*) SMC key data types in
macsmc_hwmon_read_key(), and allows writing fpe2 keys for fan speed control
in macsmc_hwmon_write_key().

> diff --git a/drivers/hwmon/macsmc-hwmon.c b/drivers/hwmon/macsmc-hwmon.c
> index 1500ec2cc9f83..2e74dcd6651b3 100644
> --- a/drivers/hwmon/macsmc-hwmon.c
> +++ b/drivers/hwmon/macsmc-hwmon.c
> @@ -221,6 +221,313 @@ static int macsmc_hwmon_read_key(struct apple_smc *smc,
>  			*val = (long)ioft;
>  		break;
>  	}

[ ... ]

> +	/* unsigned 16-bit integer */
> +	case __SMC_KEY('u', 'i', '1', '6'): {
> +		u16 ui16 = 0;
> +
> +		ret = apple_smc_read_u16(smc, sensor->macsmc_key, &ui16);
> +		if (ret)
> +			return ret;
> +
> +		*val = ui16 * scale;
> +		break;
> +	}
> +	/* signed 16-bit integer */
> +	case __SMC_KEY('s', 'i', '1', '6'): {
> +		s16 si16 = 0;
> +
> +		ret = apple_smc_read_s16(smc, sensor->macsmc_key, &si16);
> +		if (ret)
> +			return ret;
> +
> +		*val = si16 * scale;
> +		break;
> +	}
> +	/* unsigned 32-bit integer */
> +	case __SMC_KEY('u', 'i', '3', '2'): {
> +		u32 ui32 = 0;
> +
> +		ret = apple_smc_read_u32(smc, sensor->macsmc_key, &ui32);
> +		if (ret)
> +			return ret;
> +
> +		*val = ui32 * scale;
> +		break;
> +	}
> +	/* signed 32-bit integer */
> +	case __SMC_KEY('s', 'i', '3', '2'): {
> +		s32 si32;
> +
> +		ret = apple_smc_read_s32(smc, sensor->macsmc_key, &si32);
> +		if (ret)
> +			return ret;
> +
> +		*val = si32 * scale;
> +		break;
> +	}
> +	/* 2.14 unsigned fixed point decimal */
> +	case __SMC_KEY('f', 'p', '2', 'e'): {
> +		u16 fp2e;
> +
> +		ret = apple_smc_read_u16(smc, sensor->macsmc_key, &fp2e);
> +		if (ret)
> +			return ret;
> +
> +		*val = mult_frac(be16_to_cpu(fp2e), scale, BIT(14));
> +		break;
> +	}

[ ... ]

> +	/* 1.14 signed fixed point decimal */
> +	case __SMC_KEY('s', 'p', '1', 'e'): {
> +		s16 sp1e;
> +
> +		ret = apple_smc_read_s16(smc, sensor->macsmc_key, &sp1e);
> +		if (ret)
> +			return ret;
> +
> +		*val = mult_frac((s16)be16_to_cpu(sp1e), scale, BIT(14));
> +		break;
> +	}

[ ... ]

> +	/* 15.0 signed fixed point decimal */
> +	case __SMC_KEY('s', 'p', 'f', '0'): {
> +		s16 spf0;
> +
> +		ret = apple_smc_read_s16(smc, sensor->macsmc_key, &spf0);
> +		if (ret)
> +			return ret;
> +
> +		*val = (s16)be16_to_cpu(spf0) * scale;
> +		break;
> +	}

[Severity: Medium]
Can these scaling calculations in macsmc_hwmon_read_key() overflow 32-bit
integers and corrupt sensor readings when called from macsmc_hwmon_read()?

In macsmc_hwmon_read(), scale is 1000000 for hwmon_power and 1000 for
hwmon_temp, hwmon_in, and hwmon_curr, while the scale parameter in
macsmc_hwmon_read_key() has type int:

drivers/hwmon/macsmc-hwmon.c:macsmc_hwmon_read() {
	...
	case hwmon_power:
		/* SMC returns power in Watts with acceptable precision to scale to uW */
		ret = macsmc_hwmon_read_key(hwmon->smc,
					    &hwmon->power.sensors[channel],
					    1000000, val);
		break;
}

First, in the mult_frac() calls for the unsigned (fp2e through fp88) and
signed (sp1e through spd2) fixed-point types, the first argument has a 16-bit
type (u16 from be16_to_cpu() or s16 from (s16)be16_to_cpu()):

include/linux/math.h:
#define mult_frac(x, n, d)	\
({				\
	typeof(x) x_ = (x);	\
	typeof(n) n_ = (n);	\
	typeof(d) d_ = (d);	\
				\
	typeof(x_) q = x_ / d_;	\
	typeof(x_) r = x_ % d_;	\
	q * n_ + r * n_ / d_;	\
})

Because typeof(x_) is a 16-bit integer, q and r are promoted to 32-bit signed
int, and n_ (scale) is also a 32-bit signed int. Both q * n_ and r * n_ are
evaluated as 32-bit signed int multiplications before dividing by d_
(unsigned long from BIT(n)). When scale is 1000000, any remainder r >= 2148
(possible for fp2e, fp3d, fp4c, sp1e, sp2d, and sp3c) or quotient |q| >= 2148
(possible for spc3 and spd2) overflows 32-bit signed int into a negative
value, which then converts to a huge unsigned long when divided by BIT(n).

Second, for the signed sp* types (sp1e through spd2), passing a signed x_ with
an unsigned long divisor BIT(n) causes x_ / d_ and x_ % d_ inside mult_frac()
to be evaluated as unsigned division and modulo whenever x_ is negative,
corrupting negative readings.

Third, for ui16, si16, ui32, si32, and spf0, the multiplications by scale
(such as ui16 * scale, si32 * scale, and (s16)be16_to_cpu(spf0) * scale) are
all evaluated in 32-bit arithmetic before widening to long *val, which
overflows when scale is 1000000 (for values >= 2148 on signed/16-bit types or
>= 4295 on ui32) or 1000 (for 32-bit types).

Would it make sense to widen scale (or cast the operands and divisors to
signed long / s64) so that the multiplications and mult_frac() divisions are
performed in 64-bit signed arithmetic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029085504.3547-1-atharvatiwarilinuxdev@gmail.com?part=12