| Message ID | 1423649612-31746-2-git-send-email-javier.martinez@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/11, Javier Martinez Canillas wrote: > The clock passed as an argument to clk_mux_determine_rate_flags() can > not have a parent clock if is either a root clock or an orphan. > > In those cases parent is NULL so parent->hw shouldn't be dereferenced. > > Fixes: 035a61c314eb3 ("clk: Make clk API return per-user struct clk instances") > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > --- > drivers/clk/clk.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 7f53166af5e6..7bd8893c94d6 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -799,7 +799,7 @@ clk_mux_determine_rate_flags(struct clk_hw *hw, unsigned long rate, > /* if NO_REPARENT flag set, pass through to current parent */ > if (core->flags & CLK_SET_RATE_NO_REPARENT) { > parent = core->parent; > - if (core->flags & CLK_SET_RATE_PARENT) > + if (core->flags & CLK_SET_RATE_PARENT && parent) > best = __clk_determine_rate(parent->hw, rate, > min_rate, max_rate); > else if (parent) Sorry this doesn't look right. Before all the recent changes to this file we would call __clk_round_rate() which would return 0 if the first argument was NULL. Now we're going to take the else if path and do something different. So we need a parent ? parent->hw : NULL here. Of course, I wonder why a clock has the CLK_SET_RATE_PARENT flag set if it doesn't actually have a parent. That also seems wrong.
Hello Stephen, Thanks a lot for your feedback. On 02/11/2015 07:54 PM, Stephen Boyd wrote: > On 02/11, Javier Martinez Canillas wrote: >> --- a/drivers/clk/clk.c >> +++ b/drivers/clk/clk.c >> @@ -799,7 +799,7 @@ clk_mux_determine_rate_flags(struct clk_hw *hw, unsigned long rate, >> /* if NO_REPARENT flag set, pass through to current parent */ >> if (core->flags & CLK_SET_RATE_NO_REPARENT) { >> parent = core->parent; >> - if (core->flags & CLK_SET_RATE_PARENT) >> + if (core->flags & CLK_SET_RATE_PARENT && parent) >> best = __clk_determine_rate(parent->hw, rate, >> min_rate, max_rate); >> else if (parent) > > Sorry this doesn't look right. Before all the recent changes to > this file we would call __clk_round_rate() which would return 0 > if the first argument was NULL. Now we're going to take the else > if path and do something different. So we need a parent ? > parent->hw : NULL here. > Right, I'm not that familiar with the common clock framework so I didn't realize I was changing the behavior, sorry about that... > Of course, I wonder why a clock has the CLK_SET_RATE_PARENT flag > set if it doesn't actually have a parent. That also seems wrong. > Yes, I did not face this issue and only patch #2 was enough to fix my problem but the theoretical NULL pointer dereference was found when reading the code. I agree that a clock with that flag set should have at least one parent but afaict there is no sanity check on clock registration. And even if that was the case, I believe that the core should be robust enough to check for NULL before trying to dereference it. I'll post a v2 passing NULL as an argument and parent->hw if parent is not NULL as you suggested. Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 7f53166af5e6..7bd8893c94d6 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -799,7 +799,7 @@ clk_mux_determine_rate_flags(struct clk_hw *hw, unsigned long rate, /* if NO_REPARENT flag set, pass through to current parent */ if (core->flags & CLK_SET_RATE_NO_REPARENT) { parent = core->parent; - if (core->flags & CLK_SET_RATE_PARENT) + if (core->flags & CLK_SET_RATE_PARENT && parent) best = __clk_determine_rate(parent->hw, rate, min_rate, max_rate); else if (parent)
The clock passed as an argument to clk_mux_determine_rate_flags() can not have a parent clock if is either a root clock or an orphan. In those cases parent is NULL so parent->hw shouldn't be dereferenced. Fixes: 035a61c314eb3 ("clk: Make clk API return per-user struct clk instances") Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> --- drivers/clk/clk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)