linux-kernel - Re: [PATCH v2 1/2] clk: Warn and add workaround on misuse of .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]

Date:   Fri, 10 Feb 2023 16:40:29 -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>
Subject: Re: [PATCH v2 1/2] clk: Warn and add workaround on misuse of .parent_data with .name only

Quoting Christian Marangi (2023-01-31 08:08:28)
> By a simple mistake in a .parent_names to .parent_data conversion it was
> found that clk core assume fw_name is always provided with a parent_data
> struct for each parent and never fallback to .name to get parent name even
> if declared.

It sounds like you have clk_parent_data and the .index member is 0? Can
you show an example structure? I'm guessing it is like this:

	struct clk_parent_data pdata = { .name = "global_name" };

> 
> This is caused by clk_core_get that only checks for parent .fw_name and
> doesn't handle .name.

clk_core_get() is not supposed to operate on the .name member. It is a
firmware based lookup with clkdev as a fallback because clkdev is a
psudeo-firmware interface to assign a name to a clk when some device
pointer is used in conjunction with it.

> 
> While it's sane to request the dev to correctly do the conversion and
> add both .fw_name and .name in a parent_data struct, it's not sane to
> silently drop parents without a warning.

I suppose we can do

	WARN(parent->index >= 0 && !parent_data[i].fw_name && parent_data[i].name, ...);

or maybe better would be to make the clk registration fail if there's a
.name field and the index is non-negative and the fw_name is NULL.

Can you grep the code and see if anyone is assigning a .name without a
.fw_name or .index?

> 
> Fix this in 2 ways. Add a kernel warning when a wrong implementation is
> used and copy .name in .fw_name in parent map populate function to
> handle clk problems and malfunctions.

We shouldn't be copying .name to .fw_name. They're different things.