linux-kernel - Re: [PATCH] clk: Fix wrong clock returned in parent

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: <6dcb55104d5065d30a9dab4bca878922.sboyd@kernel.org>
Date:   Fri, 17 Feb 2023 14:11:20 -0800
From:   Stephen Boyd <sboyd@...nel.org>
To:     Christian Marangi <ansuelsmth@...il.com>,
        Michael Turquette <mturquette@...libre.com>,
        linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     Christian Marangi <ansuelsmth@...il.com>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Jerome Brunet <jbrunet@...libre.com>,
        Russell King <linux@...linux.org.uk>,
        Jeffrey Hugo <jhugo@...eaurora.org>,
        Chen-Yu Tsai <wens@...e.org>
Subject: Re: [PATCH] clk: Fix wrong clock returned in parent_data with .name and no .index

Quoting Christian Marangi (2023-02-15 15:27:12)
> Commit 601b6e93304a ("clk: Allow parents to be specified via clkspec index")
> introduced a regression due to a "fragile" implementation present in some very
> corner case.
> 
> Such commit introduced the support for parents to be specified using
> clkspec index. The index is an int and should be -1 if the feature
> should not be used. This is the case with parent_hws or legacy
> parent_names used and the index value is set to -1 by default.
> With parent_data the situation is different, since it's a struct that
> can have multiple value (.index, .name, .fw_name), it's init to all 0 by
> default. This cause the index value to be set to 0 everytime even if not

It's only initialized to all 0 because that's what you've decided to do.
It could be on the stack and have random stack junk values.

> intended to be defined and used.
> 
> This simple "fragile" implementation cause side-effect and unintended
> behaviour.
> 
> Assuming the following scenario (to repro the corner case and doesn't
> reflect real code):
> 
> In dt we have a node like this:
>                 acc1: clock-controller@...8000 {
>                         compatible = "qcom,kpss-acc-v1";
>                         reg = <0x02098000 0x1000>, <0x02008000 0x1000>;
>                         clock-output-names = "acpu1_aux";
>                         clocks = <&pxo_board>;
>                         clock-names = "pxo";
>                         #clock-cells = <0>;
>                 };
> 
> And on the relevant driver we have the parent data defined as such:
>                 static const struct clk_parent_data aux_parents[] = {
>                         { .name = "pll8_vote" },
>                         { .fw_name = "pxo", .name = "pxo_board" },
>                 };
> 
> Someone would expect the first parent to be globally searched and set to
> point to the clock named "pll8_vote".
> But this is not the case and instead under the hood, the parent point to
> the pxo clock. This happen without any warning and was discovered on
> another platform while the gcc driver was converted to parent_data and
> only .name was defined.

You didn't set .index explicitly to zero, but it is zero because of the
use of static struct initializers here. If the struct was on the stack
nobody knows what the value would be. Set -1 if you don't want to use
the index lookup path.