[patch] sh: clkfwk: potential null deref in debug code

[patch] sh: clkfwk: potential null deref in debug code

[Dan Carpenter]

This function is designed to accept a NULL for "best_freq" but the
debug code dereferences it unconditionally.

Signed-off-by: Dan Carpenter <error27@gmail.com>
---
Btw.  Smatch complains that "best" could NULL as well, but I don't
know if actually that's possible so I left it as is.

diff --git a/drivers/sh/clk/core.c b/drivers/sh/clk/core.c
index 5f63c3b..dee971c 100644
--- a/drivers/sh/clk/core.c
+++ b/drivers/sh/clk/core.c
@@ -616,7 +616,7 @@ long clk_round_parent(struct clk *clk, unsigned long target,
 
 		pr_debug("%u / %lu = %lu, / %lu = %lu, best %lu, parent %u\n",
 			 freq->frequency, div, freq_high, div + 1, freq_low,
-			 *best_freq, best->frequency);
+			 best_freq ? *best_freq : -1, best->frequency);
 
 		if (!error)
 			break;

Re: [patch] sh: clkfwk: potential null deref in debug code

[Guennadi Liakhovetski]

Hi Dan

On Sun, 20 Mar 2011, Dan Carpenter wrote:

> This function is designed to accept a NULL for "best_freq" but the
> debug code dereferences it unconditionally.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> Btw.  Smatch complains that "best" could NULL as well, but I don't
> know if actually that's possible so I left it as is.
> 
> diff --git a/drivers/sh/clk/core.c b/drivers/sh/clk/core.c
> index 5f63c3b..dee971c 100644
> --- a/drivers/sh/clk/core.c
> +++ b/drivers/sh/clk/core.c
> @@ -616,7 +616,7 @@ long clk_round_parent(struct clk *clk, unsigned long target,
>  
>  		pr_debug("%u / %lu = %lu, / %lu = %lu, best %lu, parent %u\n",
>  			 freq->frequency, div, freq_high, div + 1, freq_low,
> -			 *best_freq, best->frequency);
> +			 best_freq ? *best_freq : -1, best->frequency);

What about another occasion of dereferencing best_freq at the top of the 
clk_round_parent() function:

	if (!parent) {
		*parent_freq = 0;
		*best_freq = clk_round_rate(clk, target);
		return abs(target - *best_freq);
	}

?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [patch] sh: clkfwk: potential null deref in debug code

[Paul Mundt]

On Sun, Mar 20, 2011 at 04:57:50PM +0100, Guennadi Liakhovetski wrote:
> Hi Dan
> 
> On Sun, 20 Mar 2011, Dan Carpenter wrote:
> 
> > This function is designed to accept a NULL for "best_freq" but the
> > debug code dereferences it unconditionally.
> > 
> > Signed-off-by: Dan Carpenter <error27@gmail.com>
> > ---
> > Btw.  Smatch complains that "best" could NULL as well, but I don't
> > know if actually that's possible so I left it as is.
> > 
> > diff --git a/drivers/sh/clk/core.c b/drivers/sh/clk/core.c
> > index 5f63c3b..dee971c 100644
> > --- a/drivers/sh/clk/core.c
> > +++ b/drivers/sh/clk/core.c
> > @@ -616,7 +616,7 @@ long clk_round_parent(struct clk *clk, unsigned long target,
> >  
> >  		pr_debug("%u / %lu = %lu, / %lu = %lu, best %lu, parent %u\n",
> >  			 freq->frequency, div, freq_high, div + 1, freq_low,
> > -			 *best_freq, best->frequency);
> > +			 best_freq ? *best_freq : -1, best->frequency);
> 
> What about another occasion of dereferencing best_freq at the top of the 
> clk_round_parent() function:
> 
> 	if (!parent) {
> 		*parent_freq = 0;
> 		*best_freq = clk_round_rate(clk, target);
> 		return abs(target - *best_freq);
> 	}
> 
> ?
> 
Do we actually have users that are passing in NULL for best_freq? I don't
recall writing this code, so I assume you did :-)

Re: [patch] sh: clkfwk: potential null deref in debug code

[Guennadi Liakhovetski]

On Wed, 23 Mar 2011, Paul Mundt wrote:

> On Sun, Mar 20, 2011 at 04:57:50PM +0100, Guennadi Liakhovetski wrote:
> > Hi Dan
> > 
> > On Sun, 20 Mar 2011, Dan Carpenter wrote:
> > 
> > > This function is designed to accept a NULL for "best_freq" but the
> > > debug code dereferences it unconditionally.
> > > 
> > > Signed-off-by: Dan Carpenter <error27@gmail.com>
> > > ---
> > > Btw.  Smatch complains that "best" could NULL as well, but I don't
> > > know if actually that's possible so I left it as is.
> > > 
> > > diff --git a/drivers/sh/clk/core.c b/drivers/sh/clk/core.c
> > > index 5f63c3b..dee971c 100644
> > > --- a/drivers/sh/clk/core.c
> > > +++ b/drivers/sh/clk/core.c
> > > @@ -616,7 +616,7 @@ long clk_round_parent(struct clk *clk, unsigned long target,
> > >  
> > >  		pr_debug("%u / %lu = %lu, / %lu = %lu, best %lu, parent %u\n",
> > >  			 freq->frequency, div, freq_high, div + 1, freq_low,
> > > -			 *best_freq, best->frequency);
> > > +			 best_freq ? *best_freq : -1, best->frequency);
> > 
> > What about another occasion of dereferencing best_freq at the top of the 
> > clk_round_parent() function:
> > 
> > 	if (!parent) {
> > 		*parent_freq = 0;
> > 		*best_freq = clk_round_rate(clk, target);
> > 		return abs(target - *best_freq);
> > 	}
> > 
> > ?
> > 
> Do we actually have users that are passing in NULL for best_freq? I don't
> recall writing this code, so I assume you did :-)

That seems very likely;) Yes, that's from my patch, and so far we have 
only one user - ap4evb_clk_optimize() and it doesn't pass NULL to the fn. 
But perhaps it is better to be consistent and if the variable is checked 
for NULL on some occasions, it should be checked everywhere.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/