| 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
| ||
|
Message-Id: <20251023-opp-simpler-code-v2-1-44230ed00fd8@kernel.org>
Date: Thu, 23 Oct 2025 09:59:39 -0400
From: Tamir Duberstein <tamird@...nel.org>
To: Viresh Kumar <vireshk@...nel.org>, Nishanth Menon <nm@...com>,
Stephen Boyd <sboyd@...nel.org>, Miguel Ojeda <ojeda@...nel.org>,
Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>,
Gary Guo <gary@...yguo.net>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>,
Benno Lossin <lossin@...nel.org>, Andreas Hindborg <a.hindborg@...nel.org>,
Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
Danilo Krummrich <dakr@...nel.org>
Cc: linux-pm@...r.kernel.org, rust-for-linux@...r.kernel.org,
linux-kernel@...r.kernel.org, Tamir Duberstein <tamird@...nel.org>
Subject: [PATCH v2] rust: opp: simplify callers of `to_c_str_array`
Use `Option` combinators to make this a bit less noisy.
Wrap the `dev_pm_opp_set_config` operation in a closure and use type
ascription to leverage the compiler to check for use after free.
Signed-off-by: Tamir Duberstein <tamird@...nel.org>
---
Note: this diff is much smaller with whitespace suppressed (`-w`).
An alternative approach to compiler checking for UAF that doesn't change
indentation is to add `drop((self, clk_names, regulator_names))` after
`let ret = ...;` but that felt more prone to becoming out of date if
more owned data needed to be added to the function scope.
---
Changes in v2:
- Avoid use after free; add compiler checking. (Thanks Viresh!)
- Link to v1: https://patch.msgid.link/20251020-opp-simpler-code-v1-1-04f7f447712f@kernel.org
---
rust/kernel/opp.rs | 112 +++++++++++++++++++++++++++--------------------------
1 file changed, 58 insertions(+), 54 deletions(-)
diff --git a/rust/kernel/opp.rs b/rust/kernel/opp.rs
index 04472a8de3ff..f9641c639fff 100644
--- a/rust/kernel/opp.rs
+++ b/rust/kernel/opp.rs
@@ -443,66 +443,70 @@ pub fn set_supported_hw(mut self, hw: KVec<u32>) -> Result<Self> {
///
/// The returned [`ConfigToken`] will remove the configuration when dropped.
pub fn set(self, dev: &Device) -> Result<ConfigToken> {
- let (_clk_list, clk_names) = match &self.clk_names {
- Some(x) => {
- let list = to_c_str_array(x)?;
- let ptr = list.as_ptr();
- (Some(list), ptr)
- }
- None => (None, ptr::null()),
- };
+ let clk_names = self.clk_names.as_deref().map(to_c_str_array).transpose()?;
+ let regulator_names = self
+ .regulator_names
+ .as_deref()
+ .map(to_c_str_array)
+ .transpose()?;
+
+ let set_config = || {
+ let clk_names = clk_names.as_ref().map_or(ptr::null(), |c| c.as_ptr());
+ let regulator_names = regulator_names.as_ref().map_or(ptr::null(), |c| c.as_ptr());
+
+ let prop_name = self
+ .prop_name
+ .as_ref()
+ .map_or(ptr::null(), |p| p.as_char_ptr());
+
+ let (supported_hw, supported_hw_count) = self
+ .supported_hw
+ .as_ref()
+ .map_or((ptr::null(), 0), |hw| (hw.as_ptr(), hw.len() as u32));
+
+ let (required_dev, required_dev_index) = self
+ .required_dev
+ .as_ref()
+ .map_or((ptr::null_mut(), 0), |(dev, idx)| (dev.as_raw(), *idx));
+
+ let mut config = bindings::dev_pm_opp_config {
+ clk_names,
+ config_clks: if T::HAS_CONFIG_CLKS {
+ Some(Self::config_clks)
+ } else {
+ None
+ },
+ prop_name,
+ regulator_names,
+ config_regulators: if T::HAS_CONFIG_REGULATORS {
+ Some(Self::config_regulators)
+ } else {
+ None
+ },
+ supported_hw,
+ supported_hw_count,
- let (_regulator_list, regulator_names) = match &self.regulator_names {
- Some(x) => {
- let list = to_c_str_array(x)?;
- let ptr = list.as_ptr();
- (Some(list), ptr)
- }
- None => (None, ptr::null()),
- };
+ required_dev,
+ required_dev_index,
+ };
- let prop_name = self
- .prop_name
- .as_ref()
- .map_or(ptr::null(), |p| p.as_char_ptr());
-
- let (supported_hw, supported_hw_count) = self
- .supported_hw
- .as_ref()
- .map_or((ptr::null(), 0), |hw| (hw.as_ptr(), hw.len() as u32));
-
- let (required_dev, required_dev_index) = self
- .required_dev
- .as_ref()
- .map_or((ptr::null_mut(), 0), |(dev, idx)| (dev.as_raw(), *idx));
-
- let mut config = bindings::dev_pm_opp_config {
- clk_names,
- config_clks: if T::HAS_CONFIG_CLKS {
- Some(Self::config_clks)
- } else {
- None
- },
- prop_name,
- regulator_names,
- config_regulators: if T::HAS_CONFIG_REGULATORS {
- Some(Self::config_regulators)
- } else {
- None
- },
- supported_hw,
- supported_hw_count,
+ // SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety
+ // requirements. The OPP core guarantees not to access fields of [`Config`] after this
+ // call and so we don't need to save a copy of them for future use.
+ let ret = unsafe { bindings::dev_pm_opp_set_config(dev.as_raw(), &mut config) };
- required_dev,
- required_dev_index,
+ to_result(ret).map(|()| ConfigToken(ret))
};
- // SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety
- // requirements. The OPP core guarantees not to access fields of [`Config`] after this call
- // and so we don't need to save a copy of them for future use.
- let ret = unsafe { bindings::dev_pm_opp_set_config(dev.as_raw(), &mut config) };
+ // Ensure the closure does not accidentally drop owned data; if violated, the compiler
+ // produces E0525 with e.g.:
+ //
+ // ```
+ // closure is `FnOnce` because it moves the variable `clk_names` out of its environment
+ // ```
+ let _: &dyn Fn() -> _ = &set_config;
- to_result(ret).map(|()| ConfigToken(ret))
+ set_config()
}
/// Config's clk callback.
---
base-commit: 173e02d674946ff3ef8da7f44a9d5b820b9af21c
change-id: 20251018-opp-simpler-code-58662c627bf4
Best regards,
--
Tamir Duberstein <tamird@...nel.org>
Powered by blists - more mailing lists