| Message ID | 20241007084840.1167527-1-colin.i.king@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [next] clk: imx: pll14xx: Fix potential integer overflow on multiplication | expand |
On Mon, Oct 07, 2024 at 09:48:40AM +0100, Colin Ian King wrote: > The calculation of fout is using int multiplication and assigning > the result to a u64, this can potentially overflow if the int variable > mdiv is too large. Fix this by making the 65536 a u64 value to ensure a > u64 multiplication is being performed to avoid the overflow. > > Fixes: 53990cf9d5b4 ("clk: imx: pll14xx: consolidate rate calculation") > Signed-off-by: Colin Ian King <colin.i.king@gmail.com> mdiv is always clamped in then 0-1023 range by one of these: mdiv = FIELD_GET(MDIV_MASK, pll_div_ctl0); mdiv = clamp(mdiv, 64, 1023); so it can't overflow and the Fixes tag is unnecessary. I think the reason why "fout" is declared as a u64 is because we were worried that on 32 bit systems the "fout *=" operation could overflow. That looks reasonable to me. regards, dan carpenter
On 07/10/2024 10:54, Dan Carpenter wrote: > On Mon, Oct 07, 2024 at 09:48:40AM +0100, Colin Ian King wrote: >> The calculation of fout is using int multiplication and assigning >> the result to a u64, this can potentially overflow if the int variable >> mdiv is too large. Fix this by making the 65536 a u64 value to ensure a >> u64 multiplication is being performed to avoid the overflow. >> >> Fixes: 53990cf9d5b4 ("clk: imx: pll14xx: consolidate rate calculation") >> Signed-off-by: Colin Ian King <colin.i.king@gmail.com> > > mdiv is always clamped in then 0-1023 range by one of these: > > mdiv = FIELD_GET(MDIV_MASK, pll_div_ctl0); > mdiv = clamp(mdiv, 64, 1023); > > so it can't overflow and the Fixes tag is unnecessary. Good point. > > I think the reason why "fout" is declared as a u64 is because we were worried > that on 32 bit systems the "fout *=" operation could overflow. That looks > reasonable to me. Yes, that makes perfect sense. NAK my patch. Apologies for the noise. Colin > > regards, > dan carpenter >
diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c index d63564dbb12c..2afe361fc711 100644 --- a/drivers/clk/imx/clk-pll14xx.c +++ b/drivers/clk/imx/clk-pll14xx.c @@ -107,7 +107,7 @@ static long pll14xx_calc_rate(struct clk_pll14xx *pll, int mdiv, int pdiv, u64 fout = prate; /* fout = (m * 65536 + k) * Fin / (p * 65536) / (1 << sdiv) */ - fout *= (mdiv * 65536 + kdiv); + fout *= (mdiv * 65536ULL + kdiv); pdiv *= 65536; do_div(fout, pdiv << sdiv);
The calculation of fout is using int multiplication and assigning the result to a u64, this can potentially overflow if the int variable mdiv is too large. Fix this by making the 65536 a u64 value to ensure a u64 multiplication is being performed to avoid the overflow. Fixes: 53990cf9d5b4 ("clk: imx: pll14xx: consolidate rate calculation") Signed-off-by: Colin Ian King <colin.i.king@gmail.com> --- drivers/clk/imx/clk-pll14xx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)