[PATCH] iio: imu: inv_icm42600: fix 64bit multiplication

[PATCH] iio: imu: inv_icm42600: fix 64bit multiplication

[Antonio Quartulli]

In inv_icm42600_accel_convert_wom_to_roc() multiplying
`threshold` by `convert` may result in a number requiring more
than 32bit.
In this case, although `value` is 64bit wide, the result is
truncated because the multiplication is performed in the
32bit domain, due to both operands being 32bit long.

Cast the first operand to u64 to ensure the multiplication is
performed in the expected domain.

Fixes: 50cfaa9a46c8 ("iio: imu: inv_icm42600: add WoM support")
Address-Coverity-ID: 1647596 ("Integer handling issues (OVERFLOW_BEFORE_WIDEN)")
Signed-off-by: Antonio Quartulli <antonio@mandelbit.com>
---
 drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
index 7a28051330b7..218bb3eb3dd7 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
@@ -347,7 +347,7 @@ static u64 inv_icm42600_accel_convert_wom_to_roc(unsigned int threshold,
 	u64 value;
 	u64 freq_uhz;
 
-	value = threshold * convert;
+	value = (u64)threshold * convert;
 	freq_uhz = (u64)accel_hz * MICRO + (u64)accel_uhz;
 
 	/* compute the differential by multiplying by the frequency */
-- 
2.49.1

Re: [PATCH] iio: imu: inv_icm42600: fix 64bit multiplication

[Andy Shevchenko]

On Wed, Jul 16, 2025 at 10:22 PM Antonio Quartulli
<antonio@mandelbit.com> wrote:
>
> In inv_icm42600_accel_convert_wom_to_roc() multiplying
> `threshold` by `convert` may result in a number requiring more
> than 32bit.
> In this case, although `value` is 64bit wide, the result is
> truncated because the multiplication is performed in the
> 32bit domain, due to both operands being 32bit long.
>
> Cast the first operand to u64 to ensure the multiplication is
> performed in the expected domain.

Is this a theoretical or practical issue?

> Fixes: 50cfaa9a46c8 ("iio: imu: inv_icm42600: add WoM support")

> Address-Coverity-ID: 1647596 ("Integer handling issues (OVERFLOW_BEFORE_WIDEN)")

Is this tag now official? I can't find where it's documented.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: imu: inv_icm42600: fix 64bit multiplication

[Andy Shevchenko]

On Wed, Jul 16, 2025 at 10:22 PM Antonio Quartulli
<antonio@mandelbit.com> wrote:
>
> In inv_icm42600_accel_convert_wom_to_roc() multiplying
> `threshold` by `convert` may result in a number requiring more
> than 32bit.

32 bits

> In this case, although `value` is 64bit wide, the result is

64-bit

> truncated because the multiplication is performed in the
> 32bit domain, due to both operands being 32bit long.

32-bit
32-bit

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: imu: inv_icm42600: fix 64bit multiplication

[Antonio Quartulli]

On 16/07/2025 21:36, Andy Shevchenko wrote:
> On Wed, Jul 16, 2025 at 10:22 PM Antonio Quartulli
> <antonio@mandelbit.com> wrote:
>>
>> In inv_icm42600_accel_convert_wom_to_roc() multiplying
>> `threshold` by `convert` may result in a number requiring more
>> than 32bit.
>> In this case, although `value` is 64bit wide, the result is
>> truncated because the multiplication is performed in the
>> 32bit domain, due to both operands being 32bit long.
>>
>> Cast the first operand to u64 to ensure the multiplication is
>> performed in the expected domain.
> 
> Is this a theoretical or practical issue?

Can't say if it's practical because I don't know how large `threshold` 
can be.

This said, `value` is declared as u64, therefore I assumed the result is 
expected to be potentially larger than 32 bits.

> 
>> Fixes: 50cfaa9a46c8 ("iio: imu: inv_icm42600: add WoM support")
> 
>> Address-Coverity-ID: 1647596 ("Integer handling issues (OVERFLOW_BEFORE_WIDEN)")
> 
> Is this tag now official? I can't find where it's documented.

I don't think it's official, but it's what I have found in the git 
history for other bugs reported by Coverity.
I already used it on other accepted patches as well.

Regards,

-- 
Antonio Quartulli

CEO and Co-Founder
Mandelbit Srl
https://www.mandelbit.com

Re: [PATCH] iio: imu: inv_icm42600: fix 64bit multiplication

[Antonio Quartulli]

On 16/07/2025 21:38, Andy Shevchenko wrote:
> On Wed, Jul 16, 2025 at 10:22 PM Antonio Quartulli
> <antonio@mandelbit.com> wrote:
>>
>> In inv_icm42600_accel_convert_wom_to_roc() multiplying
>> `threshold` by `convert` may result in a number requiring more
>> than 32bit.
> 
> 32 bits
> 
>> In this case, although `value` is 64bit wide, the result is
> 
> 64-bit
> 
>> truncated because the multiplication is performed in the
>> 32bit domain, due to both operands being 32bit long.
> 
> 32-bit
> 32-bit

Thanks for highlighting those.
Should I send v2?

Regards,

-- 
Antonio Quartulli

CEO and Co-Founder
Mandelbit Srl
https://www.mandelbit.com

Re: [PATCH] iio: imu: inv_icm42600: fix 64bit multiplication

[David Lechner]

On 7/16/25 2:44 PM, Antonio Quartulli wrote:
> On 16/07/2025 21:36, Andy Shevchenko wrote:
>> On Wed, Jul 16, 2025 at 10:22 PM Antonio Quartulli
>> <antonio@mandelbit.com> wrote:
>>>
>>> In inv_icm42600_accel_convert_wom_to_roc() multiplying
>>> `threshold` by `convert` may result in a number requiring more
>>> than 32bit.
>>> In this case, although `value` is 64bit wide, the result is
>>> truncated because the multiplication is performed in the
>>> 32bit domain, due to both operands being 32bit long.
>>>
>>> Cast the first operand to u64 to ensure the multiplication is
>>> performed in the expected domain.
>>
>> Is this a theoretical or practical issue?
> 
> Can't say if it's practical because I don't know how large `threshold` can be.


The code is clear that it is between 0 and 255 inclusive.

unsigned int inv_icm42600_accel_convert_roc_to_wom(...)
{
	...
	if (roc == 0)
		return 0;
	...
	return clamp(value, 1, 255);
}

threshold = inv_icm42600_accel_convert_roc_to_wom(value, accel_hz, accel_uhz);
value = inv_icm42600_accel_convert_wom_to_roc(threshold, accel_hz, accel_uhz)

So I would not call this a fix, just making the code more robust against
any changes in the future.

> 
> This said, `value` is declared as u64, therefore I assumed the result is expected to be potentially larger than 32 bits.
> 
>>
>>> Fixes: 50cfaa9a46c8 ("iio: imu: inv_icm42600: add WoM support")
>>
>>> Address-Coverity-ID: 1647596 ("Integer handling issues (OVERFLOW_BEFORE_WIDEN)")
>>
>> Is this tag now official? I can't find where it's documented.
> 
> I don't think it's official, but it's what I have found in the git history for other bugs reported by Coverity.
> I already used it on other accepted patches as well.
> 
> Regards,
> 

Re: [PATCH] iio: imu: inv_icm42600: fix 64bit multiplication

[Antonio Quartulli]

On 16/07/2025 21:52, David Lechner wrote:
> On 7/16/25 2:44 PM, Antonio Quartulli wrote:
>> On 16/07/2025 21:36, Andy Shevchenko wrote:
>>> On Wed, Jul 16, 2025 at 10:22 PM Antonio Quartulli
>>> <antonio@mandelbit.com> wrote:
>>>>
>>>> In inv_icm42600_accel_convert_wom_to_roc() multiplying
>>>> `threshold` by `convert` may result in a number requiring more
>>>> than 32bit.
>>>> In this case, although `value` is 64bit wide, the result is
>>>> truncated because the multiplication is performed in the
>>>> 32bit domain, due to both operands being 32bit long.
>>>>
>>>> Cast the first operand to u64 to ensure the multiplication is
>>>> performed in the expected domain.
>>>
>>> Is this a theoretical or practical issue?
>>
>> Can't say if it's practical because I don't know how large `threshold` can be.
> 
> 
> The code is clear that it is between 0 and 255 inclusive.
> 
> unsigned int inv_icm42600_accel_convert_roc_to_wom(...)
> {
> 	...
> 	if (roc == 0)
> 		return 0;
> 	...
> 	return clamp(value, 1, 255);
> }
> 
> threshold = inv_icm42600_accel_convert_roc_to_wom(value, accel_hz, accel_uhz);
> value = inv_icm42600_accel_convert_wom_to_roc(threshold, accel_hz, accel_uhz)

Thanks for pointing this out - I overlooked that.

> 
> So I would not call this a fix, just making the code more robust against
> any changes in the future.

It's the type being used that convinced me something could be off.
Maybe threshold could just be u8 and value be u32? (just to improve self 
documentation)
Also inv_icm42600_accel_convert_roc_to_wom() could return u8 for 
consistency.

I any case, this is a bit code cleanup and a bit bike shedding :-)
I agree there is nothing to "fix" at this point.

Regards,


-- 
Antonio Quartulli

CEO and Co-Founder
Mandelbit Srl
https://www.mandelbit.com

Re: [PATCH] iio: imu: inv_icm42600: fix 64bit multiplication

[Andy Shevchenko]

On Wed, Jul 16, 2025 at 10:44 PM Antonio Quartulli
<antonio@mandelbit.com> wrote:
> On 16/07/2025 21:36, Andy Shevchenko wrote:
> > On Wed, Jul 16, 2025 at 10:22 PM Antonio Quartulli
> > <antonio@mandelbit.com> wrote:
> >>
> >> In inv_icm42600_accel_convert_wom_to_roc() multiplying
> >> `threshold` by `convert` may result in a number requiring more
> >> than 32bit.
> >> In this case, although `value` is 64bit wide, the result is
> >> truncated because the multiplication is performed in the
> >> 32bit domain, due to both operands being 32bit long.
> >>
> >> Cast the first operand to u64 to ensure the multiplication is
> >> performed in the expected domain.
> >
> > Is this a theoretical or practical issue?
>
> Can't say if it's practical because I don't know how large `threshold`
> can be.
>
> This said, `value` is declared as u64, therefore I assumed the result is
> expected to be potentially larger than 32 bits.

Yeah, but can you prove the potential overflow? Can the compiler prove
otherwise?

> >> Address-Coverity-ID: 1647596 ("Integer handling issues (OVERFLOW_BEFORE_WIDEN)")
> >
> > Is this tag now official? I can't find where it's documented.
>
> I don't think it's official, but it's what I have found in the git
> history for other bugs reported by Coverity.
> I already used it on other accepted patches as well.

Are you an official representative from the coverity that runs against
Linux source code? Or please explain how it keeps those numbers (e.g.,
1647596) unique per project. For example, you and I run coverity and
we get let's say the very same number (ID) but for different problems
(even if we are looking at the same project), is this scenario real?
If not, we need to have documentation inside the Linux kernel source
tree explaining how to run Coverity, who is eligible to do that, how
to resolve clashes (if any), etc., etc...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: imu: inv_icm42600: fix 64bit multiplication

[Antonio Quartulli]

On 17/07/2025 08:21, Andy Shevchenko wrote:
> On Wed, Jul 16, 2025 at 10:44 PM Antonio Quartulli
> <antonio@mandelbit.com> wrote:
>> On 16/07/2025 21:36, Andy Shevchenko wrote:
>>> On Wed, Jul 16, 2025 at 10:22 PM Antonio Quartulli
>>> <antonio@mandelbit.com> wrote:
>>>>
>>>> In inv_icm42600_accel_convert_wom_to_roc() multiplying
>>>> `threshold` by `convert` may result in a number requiring more
>>>> than 32bit.
>>>> In this case, although `value` is 64bit wide, the result is
>>>> truncated because the multiplication is performed in the
>>>> 32bit domain, due to both operands being 32bit long.
>>>>
>>>> Cast the first operand to u64 to ensure the multiplication is
>>>> performed in the expected domain.
>>>
>>> Is this a theoretical or practical issue?
>>
>> Can't say if it's practical because I don't know how large `threshold`
>> can be.
>>
>> This said, `value` is declared as u64, therefore I assumed the result is
>> expected to be potentially larger than 32 bits.
> 
> Yeah, but can you prove the potential overflow? Can the compiler prove
> otherwise?

As David pointed out, threshold is limited to [1, 255], as returned by 
inv_icm42600_accel_convert_roc_to_wom(), therefore no real overflow is 
possible.

It's the variable types being used that didn't make that fully clear.
Maybe this could be cleaned up a little, but there is nothing to "fix".

> 
>>>> Address-Coverity-ID: 1647596 ("Integer handling issues (OVERFLOW_BEFORE_WIDEN)")
>>>
>>> Is this tag now official? I can't find where it's documented.
>>
>> I don't think it's official, but it's what I have found in the git
>> history for other bugs reported by Coverity.
>> I already used it on other accepted patches as well.
> 
> Are you an official representative from the coverity that runs against
> Linux source code? Or please explain how it keeps those numbers (e.g.,
> 1647596) unique per project. For example, you and I run coverity and
> we get let's say the very same number (ID) but for different problems
> (even if we are looking at the same project), is this scenario real?
> If not, we need to have documentation inside the Linux kernel source
> tree explaining how to run Coverity, who is eligible to do that, how
> to resolve clashes (if any), etc., etc...

There is an ("official") linux-next project in Coverity managed by 
Gustavo A. R. Silva, where all maintainers can subscribe to and then 
receive the weekly scan results:

https://scan.coverity.com/projects/linux-next-weekly-scan
https://embeddedor.com/blog/2024/09/28/one-simple-and-rewarding-way-to-contribute-to-the-linux-kernel-fix-coverity-issues/

I assumed all IDs in git are taken from that scan/project.

Regards,

-- 
Antonio Quartulli

CEO and Co-Founder
Mandelbit Srl
https://www.mandelbit.com

Re: [PATCH] iio: imu: inv_icm42600: fix 64bit multiplication

[Andy Shevchenko]

On Thu, Jul 17, 2025 at 10:50 AM Antonio Quartulli
<antonio@mandelbit.com> wrote:
> On 17/07/2025 08:21, Andy Shevchenko wrote:
> > On Wed, Jul 16, 2025 at 10:44 PM Antonio Quartulli
> > <antonio@mandelbit.com> wrote:
> >> On 16/07/2025 21:36, Andy Shevchenko wrote:
> >>> On Wed, Jul 16, 2025 at 10:22 PM Antonio Quartulli
> >>> <antonio@mandelbit.com> wrote:

...

> >>>> Address-Coverity-ID: 1647596 ("Integer handling issues (OVERFLOW_BEFORE_WIDEN)")
> >>>
> >>> Is this tag now official? I can't find where it's documented.
> >>
> >> I don't think it's official, but it's what I have found in the git
> >> history for other bugs reported by Coverity.
> >> I already used it on other accepted patches as well.
> >
> > Are you an official representative from the coverity that runs against
> > Linux source code? Or please explain how it keeps those numbers (e.g.,
> > 1647596) unique per project. For example, you and I run coverity and
> > we get let's say the very same number (ID) but for different problems
> > (even if we are looking at the same project), is this scenario real?
> > If not, we need to have documentation inside the Linux kernel source
> > tree explaining how to run Coverity, who is eligible to do that, how
> > to resolve clashes (if any), etc., etc...
>
> There is an ("official") linux-next project in Coverity managed by
> Gustavo A. R. Silva, where all maintainers can subscribe to and then
> receive the weekly scan results:
>
> https://scan.coverity.com/projects/linux-next-weekly-scan
> https://embeddedor.com/blog/2024/09/28/one-simple-and-rewarding-way-to-contribute-to-the-linux-kernel-fix-coverity-issues/
>
> I assumed all IDs in git are taken from that scan/project.

This makes a lot of sense! Gustavo et al., can you make a
Documentation patch to explain all this?
Personally from now on I am okay with the tag as long as the patches
tagged with it are at least Acked by Gustavo.

-- 
With Best Regards,
Andy Shevchenko