[PATCH] clk: sunxi: add explicit casting to prevent overflow

[PATCH] clk: sunxi: add explicit casting to prevent overflow

[Anastasia Belova]

If n = 255, the result of multiplication of n and 24000000
may not fit int type. Add explicit casting to prevent overflow.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 6424e0aeebc4 ("clk: sunxi: rewrite sun9i_a80_get_pll4_factors()")
Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
---
 drivers/clk/sunxi/clk-sun9i-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/sunxi/clk-sun9i-core.c b/drivers/clk/sunxi/clk-sun9i-core.c
index d93c7a53c6c0..70fbd7390d96 100644
--- a/drivers/clk/sunxi/clk-sun9i-core.c
+++ b/drivers/clk/sunxi/clk-sun9i-core.c
@@ -50,7 +50,7 @@ static void sun9i_a80_get_pll4_factors(struct factors_request *req)
 	else if (n < 12)
 		n = 12;
 
-	req->rate = ((24000000 * n) >> p) / (m + 1);
+	req->rate = ((24000000ULL * n) >> p) / (m + 1);
 	req->n = n;
 	req->m = m;
 	req->p = p;
-- 
2.43.0

Re: [PATCH] clk: sunxi: add explicit casting to prevent overflow

[David Laight]

On Mon, 20 Jan 2025 11:47:16 +0300
Anastasia Belova <abelova@astralinux.ru> wrote:

> If n = 255, the result of multiplication of n and 24000000
> may not fit int type. Add explicit casting to prevent overflow.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.

You need to read and understand the code before writing any patches.
The '>> p' and '/ (m + 1)' are both just conditional 'divide by 2'.
So can be done before the multiply.
Since req->rate is 'signed long' and the value is a frequency it is
only just possible that it exceeds 31 bits (and will be wrong on 32bit
builds - but sun-9 might be 64bit only?)

In any case it would be sensible to force an unsigned divide.
So perhaps:
	unsigned int n = DIV_ROUND_UP(req->rate, 6000000ul);
	...
	req->rate = ((24000000ul >> p) / (m + 1)) * n;

David

> 
> Fixes: 6424e0aeebc4 ("clk: sunxi: rewrite sun9i_a80_get_pll4_factors()")
> Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
> ---
>  drivers/clk/sunxi/clk-sun9i-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/sunxi/clk-sun9i-core.c b/drivers/clk/sunxi/clk-sun9i-core.c
> index d93c7a53c6c0..70fbd7390d96 100644
> --- a/drivers/clk/sunxi/clk-sun9i-core.c
> +++ b/drivers/clk/sunxi/clk-sun9i-core.c
> @@ -50,7 +50,7 @@ static void sun9i_a80_get_pll4_factors(struct factors_request *req)
>  	else if (n < 12)
>  		n = 12;
>  
> -	req->rate = ((24000000 * n) >> p) / (m + 1);
> +	req->rate = ((24000000ULL * n) >> p) / (m + 1);
>  	req->n = n;
>  	req->m = m;
>  	req->p = p;

Re: [PATCH] clk: sunxi: add explicit casting to prevent overflow

[Andre Przywara]

On Wed, 22 Jan 2025 22:58:05 +0000
David Laight <david.laight.linux@gmail.com> wrote:

Hi,

please note that this is all practically irrelevant:
- PLL4 is PLL_PERIPH0, which is meant to be fixed to 960MHz. Linux
  would not change this frequency.
- the Allwinner A80 is both old and quite rare/obscure: the most
  prominent board (Cubieboard4) was broken for a while and nobody
  noticed
- this "allwinner,sun9i-a80-pll4-clk" clock is not used by any DT
  in the kernel, so it's effectively dead code

But just for sports:

> On Mon, 20 Jan 2025 11:47:16 +0300
> Anastasia Belova <abelova@astralinux.ru> wrote:
> 
> > If n = 255, the result of multiplication of n and 24000000
> > may not fit int type. Add explicit casting to prevent overflow.
> > 
> > Found by Linux Verification Center (linuxtesting.org) with SVACE.  
> 
> You need to read and understand the code before writing any patches.
> The '>> p' and '/ (m + 1)' are both just conditional 'divide by 2'.
> So can be done before the multiply.

Well, normally you would try to multiply first, then divide, to avoid
losing precision. In this case it's fine, since it's just dividing by 2
or 4, and 24E6 is dividable by both, so no loss. But the formula in the
data sheet is written as "24MHz*N/(Input_div+1)/(Output_div+1)", which
matches the code (somewhat).
So I think it's indeed better to divide first here, to avoid using
heavy library based 64-bit mul/div algorithms, just for this one corner
case, but it would need a comment, to point to the problem and avoid
people "fixing it back".

> Since req->rate is 'signed long' and the value is a frequency it is

struct factors_request.rate is "unsigned long"

> only just possible that it exceeds 31 bits (and will be wrong on 32bit
> builds - but sun-9 might be 64bit only?)

The A80 has Cortex-A7 cores, so it's 32-bit only. The SoC can address
more than 4GB, but that's not relevant here.
 
> In any case it would be sensible to force an unsigned divide.
> So perhaps:
> 	unsigned int n = DIV_ROUND_UP(req->rate, 6000000ul);
> 	...
> 	req->rate = ((24000000ul >> p) / (m + 1)) * n;

Yeah, I don't think we need the "long" qualifier, but this looks like
indeed the best solution, just with an added comment. And we probably
want to change the type of "p" and "m" to u8 on the way, to match the
struct and make them unsigned as well.

Cheers,
Andre
 

> 
> David
> 
> > 
> > Fixes: 6424e0aeebc4 ("clk: sunxi: rewrite sun9i_a80_get_pll4_factors()")
> > Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
> > ---
> >  drivers/clk/sunxi/clk-sun9i-core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/sunxi/clk-sun9i-core.c b/drivers/clk/sunxi/clk-sun9i-core.c
> > index d93c7a53c6c0..70fbd7390d96 100644
> > --- a/drivers/clk/sunxi/clk-sun9i-core.c
> > +++ b/drivers/clk/sunxi/clk-sun9i-core.c
> > @@ -50,7 +50,7 @@ static void sun9i_a80_get_pll4_factors(struct factors_request *req)
> >  	else if (n < 12)
> >  		n = 12;
> >  
> > -	req->rate = ((24000000 * n) >> p) / (m + 1);
> > +	req->rate = ((24000000ULL * n) >> p) / (m + 1);
> >  	req->n = n;
> >  	req->m = m;
> >  	req->p = p;  
> 
> 

Re: [PATCH] clk: sunxi: add explicit casting to prevent overflow

[David Laight]

On Thu, 23 Jan 2025 00:55:56 +0000
Andre Przywara <andre.przywara@arm.com> wrote:

> On Wed, 22 Jan 2025 22:58:05 +0000
> David Laight <david.laight.linux@gmail.com> wrote:
> 
> Hi,
> 
> please note that this is all practically irrelevant:
> - PLL4 is PLL_PERIPH0, which is meant to be fixed to 960MHz. Linux
>   would not change this frequency.
> - the Allwinner A80 is both old and quite rare/obscure: the most
>   prominent board (Cubieboard4) was broken for a while and nobody
>   noticed
> - this "allwinner,sun9i-a80-pll4-clk" clock is not used by any DT
>   in the kernel, so it's effectively dead code
> 
> But just for sports:

Doesn't surprise me ...

> 
> > On Mon, 20 Jan 2025 11:47:16 +0300
> > Anastasia Belova <abelova@astralinux.ru> wrote:
> >   
> > > If n = 255, the result of multiplication of n and 24000000
> > > may not fit int type. Add explicit casting to prevent overflow.
> > > 
> > > Found by Linux Verification Center (linuxtesting.org) with SVACE.    
> > 
> > You need to read and understand the code before writing any patches.
> > The '>> p' and '/ (m + 1)' are both just conditional 'divide by 2'.
> > So can be done before the multiply.  
> 
> Well, normally you would try to multiply first, then divide, to avoid
> losing precision. In this case it's fine, since it's just dividing by 2
> or 4, and 24E6 is dividable by both, so no loss. But the formula in the
> data sheet is written as "24MHz*N/(Input_div+1)/(Output_div+1)", which
> matches the code (somewhat).

That PLL can generate all sorts of frequencies due to the multiply
and divide (as well as the shift).
The code was clearly sub-optimal for arbitrary frequencies :-)
 
> So I think it's indeed better to divide first here, to avoid using
> heavy library based 64-bit mul/div algorithms, just for this one corner
> case, but it would need a comment, to point to the problem and avoid
> people "fixing it back".
> 
> > Since req->rate is 'signed long' and the value is a frequency it is  
> 
> struct factors_request.rate is "unsigned long"
> 
> > only just possible that it exceeds 31 bits (and will be wrong on 32bit
> > builds - but sun-9 might be 64bit only?)  
> 
> The A80 has Cortex-A7 cores, so it's 32-bit only. The SoC can address
> more than 4GB, but that's not relevant here.

I couldn't decide whether the code was for 32bit or not.
Using 'long' is pretty dubious almost everywhere.
I'm sure it is a hangover from people worried about int being 16bit.
But that has never been true for linux (or pretty much any unix since
the early 1980s).

>  
> > In any case it would be sensible to force an unsigned divide.
> > So perhaps:
> > 	unsigned int n = DIV_ROUND_UP(req->rate, 6000000ul);
> > 	...
> > 	req->rate = ((24000000ul >> p) / (m + 1)) * n;  
> 
> Yeah, I don't think we need the "long" qualifier, but this looks like
> indeed the best solution, just with an added comment.

Maybe just mention it only need to generate 96MHz.

> And we probably
> want to change the type of "p" and "m" to u8 on the way, to match the
> struct and make them unsigned as well.

Make them unsigned, but not u8.
The u8 would get promoted to signed int before any arithmetic.

	David

> 
> Cheers,
> Andre
>  
> 
> > 
> > David
> >   
> > > 
> > > Fixes: 6424e0aeebc4 ("clk: sunxi: rewrite sun9i_a80_get_pll4_factors()")
> > > Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
> > > ---
> > >  drivers/clk/sunxi/clk-sun9i-core.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/clk/sunxi/clk-sun9i-core.c b/drivers/clk/sunxi/clk-sun9i-core.c
> > > index d93c7a53c6c0..70fbd7390d96 100644
> > > --- a/drivers/clk/sunxi/clk-sun9i-core.c
> > > +++ b/drivers/clk/sunxi/clk-sun9i-core.c
> > > @@ -50,7 +50,7 @@ static void sun9i_a80_get_pll4_factors(struct factors_request *req)
> > >  	else if (n < 12)
> > >  		n = 12;
> > >  
> > > -	req->rate = ((24000000 * n) >> p) / (m + 1);
> > > +	req->rate = ((24000000ULL * n) >> p) / (m + 1);
> > >  	req->n = n;
> > >  	req->m = m;
> > >  	req->p = p;    
> > 
> >   
>