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 <dan.carpenter@linaro.org>]

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