linux-kernel - Re: [PATCH] clk: fix a panic error caused by accessing NULL pointer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAAfSe-sr_THp1PhVGG-ZENQwVGVizNxjqweffbgtXmDZ56=+cw@mail.gmail.com>
Date:   Wed, 6 Dec 2017 15:38:47 +0800
From:   Chunyan Zhang <zhang.lyra@...il.com>
To:     Stephen Boyd <sboyd@...eaurora.org>
Cc:     Chunyan Zhang <chunyan.zhang@...eadtrum.com>,
        Michael Turquette <mturquette@...libre.com>,
        linux-clk <linux-clk@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Cai Li <cai.li@...eadtrum.com>,
        Orson Zhai <orson.zhai@...eadtrum.com>,
        Ben Li <ben.li@...eadtrum.com>,
        Xiaolong Zhang <xiaolong.zhang@...eadtrum.com>
Subject: Re: [PATCH] clk: fix a panic error caused by accessing NULL pointer

On 6 December 2017 at 07:28, Stephen Boyd <sboyd@...eaurora.org> wrote:
> On 11/21, Chunyan Zhang wrote:
>> On 21 November 2017 at 16:57, Chunyan Zhang <zhang.lyra@...il.com> wrote:
>> > On 21 November 2017 at 03:12, Stephen Boyd <sboyd@...eaurora.org> wrote:
>> >> On 11/20, Chunyan Zhang wrote:
>> >>> From: Cai Li <cai.li@...eadtrum.com>
>> >>>
>> >>> In some cases the clock parent would be set NULL when doing re-parent,
>> >>> it will cause a NULL pointer accessing if clk_set trace event is enabled,
>> >>> since the trace event function would not check the input parameter.
>> >>>
>> >>> Signed-off-by: Cai Li <cai.li@...eadtrum.com>
>> >>> Signed-off-by: Chunyan Zhang <chunyan.zhang@...eadtrum.com>
>> >>
>> >> Fixes: tag?
>> >>
>> >>> ---
>> >>>  drivers/clk/clk.c | 9 ++++-----
>> >>>  1 file changed, 4 insertions(+), 5 deletions(-)
>> >>>
>> >>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> >>> index c8d83ac..64efaf0 100644
>> >>> --- a/drivers/clk/clk.c
>> >>> +++ b/drivers/clk/clk.c
>> >>> @@ -1242,13 +1242,12 @@ static int __clk_set_parent(struct clk_core *core, struct clk_core *parent,
>> >>>
>> >>>       old_parent = __clk_set_parent_before(core, parent);
>> >>>
>> >>> -     trace_clk_set_parent(core, parent);
>> >>> -
>> >>>       /* change clock input source */
>> >>> -     if (parent && core->ops->set_parent)
>> >>> +     if (parent && core->ops->set_parent) {
>> >>> +             trace_clk_set_parent(core, parent);
>> >>>               ret = core->ops->set_parent(core->hw, p_index);
>> >>> -
>> >>> -     trace_clk_set_parent_complete(core, parent);
>> >>> +             trace_clk_set_parent_complete(core, parent);
>> >>> +     }
>> >>
>> >> Is the problem that parent may be NULL and the tracepoint
>> >> dereferences it?
>> >
>> > Yes, I think that probably is uncommon usage though, it revealed that
>> > the tracepoint could be stronger :)
>>
>> The reason we need to set the parent as NULL is to disable the clk for
>> the purpose of saving power.
>
> Do you have drivers calling set_parent with NULL to save power?
> Seems sort of odd. Why not do something when disabling the clk in

It's a mux clk in the same address area with a device, when powerring
off the device module, the registers of this mux are reset with
default value by hardware.  When the device resumed afterward, the mux
clock needs to recover its previous clk parent, but the clk core would
find it is setting a same clk parent, the function
clk_core_set_parent() [1] will return directly without writing the
exact register to set the parent again.

So setting the clk parent with NULL is just a workaround, we will
appreciate if you have other better solution for us.

Thanks,
Chunyan

[1] http://elixir.free-electrons.com/linux/v4.15-rc1/source/drivers/clk/clk.c#L1886

> clk_disable() path instead? Either way, I'll apply the patch to
> clk-fixes.
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project