Re: [PATCH v2] media: i2c: ov5640: Fix potential integer overflow in sysclk calculation

On 8/17/26 19:11, Daniil Iskhakov wrote:
> The calculation of sysclk uses 32-bit arithmetic because
> sensor->xclk_freq is a 32-bit integer. The intermediate multiplication
> result can overflow 32 bits sysclk variable.
> 
> For example, with pll_prediv fixed at 3 (OV5640_PLL_PREDIV), xclk_freq
> set to its maximum of 54MHz (OV5640_XCLK_MAX) and a pll_mult value above
> 238 (the maximum value for pll_mult is 252, defined as
> OV5640_PLL_MULT_MAX), the result exceeds the 32-bit limit.
> 
> This overflow causes the 1GHz safety check to fail, as the truncated
> value fits within the 1GHz limit. Consequently, the function returns an
> incorrect frequency, leading to misconfiguration of the sensor.
> 
> The expression sysclk / 1000000 > 1000 now also cannot be compiled on
> nios2.
> 
> Cast the result of the naturally 32-bit xclk_freq / pll_prediv division
> to u64 instead. This keeps the pre-divider operation 32-bit while the
> potentially overflowing multiplication is performed with 64-bit
> precision. Once the 1GHz safety check has passed, sysclk is guaranteed
> to fit in an unsigned long even on 32-bit systems. Cast it back before
> dividing by sysdiv to avoid another variable 64-bit division. Avoid
> 64-bit division in test.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: aa2882481cad ("media: ov5640: Adjust the clock based on the expected rate")
> Co-developed-by: Agalakov Daniil <[email protected]>
> Signed-off-by: Agalakov Daniil <[email protected]>
> Signed-off-by: Daniil Iskhakov <[email protected]>
> ---
> v2: The patch is adapted for nios2 thanks to automated tests and a tip from
> David Laight <[email protected]>. The first version cast
> sensor->xclk_freq to u64, making the division by pll_prediv a 64-bit operation.
> Keeping sysclk as u64 also made the final division by sysdiv a 64-bit operation.
> The kernel test robot reported that nios2 GCC emitted unresolved references to
> __udivdi3 and __divdi3. These libgcc helpers are not provided by the kernel,
> causing modpost to fail.
> 
>   drivers/media/i2c/ov5640.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 92d2d6cd4ba4..dbe767afefd5 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -1377,13 +1377,13 @@ static unsigned long ov5640_compute_sys_clk(struct ov5640_dev *sensor,
>   					    u8 pll_prediv, u8 pll_mult,
>   					    u8 sysdiv)
>   {
> -	unsigned long sysclk = sensor->xclk_freq / pll_prediv * pll_mult;
> +	u64 sysclk = (u64)(sensor->xclk_freq / pll_prediv) * pll_mult;
>   
>   	/* PLL1 output cannot exceed 1GHz. */
> -	if (sysclk / 1000000 > 1000)
> +	if (sysclk > 1000000000)
>   		return 0;
>   
> -	return sysclk / sysdiv;
> +	return (unsigned long)sysclk / sysdiv;
>   }
>   
>   static unsigned long ov5640_calc_sys_clk(struct ov5640_dev *sensor,

Reviewed-by: Vladimir Zapolskiy <[email protected]>

-- 
Best wishes,
Vladimir