* Re: [patch] sh: clkfwk: potential null deref in debug code
2011-03-20 11:12 [patch] sh: clkfwk: potential null deref in debug code Dan Carpenter
@ 2011-03-20 15:57 ` Guennadi Liakhovetski
2011-03-23 13:32 ` Paul Mundt
2011-03-23 14:01 ` Guennadi Liakhovetski
2 siblings, 0 replies; 4+ messages in thread
From: Guennadi Liakhovetski @ 2011-03-20 15:57 UTC (permalink / raw)
To: linux-sh
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/
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [patch] sh: clkfwk: potential null deref in debug code
2011-03-20 11:12 [patch] sh: clkfwk: potential null deref in debug code Dan Carpenter
2011-03-20 15:57 ` Guennadi Liakhovetski
@ 2011-03-23 13:32 ` Paul Mundt
2011-03-23 14:01 ` Guennadi Liakhovetski
2 siblings, 0 replies; 4+ messages in thread
From: Paul Mundt @ 2011-03-23 13:32 UTC (permalink / raw)
To: linux-sh
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 :-)
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [patch] sh: clkfwk: potential null deref in debug code
2011-03-20 11:12 [patch] sh: clkfwk: potential null deref in debug code Dan Carpenter
2011-03-20 15:57 ` Guennadi Liakhovetski
2011-03-23 13:32 ` Paul Mundt
@ 2011-03-23 14:01 ` Guennadi Liakhovetski
2 siblings, 0 replies; 4+ messages in thread
From: Guennadi Liakhovetski @ 2011-03-23 14:01 UTC (permalink / raw)
To: linux-sh
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/
^ permalink raw reply [flat|nested] 4+ messages in thread