iio: inkern: Avoid risky abs() usage in iio_multiply_value()

[PATCH] iio: inkern: Avoid risky abs() usage in iio_multiply_value()

Posted by Romain Gantois 1 day, 2 hours ago

iio_multiply_value() passes integers val and val2 directly to abs(). This
is problematic because if a signed argument to abs is the lowest value for
its type, then the result is undefined due to overflow.

Cast val and val2 to s64 before passing them to abs() to avoid this issue.

Fixes: 0f85406bf830 ("iio: consumers: Fix handling of negative channel scale in iio_convert_raw_to_processed()")
Cc: stable@vger.kernel.org
Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>
---
 drivers/iio/inkern.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index 0df0ab3de2709..59e8c01457f72 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -618,8 +618,8 @@ int iio_multiply_value(int *result, s64 multiplier,
 			denominator = NANO;
 			break;
 		}
-		*result = multiplier * abs(val);
-		*result += div_s64(multiplier * abs(val2), denominator);
+		*result = multiplier * abs((s64)val);
+		*result += div_s64(multiplier * abs((s64)val2), denominator);
 		if (val < 0 || val2 < 0)
 			*result *= -1;
 		return IIO_VAL_INT;

---
base-commit: 6de23f81a5e08be8fbf5e8d7e9febc72a5b5f27f
change-id: 20260331-iio-multiply-abs-usage-2762180a8b0c

Best regards,
-- 
Romain Gantois <romain.gantois@bootlin.com>

Re: [PATCH] iio: inkern: Avoid risky abs() usage in iio_multiply_value()

Posted by Andy Shevchenko 1 day, 1 hour ago

On Tue, Mar 31, 2026 at 10:49:59AM +0200, Romain Gantois wrote:
> iio_multiply_value() passes integers val and val2 directly to abs(). This
> is problematic because if a signed argument to abs is the lowest value for
> its type, then the result is undefined due to overflow.
> 
> Cast val and val2 to s64 before passing them to abs() to avoid this issue.

...

> Fixes: 0f85406bf830 ("iio: consumers: Fix handling of negative channel scale in iio_convert_raw_to_processed()")

Doesn't fix any know issue for now.

...

> -		*result = multiplier * abs(val);
> -		*result += div_s64(multiplier * abs(val2), denominator);
> +		*result = multiplier * abs((s64)val);
> +		*result += div_s64(multiplier * abs((s64)val2), denominator);

Right, but here we get val and val2 from either static values from the driver
(when it is SCALE channel), or when channel has PROCESSED support.
In the latter one it might theoretically be possible to go till the INT_MIN,
but practically I don't know how, except for the broken driver code in the
first place. With that being said, I think it's better to validate somewhere
the multipliers (when it's SCALE or PROCESSED channel). I also noted that
for the _PROCESSED some drivers keep a garbage in val2. That probably needs
to be addressed as well (exempli gratia: bmi270_read_raw() does that).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: inkern: Avoid risky abs() usage in iio_multiply_value()

Posted by David Laight 19 hours ago

On Tue, 31 Mar 2026 12:29:22 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Tue, Mar 31, 2026 at 10:49:59AM +0200, Romain Gantois wrote:
> > iio_multiply_value() passes integers val and val2 directly to abs(). This
> > is problematic because if a signed argument to abs is the lowest value for
> > its type, then the result is undefined due to overflow.
> > 
> > Cast val and val2 to s64 before passing them to abs() to avoid this issue.  
> 
> ...
> 
> > Fixes: 0f85406bf830 ("iio: consumers: Fix handling of negative channel scale in iio_convert_raw_to_processed()")  
> 
> Doesn't fix any know issue for now.
> 
> ...
> 
> > -		*result = multiplier * abs(val);
> > -		*result += div_s64(multiplier * abs(val2), denominator);
> > +		*result = multiplier * abs((s64)val);
> > +		*result += div_s64(multiplier * abs((s64)val2), denominator);  
> 
> Right, but here we get val and val2 from either static values from the driver
> (when it is SCALE channel), or when channel has PROCESSED support.
> In the latter one it might theoretically be possible to go till the INT_MIN,
> but practically I don't know how, except for the broken driver code in the
> first place. With that being said, I think it's better to validate somewhere
> the multipliers (when it's SCALE or PROCESSED channel). I also noted that
> for the _PROCESSED some drivers keep a garbage in val2. That probably needs
> to be addressed as well (exempli gratia: bmi270_read_raw() does that).
> 

I've just looked at the 'work of art' that is abs().
What is wrong with:
#define abs(x) (sizeof(x) == sizeof(long long) ? __abs(long long, x) : \
		__abs(int, x))
#define __abs(type, x) \
	({ type __abs_x = (x); __abs_x < 0 ? -__abs_x : __abs_x;})

It is just as broken for u128.
It will use the correct signedness for char (but it is unsigned now).
It doesn't cast back to char, but that is entirely pointless unless code
looks at the type of the expression, the return value itself is always
promoted to int before being used.

Actually replace the -__abs_x (UB for INT_MIN) with the safe:
	(unsigned type)-(__abs_x + 1) + 1
and the return type will be unsigned with a correct value for -INT_MIN.
(Oh and the compiler sees through the mess.)

	David

Re: [PATCH] iio: inkern: Avoid risky abs() usage in iio_multiply_value()

Posted by Andy Shevchenko 16 hours ago

On Tue, Mar 31, 2026 at 04:26:35PM +0100, David Laight wrote:
> On Tue, 31 Mar 2026 12:29:22 +0300
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > On Tue, Mar 31, 2026 at 10:49:59AM +0200, Romain Gantois wrote:

> > > iio_multiply_value() passes integers val and val2 directly to abs(). This
> > > is problematic because if a signed argument to abs is the lowest value for
> > > its type, then the result is undefined due to overflow.
> > > 
> > > Cast val and val2 to s64 before passing them to abs() to avoid this issue.  

...

> I've just looked at the 'work of art' that is abs().
> What is wrong with:
> #define abs(x) (sizeof(x) == sizeof(long long) ? __abs(long long, x) : \
> 		__abs(int, x))
> #define __abs(type, x) \
> 	({ type __abs_x = (x); __abs_x < 0 ? -__abs_x : __abs_x;})
> 
> It is just as broken for u128.
> It will use the correct signedness for char (but it is unsigned now).
> It doesn't cast back to char, but that is entirely pointless unless code
> looks at the type of the expression, the return value itself is always
> promoted to int before being used.
> 
> Actually replace the -__abs_x (UB for INT_MIN) with the safe:
> 	(unsigned type)-(__abs_x + 1) + 1
> and the return type will be unsigned with a correct value for -INT_MIN.
> (Oh and the compiler sees through the mess.)

And this is definitely wrong. We must keep type, because abs() might be used in
the comparisons with signed or as parameter to multiplication or division where
sign has to be preserved.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: inkern: Avoid risky abs() usage in iio_multiply_value()

Posted by David Laight 13 hours ago

On Tue, 31 Mar 2026 21:34:06 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Tue, Mar 31, 2026 at 04:26:35PM +0100, David Laight wrote:
> > On Tue, 31 Mar 2026 12:29:22 +0300
> > Andy Shevchenko <andriy.shevchenko@intel.com> wrote:  
> > > On Tue, Mar 31, 2026 at 10:49:59AM +0200, Romain Gantois wrote:  
> 
> > > > iio_multiply_value() passes integers val and val2 directly to abs(). This
> > > > is problematic because if a signed argument to abs is the lowest value for
> > > > its type, then the result is undefined due to overflow.
> > > > 
> > > > Cast val and val2 to s64 before passing them to abs() to avoid this issue.    
> 
> ...
> 
> > I've just looked at the 'work of art' that is abs().
> > What is wrong with:
> > #define abs(x) (sizeof(x) == sizeof(long long) ? __abs(long long, x) : \
> > 		__abs(int, x))
> > #define __abs(type, x) \
> > 	({ type __abs_x = (x); __abs_x < 0 ? -__abs_x : __abs_x;})
> > 
> > It is just as broken for u128.
> > It will use the correct signedness for char (but it is unsigned now).
> > It doesn't cast back to char, but that is entirely pointless unless code
> > looks at the type of the expression, the return value itself is always
> > promoted to int before being used.
> > 
> > Actually replace the -__abs_x (UB for INT_MIN) with the safe:
> > 	(unsigned type)-(__abs_x + 1) + 1
> > and the return type will be unsigned with a correct value for -INT_MIN.
> > (Oh and the compiler sees through the mess.)  
> 
> And this is definitely wrong. We must keep type, because abs() might be used in
> the comparisons with signed or as parameter to multiplication or division where
> sign has to be preserved.

Thinks.... (bad at 11pm)
IIRC -INT_MIN is UB, but (~INT_MIN + 1) is fine provided -fno-strict-overflow
is set - which it is for kernel builds.
At least that guarantees the abs(-INT_MIN) == INT_MIN which is about the best
you can do.
It isn't as if it is ever going to happen.
There are all sorts of ways to break things in a driver.

	David

Re: [PATCH] iio: inkern: Avoid risky abs() usage in iio_multiply_value()

Posted by Andy Shevchenko 1 day, 1 hour ago

On Tue, Mar 31, 2026 at 12:29:22PM +0300, Andy Shevchenko wrote:
> On Tue, Mar 31, 2026 at 10:49:59AM +0200, Romain Gantois wrote:

...

> > -		*result = multiplier * abs(val);
> > -		*result += div_s64(multiplier * abs(val2), denominator);
> > +		*result = multiplier * abs((s64)val);
> > +		*result += div_s64(multiplier * abs((s64)val2), denominator);
> 
> Right, but here we get val and val2 from either static values from the driver
> (when it is SCALE channel), or when channel has PROCESSED support.
> In the latter one it might theoretically be possible to go till the INT_MIN,
> but practically I don't know how, except for the broken driver code in the
> first place. With that being said, I think it's better to validate somewhere
> the multipliers (when it's SCALE or PROCESSED channel). I also noted that
> for the _PROCESSED some drivers keep a garbage in val2. That probably needs
> to be addressed as well (exempli gratia: bmi270_read_raw() does that).

Actually the data in the val and val2 should be aligned with the returned type,
hence the potential bugs might only come from the untested drivers. Which means
that this patch doesn't improve the situation.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: inkern: Avoid risky abs() usage in iio_multiply_value()

Posted by Romain Gantois 22 hours ago

Hello Andy,

On Tuesday, 31 March 2026 12:08:26 CEST Andy Shevchenko wrote:
> On Tue, Mar 31, 2026 at 12:29:22PM +0300, Andy Shevchenko wrote:
> > On Tue, Mar 31, 2026 at 10:49:59AM +0200, Romain Gantois wrote:
> ...
> 
> > > -		*result = multiplier * abs(val);
> > > -		*result += div_s64(multiplier * abs(val2), denominator);
> > > +		*result = multiplier * abs((s64)val);
> > > +		*result += div_s64(multiplier * abs((s64)val2), denominator);
> > 
> > Right, but here we get val and val2 from either static values from the
> > driver (when it is SCALE channel), or when channel has PROCESSED support.
> > In the latter one it might theoretically be possible to go till the
> > INT_MIN, but practically I don't know how, except for the broken driver
> > code in the first place. With that being said, I think it's better to
> > validate somewhere the multipliers (when it's SCALE or PROCESSED
> > channel). I also noted that for the _PROCESSED some drivers keep a
> > garbage in val2. That probably needs to be addressed as well (exempli
> > gratia: bmi270_read_raw() does that).
> 
> Actually the data in the val and val2 should be aligned with the returned
> type, hence the potential bugs might only come from the untested drivers.
> Which means that this patch doesn't improve the situation.

I'm a bit confused: when you say "the returned type" what returning function 
are you referring to? Also, doesn't the patch still fix the bug for potentially 
untested drivers which use PROCESSED?

Thanks,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] iio: inkern: Avoid risky abs() usage in iio_multiply_value()

Posted by Andy Shevchenko 16 hours ago

On Tue, Mar 31, 2026 at 02:13:29PM +0200, Romain Gantois wrote:
> On Tuesday, 31 March 2026 12:08:26 CEST Andy Shevchenko wrote:
> > On Tue, Mar 31, 2026 at 12:29:22PM +0300, Andy Shevchenko wrote:
> > > On Tue, Mar 31, 2026 at 10:49:59AM +0200, Romain Gantois wrote:

...

> > > > -		*result = multiplier * abs(val);
> > > > -		*result += div_s64(multiplier * abs(val2), denominator);
> > > > +		*result = multiplier * abs((s64)val);
> > > > +		*result += div_s64(multiplier * abs((s64)val2), denominator);
> > > 
> > > Right, but here we get val and val2 from either static values from the
> > > driver (when it is SCALE channel), or when channel has PROCESSED support.
> > > In the latter one it might theoretically be possible to go till the
> > > INT_MIN, but practically I don't know how, except for the broken driver
> > > code in the first place. With that being said, I think it's better to
> > > validate somewhere the multipliers (when it's SCALE or PROCESSED
> > > channel). I also noted that for the _PROCESSED some drivers keep a
> > > garbage in val2. That probably needs to be addressed as well (exempli
> > > gratia: bmi270_read_raw() does that).
> > 
> > Actually the data in the val and val2 should be aligned with the returned
> > type, hence the potential bugs might only come from the untested drivers.
> > Which means that this patch doesn't improve the situation.
> 
> I'm a bit confused: when you say "the returned type" what returning function 
> are you referring to?

_read_channel(). It returns the type of the value in IIO namespace.

> Also, doesn't the patch still fix the bug for potentially 
> untested drivers which use PROCESSED?

No it doesn't. It just makes it different, but not necessary working.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: inkern: Avoid risky abs() usage in iio_multiply_value()

Posted by Andy Shevchenko 1 day, 1 hour ago

On Tue, Mar 31, 2026 at 12:29:28PM +0300, Andy Shevchenko wrote:
> On Tue, Mar 31, 2026 at 10:49:59AM +0200, Romain Gantois wrote:
> > iio_multiply_value() passes integers val and val2 directly to abs(). This
> > is problematic because if a signed argument to abs is the lowest value for
> > its type, then the result is undefined due to overflow.
> > 
> > Cast val and val2 to s64 before passing them to abs() to avoid this issue.

...

> > -		*result = multiplier * abs(val);
> > -		*result += div_s64(multiplier * abs(val2), denominator);
> > +		*result = multiplier * abs((s64)val);
> > +		*result += div_s64(multiplier * abs((s64)val2), denominator);
> 
> Right, but here we get val and val2 from either static values from the driver
> (when it is SCALE channel), or when channel has PROCESSED support.
> In the latter one it might theoretically be possible to go till the INT_MIN,
> but practically I don't know how, except for the broken driver code in the
> first place. With that being said, I think it's better to validate somewhere
> the multipliers (when it's SCALE or PROCESSED channel). I also noted that
> for the _PROCESSED some drivers keep a garbage in val2. That probably needs
> to be addressed as well (exempli gratia: bmi270_read_raw() does that).

And start from the test cases actually.

-- 
With Best Regards,
Andy Shevchenko