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

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

[Daniil Iskhakov]

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 <ade@amicon.ru>
Signed-off-by: Agalakov Daniil <ade@amicon.ru>
Signed-off-by: Daniil Iskhakov <dish@amicon.ru>
---
v2: The patch is adapted for nios2 thanks to automated tests and a tip from
David Laight <david.laight.linux@gmail.com>. 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,
-- 
2.53.0

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

[Vladimir Zapolskiy]

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 <ade@amicon.ru>
> Signed-off-by: Agalakov Daniil <ade@amicon.ru>
> Signed-off-by: Daniil Iskhakov <dish@amicon.ru>
> ---
> v2: The patch is adapted for nios2 thanks to automated tests and a tip from
> David Laight <david.laight.linux@gmail.com>. 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 <vz@kernel.org>

-- 
Best wishes,
Vladimir