| Message ID | 20241021205101.13416-1-zichenxie0106@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: sophgo: Cast an operand to u64 to prevent potential unsigned long overflow on 32-bit machine in sg2042_pll_recalc_rate() | expand |
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>
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
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
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
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; }