[PATCH] clk: check ops pointer on clock register

[PATCH] clk: check ops pointer on clock register

[Jerome Brunet]

Nothing really prevents a provider from (trying to) register a clock
without providing the clock ops structure.

We do check the individual fields before using them, but not the
structure pointer itself. This may have the usual nasty consequences when
the pointer is dereferenced, mostly likely when checking one the field
during the initialization.

This is fixed by returning an error on clock register if the ops pointer
is NULL

Fixes: b2476490ef11 ("clk: introduce the common clock framework")
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---

Mike, Stephen,

I'm not really sure what the Fixes tag should here. From what I could
see, we never checked the ops pointer before using it since the
beginning of CCF.

Regards
Jerome

 drivers/clk/clk.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 8a1860a36c77..275b45664227 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2683,7 +2683,13 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 		ret = -ENOMEM;
 		goto fail_name;
 	}
+
+	if (!hw->init->ops) {
+		ret = -EINVAL;
+		goto fail_ops;
+	}
 	core->ops = hw->init->ops;
+
 	if (dev && pm_runtime_enabled(dev))
 		core->dev = dev;
 	if (dev && dev->driver)
@@ -2745,6 +2751,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 		kfree_const(core->parent_names[i]);
 	kfree(core->parent_names);
 fail_parent_names:
+fail_ops:
 	kfree_const(core->name);
 fail_name:
 	kfree(core);
-- 
2.14.3

Re: [PATCH] clk: check ops pointer on clock register

[Stephen Boyd]

On 12/18, Jerome Brunet wrote:
> Nothing really prevents a provider from (trying to) register a clock
> without providing the clock ops structure.
> 
> We do check the individual fields before using them, but not the
> structure pointer itself. This may have the usual nasty consequences when
> the pointer is dereferenced, mostly likely when checking one the field
> during the initialization.

Yes, that nasty consequence should be a kernel oops, and the
developer should notice that before submitting the driver for
inclusion. I don't think we really care to return an error here
if this happens.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] clk: check ops pointer on clock register

[Jerome Brunet]

On Mon, 2017-12-18 at 11:03 -0800, Stephen Boyd wrote:
> On 12/18, Jerome Brunet wrote:
> > Nothing really prevents a provider from (trying to) register a clock
> > without providing the clock ops structure.
> > 
> > We do check the individual fields before using them, but not the
> > structure pointer itself. This may have the usual nasty consequences when
> > the pointer is dereferenced, mostly likely when checking one the field
> > during the initialization.
> 
> Yes, that nasty consequence should be a kernel oops,

Precisely

> and the
> developer should notice that before submitting the driver for
> inclusion. 

Agreed. But people may make mistakes, which is why (at least partly) we
do checks, isn't it ?

> I don't think we really care to return an error here
> if this happens.
> 

I don't understand why we would let a oops happen when can catch the error
properly ?

Re: [PATCH] clk: check ops pointer on clock register

[Michael Turquette]

Hi Jerome & Stephen,

On Mon, Dec 18, 2017 at 12:06 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Mon, 2017-12-18 at 11:03 -0800, Stephen Boyd wrote:
>> On 12/18, Jerome Brunet wrote:
>> > Nothing really prevents a provider from (trying to) register a clock
>> > without providing the clock ops structure.
>> >
>> > We do check the individual fields before using them, but not the
>> > structure pointer itself. This may have the usual nasty consequences when
>> > the pointer is dereferenced, mostly likely when checking one the field
>> > during the initialization.
>>
>> Yes, that nasty consequence should be a kernel oops,
>
> Precisely
>
>> and the
>> developer should notice that before submitting the driver for
>> inclusion.
>
> Agreed. But people may make mistakes, which is why (at least partly) we
> do checks, isn't it ?

Agreed the developers should test before submitting, but procedurally
generated clocks (e.g. registering clocks in a loop using a
predictable register map, etc) could lead to a situation where a
developer doesn't test every possible iteration.

Hypothetical, but easy easy easy to fix with Jerome's patch.

>
>> I don't think we really care to return an error here
>> if this happens.
>>
>
> I don't understand why we would let a oops happen when can catch the error
> properly ?
>

Agreed with Jerome on this one.

Let's flip it on its head: any downside to this patch? If not I can merge.

Regards,
Mike

Re: [PATCH] clk: check ops pointer on clock register

[Stephen Boyd]

On 12/18, Michael Turquette wrote:
> Hi Jerome & Stephen,
> 
> On Mon, Dec 18, 2017 at 12:06 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> > On Mon, 2017-12-18 at 11:03 -0800, Stephen Boyd wrote:
> >> On 12/18, Jerome Brunet wrote:
> >> > Nothing really prevents a provider from (trying to) register a clock
> >> > without providing the clock ops structure.
> >> >
> >> > We do check the individual fields before using them, but not the
> >> > structure pointer itself. This may have the usual nasty consequences when
> >> > the pointer is dereferenced, mostly likely when checking one the field
> >> > during the initialization.
> >>
> >> Yes, that nasty consequence should be a kernel oops,
> >
> > Precisely
> >
> >> and the
> >> developer should notice that before submitting the driver for
> >> inclusion.
> >
> > Agreed. But people may make mistakes, which is why (at least partly) we
> > do checks, isn't it ?
> 
> Agreed the developers should test before submitting, but procedurally
> generated clocks (e.g. registering clocks in a loop using a
> predictable register map, etc) could lead to a situation where a
> developer doesn't test every possible iteration.
> 
> Hypothetical, but easy easy easy to fix with Jerome's patch.
> 
> >
> >> I don't think we really care to return an error here
> >> if this happens.
> >>
> >
> > I don't understand why we would let a oops happen when can catch the error
> > properly ?
> >
> 
> Agreed with Jerome on this one.
> 
> Let's flip it on its head: any downside to this patch? If not I can merge.
> 

If code is not checking return values from clk_register(), then
an oops turns into a silently ignored error. Hunting that down is
going to take some time vs. an oops when we attempt to call the
clk ops that aren't there.

The idea is fine, but I would change two things. First I would
throw a WARN_ON() around the condition so developers notice
quicker that something is wrong, and second I would strip off the
'Fixes' tag because this isn't really fixing anything that we
need to backport to stable trees. It just converts an oops into a
warning.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] clk: check ops pointer on clock register

[Jerome Brunet]

On Mon, 2017-12-18 at 13:06 -0800, Stephen Boyd wrote:
> On 12/18, Michael Turquette wrote:
> > Hi Jerome & Stephen,
> > 
> > On Mon, Dec 18, 2017 at 12:06 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> > > On Mon, 2017-12-18 at 11:03 -0800, Stephen Boyd wrote:
> > > > On 12/18, Jerome Brunet wrote:
> > > > > Nothing really prevents a provider from (trying to) register a clock
> > > > > without providing the clock ops structure.
> > > > > 
> > > > > We do check the individual fields before using them, but not the
> > > > > structure pointer itself. This may have the usual nasty consequences when
> > > > > the pointer is dereferenced, mostly likely when checking one the field
> > > > > during the initialization.
> > > > 
> > > > Yes, that nasty consequence should be a kernel oops,
> > > 
> > > Precisely
> > > 
> > > > and the
> > > > developer should notice that before submitting the driver for
> > > > inclusion.
> > > 
> > > Agreed. But people may make mistakes, which is why (at least partly) we
> > > do checks, isn't it ?
> > 
> > Agreed the developers should test before submitting, but procedurally
> > generated clocks (e.g. registering clocks in a loop using a
> > predictable register map, etc) could lead to a situation where a
> > developer doesn't test every possible iteration.
> > 
> > Hypothetical, but easy easy easy to fix with Jerome's patch.
> > 
> > > 
> > > > I don't think we really care to return an error here
> > > > if this happens.
> > > > 
> > > 
> > > I don't understand why we would let a oops happen when can catch the error
> > > properly ?
> > > 
> > 
> > Agreed with Jerome on this one.
> > 
> > Let's flip it on its head: any downside to this patch? If not I can merge.
> > 
> 
> If code is not checking return values from clk_register(), then
> an oops turns into a silently ignored error. 

It would really be asking for trouble, wouldn't it ?

> Hunting that down is
> going to take some time vs. an oops when we attempt to call the
> clk ops that aren't there.
> 
> The idea is fine, but I would change two things. First I would
> throw a WARN_ON() around the condition so developers notice
> quicker that something is wrong, and second I would strip off the
> 'Fixes' tag because this isn't really fixing anything that we
> need to backport to stable trees. It just converts an oops into a
> warning.
> 

Fine by me.

Re: [PATCH] clk: check ops pointer on clock register

[Geert Uytterhoeven]

On Mon, Dec 18, 2017 at 8:03 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 12/18, Jerome Brunet wrote:
>> Nothing really prevents a provider from (trying to) register a clock
>> without providing the clock ops structure.
>>
>> We do check the individual fields before using them, but not the
>> structure pointer itself. This may have the usual nasty consequences when
>> the pointer is dereferenced, mostly likely when checking one the field
>> during the initialization.
>
> Yes, that nasty consequence should be a kernel oops, and the
> developer should notice that before submitting the driver for
> inclusion. I don't think we really care to return an error here
> if this happens.

+1

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds