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 <zichenxie0106@gmail.com>]

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