Re: [PATCH v2 1/6] iio: pressure: dps310: fix overflow in the pressure compensation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] iio: pressure: dps310: signed integer overflow in compensation edge cases
--

commit 0579c95e71d8f32f605a58fb3d4e9b5b43e70e29
Author: Paulius Zaleckas <paulius.zaleckas@gmail.com>
Subject: iio: pressure: dps310: fix overflow in the pressure compensation

This commit changes the pressure compensation polynomial in
dps310_calculate_pressure() to use Q20 fixed-point arithmetic. It replaces
the previous div64_s64_rem() approach to prevent overflows when evaluating
higher-order polynomial terms.

> diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
> index 85df58ac18090..3bc690cc86090 100644
> --- a/drivers/iio/pressure/dps310.c
> +++ b/drivers/iio/pressure/dps310.c
> @@ -660,71 +660,56 @@ static int dps310_write_raw(struct iio_dev *iio,
>  	}
>  }
>  
> +/* Fraction bits of the fixed point pressure compensation */
> +#define DPS310_Q	20
> +#define DPS310_Q_ONE	(1LL << DPS310_Q)
> +
> +static s64 dps310_mul_q(s64 a, s64 b)
> +{
> +	return (a * b) >> DPS310_Q;
> +}
> +
>  static int dps310_calculate_pressure(struct dps310_data *data, int *val)
>  	__must_hold(&data->lock)
>  {

[ ... ]

> +	/*
> +	 * Section 4.9.1 of the DPS310 spec in fixed point. Dividing p^3 * c30
> +	 * by kP^3 directly overflows s64 at most oversampling rates.
> +	 */
> +	ps = div_s64(p * DPS310_Q_ONE, kp);
> +	ts = div_s64(t * DPS310_Q_ONE, kt);
> +	x2 = dps310_mul_q(ps, ps);
> +	x3 = dps310_mul_q(x2, ps);
> +
> +	pressure = data->c00 * DPS310_Q_ONE;
> +	pressure += div_s64(p * data->c10 * DPS310_Q_ONE, kp);
> +	pressure += div_s64(t * data->c01 * DPS310_Q_ONE, kt);
> +	pressure += dps310_mul_q(data->c11 * ps, ts);

[Severity: Low]
Can this code still overflow s64 under maximum theoretical raw sensor
inputs?

In drivers/iio/pressure/dps310.c:dps310_calculate_pressure(), the
intermediate product data->c11 * ps is evaluated before being passed into
dps310_mul_q(). Once inside dps310_mul_q(), multiplying this product by ts
can exceed s64 bounds when extreme hardware values are returned.

> +	pressure += data->c20 * x2 + data->c30 * x3;
> +	pressure += dps310_mul_q(data->c21 * x2, ts);

[Severity: Low]
Does this nested product vastly exceed the maximum capacity of a signed
64-bit integer when extreme hardware values are returned?

For example, when the sensor returns maximum possible 24-bit raw values
(e.g., from a faulty or maliciously manipulated I2C sensor) combined with
the minimum configuration scale factor (kp/kt of 253952), ps and ts
approach ~33 * 2^20.

The 64-bit product (data->c21 * x2) * ts evaluated inside dps310_mul_q()
exceeds ~1.29 * 10^21, which easily overflows the s64 maximum of
~9.22 * 10^18.

Since the kernel's -fno-strict-overflow compiler flag guarantees
wrap-around rather than undefined behavior, this results in a wrapped
garbage output, defeating the patch's stated primary goal of fixing math
overflows in this calculation.

Could this overflow be prevented without meaningful precision loss by
reordering operations, such as computing data->c21 * dps310_mul_q(x2, ts)
instead of dps310_mul_q(data->c21 * x2, ts)?

This flaw persists and affects the new c31 term introduced later in the
series via commit "iio: pressure: dps310: add Zilltek ZTS8032 support".

> +	pressure >>= DPS310_Q;
>  	if (pressure < 0LL)
>  		return -ERANGE;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013112124.1878296-1-paulius.zaleckas@gmail.com?part=1