linux-kernel - Re: [PATCH 1/2] clk: stm32: initialize syscon after clocks are registered

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: <ZaG2ZDCLP34jcI6Y@dell-precision-5540>
Date: Fri, 12 Jan 2024 17:00:04 -0500
From: Ben Wolsieffer <ben.wolsieffer@...ring.com>
To: Stephen Boyd <sboyd@...nel.org>
Cc: devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-stm32@...md-mailman.stormreply.com,
	Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Maxime Coquelin <mcoquelin.stm32@...il.com>,
	Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Michael Turquette <mturquette@...libre.com>
Subject: Re: [PATCH 1/2] clk: stm32: initialize syscon after clocks are
 registered

On Sun, Dec 17, 2023 at 03:05:01PM -0800, Stephen Boyd wrote:
> Quoting Ben Wolsieffer (2023-10-02 11:08:53)
> > The stm32-power-config syscon (PWR peripheral) is used in this driver
> > and the STM32 RTC driver to enable write access to backup domain
> > registers. The syscon's clock has a gate controlled by this clock
> > driver, but this clock is currently not registered in the device tree.
> > This only happens to work currently because all relevant clock setup and
> > RTC initialization happens before clk_disabled_unused(). After this
> > point, all syscon register writes are ignored.
> 
> Seems like we should mark those clks as CLK_IGNORE_UNUSED and add a
> comment to that fact.

That seems like a worse solution than specifying the clock dependency in
the device tree.

> 
> > 
> > If we simply add the syscon clock in the device tree, we end up with a
> > circular dependency because the clock has not been registered at the
> > point this driver requests the syscon.
> > 
> > This patch avoids this circular dependency by moving the syscon lookup
> > after the clocks are registered. This does appear to create a possible
> > race condition where someone could attempt to perform an operation on a
> > backup domain clock before the syscon has been initialized. This would
> > result in the operation having no effect because backup domain writes
> > could not be enabled. I'm not sure if this is a problem or if there is
> > a way to avoid it.
> 
> There's no comment in the code that says the regmap must be set there
> instead of earlier. What's to stop someone from tripping over this
> problem later? At the least, please add a comment.

Yeah, I'll fix that. Do you have any thoughts on the race condition I
described? Should I add some kind of locking to block
enable/disable_power_domain_write_protection() until stm32f4_rcc_init()
attempts to initialize the syscon?

Thank you, Ben