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

[David Laight <david.laight.linux@gmail.com>]

On Mon, 20 Apr 2026 18:39:50 +0300
Agalakov Daniil <ade@amicon.ru> 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.
> 
> Explicitly cast the first operand to u64 to ensure the entire expression
> is evaluated in 64-bit precision.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: aa2882481cad ("media: ov5640: Adjust the clock based on the expected rate")
> Signed-off-by: Agalakov Daniil <ade@amicon.ru>
> ---
>  drivers/media/i2c/ov5640.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 85ecc23b3587..04e5a683976a 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -1377,7 +1377,7 @@ 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)

It would be much better to rework the test to avoid a 64bit divide
(which will fail to compile on 32bit).
Even the 'sysclk / 1000000 > 1000' test may fail to compile,
the compiler might convert it so the equivalent 'sysclk > 1000999999'
to avoid the division.
I don't know if that is the correct test - it doesn't match the comment.

You could do something like:
	u32 sysclk;
	if ((sensor->xclk_freq >> 8) * pll_mult > (2000000000 >> 8) * pll_prediv)
		/* output would be over 2GHz (or thereabouts) */
		...
	sysclk = sensor->xclk_freq / pll_prediv * pll_mult;
	if (sysclk > 1000000000)
		/* output is over 1GHz */

	David