linux-kernel - Re: [PATCH] clk: bcm: iproc: Fix overflow in clock calculation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <771696afba71e2f2c059e6690d5ba63fdfd8fcaf.camel@alliedtelesis.co.nz>
Date: Sun, 1 Feb 2026 23:54:34 +0000
From: Mark Tomlinson <Mark.Tomlinson@...iedtelesis.co.nz>
To: "ray.jui@...adcom.com" <ray.jui@...adcom.com>
CC: "bcm-kernel-feedback-list@...adcom.com"
	<bcm-kernel-feedback-list@...adcom.com>, "rjui@...adcom.com"
	<rjui@...adcom.com>, "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
	"sboyd@...nel.org" <sboyd@...nel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] clk: bcm: iproc: Fix overflow in clock calculation

On Fri, 2026-01-30 at 08:52 -0800, Ray Jui wrote:
> 
> 
> On Wed, Jan 28, 2026 at 4:41 PM Mark Tomlinson
> <mark.tomlinson@...iedtelesis.co.nz> wrote:
> > Some BCM iproc devices run at 1.25GHz. This comes from a 50MHZ crystal
> > with a pre-divider of two, a post-divider of two and a PLL multiplier
> > of
> > 100. As the multiply is done first (50M * 100) this overflows a 32-bit
> > value. The multiplication was done as a 64-bit multiply, but the result
> > assigned to a 32-bit variable before the division was done. To fix
> > this,
> > use a 64-bit temporary variable and use the do_div() macro to perform a
> > 64-bit/32-bit division.
> > 
> > Fixes: 5fe225c105fd ("clk: iproc: add initial common clock support")
> > Signed-off-by: Mark Tomlinson <mark.tomlinson@...iedtelesis.co.nz>
> > ---
> >  drivers/clk/bcm/clk-iproc-armpll.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/clk/bcm/clk-iproc-armpll.c b/drivers/clk/bcm/clk-
> > iproc-armpll.c
> > index 9e86c0c10b57..665f1c531c4f 100644
> > --- a/drivers/clk/bcm/clk-iproc-armpll.c
> > +++ b/drivers/clk/bcm/clk-iproc-armpll.c
> > @@ -180,7 +180,7 @@ static unsigned int __get_ndiv(struct iproc_arm_pll
> > *pll)
> >   *   mdiv = ARM PLL post divider
> >   *
> >   * The frequency is calculated by:
> > - *   ((ndiv * parent clock rate) / pdiv) / mdiv
> > + *   (ndiv * parent clock rate) / (pdiv * mdiv)
> >   */
> >  static unsigned long iproc_arm_pll_recalc_rate(struct clk_hw *hw,
> >                 unsigned long parent_rate)
> > @@ -189,6 +189,7 @@ static unsigned long
> > iproc_arm_pll_recalc_rate(struct clk_hw *hw,
> >         u32 val;
> >         int mdiv;
> >         u64 ndiv;
> > +       u64 rate;
> >         unsigned int pdiv;
> > 
> >         /* in bypass mode, use parent rate */
> > @@ -216,8 +217,9 @@ static unsigned long
> > iproc_arm_pll_recalc_rate(struct clk_hw *hw,
> >                 pll->rate = 0;
> >                 return 0;
> >         }
> > -       pll->rate = (ndiv * parent_rate) >> 20;
> > 
> 
> 
> 50 MHz crystal * ndiv of 100 indeed > 32-bit, but the right shift by 20
> would have ensured the result is way less than 32-bit.
> I guess this is not a real world problem (you won't have a crystal as
> reference clock of a PLL running at a frequency much higher than 50 MHz).
> But just to be safe, using a 64-bit temporary variable for 'rate' before
> further division below does not hurt.

My description was a bit simplified. ndiv at this point is shifted left by
20 bits, so is 100 << 20, not just 100. So the calculation after the >> 20
was still 5 billion. This is a real problem on the BCM56160, which uses
these values.

Thanks for the Ack.