linux-kernel - Re: [PATCH RESEND 1/2] clk: Implement protected-clocks for all OF clock providers

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200903073159.2tabexowuo7wgdnh@gilmour.lan>
Date:   Thu, 3 Sep 2020 09:31:59 +0200
From:   Maxime Ripard <maxime@...no.tech>
To:     Samuel Holland <samuel@...lland.org>
Cc:     Stephen Boyd <sboyd@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        linux-arm-msm@...r.kernel.org, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH RESEND 1/2] clk: Implement protected-clocks for all OF
 clock providers

On Wed, Sep 02, 2020 at 11:00:14PM -0500, Samuel Holland wrote:
> This is a generic implementation of the "protected-clocks" property from
> the common clock binding. It allows firmware to inform the OS about
> clocks that must not be disabled while the OS is running.
> 
> This implementation comes with some caveats:
> 
> 1) Clocks that have CLK_IS_CRITICAL in their init data are prepared/
> enabled before they are attached to the clock tree. protected-clocks are
> only protected once the clock provider is added, which is generally
> after all of the clocks it provides have been registered. This leaves a
> window of opportunity where something could disable or modify the clock,
> such as a driver running on another CPU, or the clock core itself. There
> is a comment to this effect in __clk_core_init():
> 
>   /*
>    * Enable CLK_IS_CRITICAL clocks so newly added critical clocks
>    * don't get accidentally disabled when walking the orphan tree and
>    * reparenting clocks
>    */
> 
> Similarly, these clocks will be enabled after they are first reparented,
> unlike other CLK_IS_CRITICAL clocks. See the comment in
> clk_core_reparent_orphans_nolock():
> 
>   /*
>    * We need to use __clk_set_parent_before() and _after() to
>    * to properly migrate any prepare/enable count of the orphan
>    * clock. This is important for CLK_IS_CRITICAL clocks, which
>    * are enabled during init but might not have a parent yet.
>    */
> 
> Ideally we could detect protected clocks before they are reparented, but
> there are two problems with that:
> 
>   a) From the clock core's perspective, hw->init is const.
> 
>   b) The clock core doesn't see the device_node until __clk_register is
>      called on the first clock.
> 
> So the only "race-free" way to detect protected-clocks is to do it in
> the middle of __clk_register, between when core->flags is initialized
> and calling __clk_core_init(). That requires scanning the device tree
> again for each clock, which is part of why I didn't do it that way.
> 
> 2) __clk_protect needs to be idempotent, for two reasons:
> 
>   a) Clocks with CLK_IS_CRITICAL in their init data are already
>      prepared/enabled, and we don't want to prepare/enable them again.
> 
>   b) of_clk_set_defaults() is called twice for (at least some) clock
>      controllers registered with CLK_OF_DECLARE. It is called first in
>      of_clk_add_provider()/of_clk_add_hw_provider() inside clk_init_cb,
>      and again afterward in of_clk_init(). The second call in
>      of_clk_init() may be unnecessary, but verifying that would require
>      auditing all users of CLK_OF_DECLARE to ensure they called one of
>      the of_clk_add{,_hw}_provider functions.
> 
> Signed-off-by: Samuel Holland <samuel@...lland.org>

Reviewed-by: Maxime Ripard <mripard@...nel.org>

Thanks!
Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)