[PATCH] clk: sophgo: Cast an operand to u64 to prevent potential unsigned long overflow on 32-bit machine in sg2042_pll_recalc_rate()

[PATCH] clk: sophgo: Cast an operand to u64 to prevent potential unsigned long overflow on 32-bit machine in sg2042_pll_recalc_rate()

[Gax-c]

From: Zichen Xie <zichenxie0106@gmail.com>

This was found by a static analyzer.
There may be a potential integer overflow issue in
sg2042_pll_recalc_rate(). numerator is defined as u64 while
parent_rate is defined as unsigned long and ctrl_table.fbdiv
is defined as unsigned int. On 32-bit machine, the result of
the calculation will be limited to "u32" without correct casting.
Integer overflow may occur on high-performance systems.
For the same reason, adding a cast to denominator could be better.
So, we recommend adding an extra cast to prevent potential
integer overflow.

Fixes: 48cf7e01386e ("clk: sophgo: Add SG2042 clock driver")
Signed-off-by: Zichen Xie <zichenxie0106@gmail.com>
---
 drivers/clk/sophgo/clk-sg2042-pll.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/sophgo/clk-sg2042-pll.c b/drivers/clk/sophgo/clk-sg2042-pll.c
index ff9deeef509b..e0f92f7a21bd 100644
--- a/drivers/clk/sophgo/clk-sg2042-pll.c
+++ b/drivers/clk/sophgo/clk-sg2042-pll.c
@@ -153,8 +153,8 @@ static unsigned long sg2042_pll_recalc_rate(unsigned int reg_value,
 
 	sg2042_pll_ctrl_decode(reg_value, &ctrl_table);
 
-	numerator = parent_rate * ctrl_table.fbdiv;
-	denominator = ctrl_table.refdiv * ctrl_table.postdiv1 * ctrl_table.postdiv2;
+	numerator = (u64)parent_rate * ctrl_table.fbdiv;
+	denominator = (u64)ctrl_table.refdiv * ctrl_table.postdiv1 * ctrl_table.postdiv2;
 	do_div(numerator, denominator);
 	return numerator;
 }
-- 
2.34.1

Re: [PATCH] clk: sophgo: Cast an operand to u64 to prevent potential unsigned long overflow on 32-bit machine in sg2042_pll_recalc_rate()

[Chen Wang]

On 2024/10/22 4:51, Gax-c wrote:
> From: Zichen Xie <zichenxie0106@gmail.com>
>
> This was found by a static analyzer.
> There may be a potential integer overflow issue in
> sg2042_pll_recalc_rate(). numerator is defined as u64 while
> parent_rate is defined as unsigned long and ctrl_table.fbdiv
> is defined as unsigned int. On 32-bit machine, the result of
> the calculation will be limited to "u32" without correct casting.
> Integer overflow may occur on high-performance systems.
> For the same reason, adding a cast to denominator could be better.
> So, we recommend adding an extra cast to prevent potential
> integer overflow.
>
> Fixes: 48cf7e01386e ("clk: sophgo: Add SG2042 clock driver")
> Signed-off-by: Zichen Xie <zichenxie0106@gmail.com>
> ---
>   drivers/clk/sophgo/clk-sg2042-pll.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/sophgo/clk-sg2042-pll.c b/drivers/clk/sophgo/clk-sg2042-pll.c
> index ff9deeef509b..e0f92f7a21bd 100644
> --- a/drivers/clk/sophgo/clk-sg2042-pll.c
> +++ b/drivers/clk/sophgo/clk-sg2042-pll.c
> @@ -153,8 +153,8 @@ static unsigned long sg2042_pll_recalc_rate(unsigned int reg_value,
>   
>   	sg2042_pll_ctrl_decode(reg_value, &ctrl_table);
>   
> -	numerator = parent_rate * ctrl_table.fbdiv;
> -	denominator = ctrl_table.refdiv * ctrl_table.postdiv1 * ctrl_table.postdiv2;
> +	numerator = (u64)parent_rate * ctrl_table.fbdiv;
> +	denominator = (u64)ctrl_table.refdiv * ctrl_table.postdiv1 * ctrl_table.postdiv2;
>   	do_div(numerator, denominator);
>   	return numerator;
>   }

LGTM, thanks.

Reviewed-by: Chen Wang <unicorn_wang@outlook.com>

Re: [PATCH] clk: sophgo: Cast an operand to u64 to prevent potential unsigned long overflow on 32-bit machine in sg2042_pll_recalc_rate()

[Dan Carpenter]

On Mon, Oct 21, 2024 at 03:51:02PM -0500, Gax-c wrote:
> From: Zichen Xie <zichenxie0106@gmail.com>
> 
> This was found by a static analyzer.
> There may be a potential integer overflow issue in
> sg2042_pll_recalc_rate(). numerator is defined as u64 while
> parent_rate is defined as unsigned long and ctrl_table.fbdiv
> is defined as unsigned int. On 32-bit machine, the result of
> the calculation will be limited to "u32" without correct casting.
> Integer overflow may occur on high-performance systems.
> For the same reason, adding a cast to denominator could be better.
> So, we recommend adding an extra cast to prevent potential
> integer overflow.
> 
> Fixes: 48cf7e01386e ("clk: sophgo: Add SG2042 clock driver")
> Signed-off-by: Zichen Xie <zichenxie0106@gmail.com>
> ---
>  drivers/clk/sophgo/clk-sg2042-pll.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/sophgo/clk-sg2042-pll.c b/drivers/clk/sophgo/clk-sg2042-pll.c
> index ff9deeef509b..e0f92f7a21bd 100644
> --- a/drivers/clk/sophgo/clk-sg2042-pll.c
> +++ b/drivers/clk/sophgo/clk-sg2042-pll.c
> @@ -153,8 +153,8 @@ static unsigned long sg2042_pll_recalc_rate(unsigned int reg_value,
>  
>  	sg2042_pll_ctrl_decode(reg_value, &ctrl_table);
                                          ^^^^^^^^^^^
>  
> -	numerator = parent_rate * ctrl_table.fbdiv;
> -	denominator = ctrl_table.refdiv * ctrl_table.postdiv1 * ctrl_table.postdiv2;
> +	numerator = (u64)parent_rate * ctrl_table.fbdiv;

This seems reasonable.

> +	denominator = (u64)ctrl_table.refdiv * ctrl_table.postdiv1 * ctrl_table.postdiv2;

These values from sg2042_pll_ctrl_decode() and there is no way they can
overflow.  The highest they can be is 63 * 7 * 7 = 3087.

regards,
dan carpenter

>  	do_div(numerator, denominator);
>  	return numerator;
>  }
> -- 
> 2.34.1

Re: [PATCH] clk: sophgo: Cast an operand to u64 to prevent potential unsigned long overflow on 32-bit machine in sg2042_pll_recalc_rate()

[Zichen Xie]

On 2024/10/22 2:58, Dan Carpenter wrote:
> On Mon, Oct 21, 2024 at 03:51:02PM -0500, Gax-c wrote:
>> From: Zichen Xie <zichenxie0106@gmail.com>
>>
>> This was found by a static analyzer.
>> There may be a potential integer overflow issue in
>> sg2042_pll_recalc_rate(). numerator is defined as u64 while
>> parent_rate is defined as unsigned long and ctrl_table.fbdiv
>> is defined as unsigned int. On 32-bit machine, the result of
>> the calculation will be limited to "u32" without correct casting.
>> Integer overflow may occur on high-performance systems.
>> For the same reason, adding a cast to denominator could be better.
>> So, we recommend adding an extra cast to prevent potential
>> integer overflow.
>>
>> Fixes: 48cf7e01386e ("clk: sophgo: Add SG2042 clock driver")
>> Signed-off-by: Zichen Xie <zichenxie0106@gmail.com>
>> ---
>>   drivers/clk/sophgo/clk-sg2042-pll.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/sophgo/clk-sg2042-pll.c b/drivers/clk/sophgo/clk-sg2042-pll.c
>> index ff9deeef509b..e0f92f7a21bd 100644
>> --- a/drivers/clk/sophgo/clk-sg2042-pll.c
>> +++ b/drivers/clk/sophgo/clk-sg2042-pll.c
>> @@ -153,8 +153,8 @@ static unsigned long sg2042_pll_recalc_rate(unsigned int reg_value,
>>   
>>   	sg2042_pll_ctrl_decode(reg_value, &ctrl_table);
>                                            ^^^^^^^^^^^
>>   
>> -	numerator = parent_rate * ctrl_table.fbdiv;
>> -	denominator = ctrl_table.refdiv * ctrl_table.postdiv1 * ctrl_table.postdiv2;
>> +	numerator = (u64)parent_rate * ctrl_table.fbdiv;
> This seems reasonable.
>
>> +	denominator = (u64)ctrl_table.refdiv * ctrl_table.postdiv1 * ctrl_table.postdiv2;
> These values from sg2042_pll_ctrl_decode() and there is no way they can
> overflow.  The highest they can be is 63 * 7 * 7 = 3087.

You are right. But I think it could be better to add this cast to 
demonstrate that
developers have realized the potential of integer overflow.

And it can also prevent some static analyzers from reporting such bugs.

Anyway, I can still provide a patch with "numerator" cast only if it's 
better.


Best,

Zichen

>
> regards,
> dan carpenter
>
>>   	do_div(numerator, denominator);
>>   	return numerator;
>>   }
>> -- 
>> 2.34.1

Re: [PATCH] clk: sophgo: Cast an operand to u64 to prevent potential unsigned long overflow on 32-bit machine in sg2042_pll_recalc_rate()

[Dan Carpenter]

On Tue, Oct 22, 2024 at 10:39:42AM -0500, Zichen Xie wrote:
> 
> On 2024/10/22 2:58, Dan Carpenter wrote:
> > On Mon, Oct 21, 2024 at 03:51:02PM -0500, Gax-c wrote:
> > > From: Zichen Xie <zichenxie0106@gmail.com>
> > > 
> > > This was found by a static analyzer.
> > > There may be a potential integer overflow issue in
> > > sg2042_pll_recalc_rate(). numerator is defined as u64 while
> > > parent_rate is defined as unsigned long and ctrl_table.fbdiv
> > > is defined as unsigned int. On 32-bit machine, the result of
> > > the calculation will be limited to "u32" without correct casting.
> > > Integer overflow may occur on high-performance systems.
> > > For the same reason, adding a cast to denominator could be better.
> > > So, we recommend adding an extra cast to prevent potential
> > > integer overflow.
> > > 
> > > Fixes: 48cf7e01386e ("clk: sophgo: Add SG2042 clock driver")
> > > Signed-off-by: Zichen Xie <zichenxie0106@gmail.com>
> > > ---
> > >   drivers/clk/sophgo/clk-sg2042-pll.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/clk/sophgo/clk-sg2042-pll.c b/drivers/clk/sophgo/clk-sg2042-pll.c
> > > index ff9deeef509b..e0f92f7a21bd 100644
> > > --- a/drivers/clk/sophgo/clk-sg2042-pll.c
> > > +++ b/drivers/clk/sophgo/clk-sg2042-pll.c
> > > @@ -153,8 +153,8 @@ static unsigned long sg2042_pll_recalc_rate(unsigned int reg_value,
> > >   	sg2042_pll_ctrl_decode(reg_value, &ctrl_table);
> >                                            ^^^^^^^^^^^
> > > -	numerator = parent_rate * ctrl_table.fbdiv;
> > > -	denominator = ctrl_table.refdiv * ctrl_table.postdiv1 * ctrl_table.postdiv2;
> > > +	numerator = (u64)parent_rate * ctrl_table.fbdiv;
> > This seems reasonable.
> > 
> > > +	denominator = (u64)ctrl_table.refdiv * ctrl_table.postdiv1 * ctrl_table.postdiv2;
> > These values from sg2042_pll_ctrl_decode() and there is no way they can
> > overflow.  The highest they can be is 63 * 7 * 7 = 3087.
> 
> You are right. But I think it could be better to add this cast to
> demonstrate that
> developers have realized the potential of integer overflow.
> 
> And it can also prevent some static analyzers from reporting such bugs.

Pointless casting is just confusing and harmful.  The static checker could be
fixed to parse that code correctly.

> 
> Anyway, I can still provide a patch with "numerator" cast only if it's
> better.
> 

Yes, please.

regards,
dan carpenter