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

[David Lechner <dlechner@baylibre.com>]

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,
>