The patch below does not apply to the 6.1-stable tree. If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to stable@vger.kernel.org.
To reproduce the conflict and resubmit, you may use the following commands:
git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-6.1.y git checkout FETCH_HEAD git cherry-pick -x 7044dcff8301b29269016ebd17df27c4736140d2 # <resolve conflicts, build, test, etc.> git commit -s git send-email --to 'stable@vger.kernel.org' --in-reply-to '2024042940-plod-embellish-5a76@gregkh' --subject-prefix 'PATCH 6.1.y' HEAD^..
Possible dependencies:
7044dcff8301 ("rust: macros: fix soundness issue in `module!` macro") 1b6170ff7a20 ("rust: module: place generated init_module() function in .init.text") 41bdc6decda0 ("btf, scripts: rust: drop is_rust_module.sh") 310897659cf0 ("Merge tag 'rust-6.4' of https://github.com/Rust-for-Linux/linux")
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
From 7044dcff8301b29269016ebd17df27c4736140d2 Mon Sep 17 00:00:00 2001 From: Benno Lossin benno.lossin@proton.me Date: Mon, 1 Apr 2024 18:52:50 +0000 Subject: [PATCH] rust: macros: fix soundness issue in `module!` macro MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
The `module!` macro creates glue code that are called by C to initialize the Rust modules using the `Module::init` function. Part of this glue code are the local functions `__init` and `__exit` that are used to initialize/destroy the Rust module.
These functions are safe and also visible to the Rust mod in which the `module!` macro is invoked. This means that they can be called by other safe Rust code. But since they contain `unsafe` blocks that rely on only being called at the right time, this is a soundness issue.
Wrap these generated functions inside of two private modules, this guarantees that the public functions cannot be called from the outside. Make the safe functions `unsafe` and add SAFETY comments.
Cc: stable@vger.kernel.org Reported-by: Björn Roy Baron bjorn3_gh@protonmail.com Closes: https://github.com/Rust-for-Linux/linux/issues/629 Fixes: 1fbde52bde73 ("rust: add `macros` crate") Signed-off-by: Benno Lossin benno.lossin@proton.me Reviewed-by: Wedson Almeida Filho walmeida@microsoft.com Link: https://lore.kernel.org/r/20240401185222.12015-1-benno.lossin@proton.me [ Moved `THIS_MODULE` out of the private-in-private modules since it should remain public, as Dirk Behme noticed [1]. Capitalized comments, avoided newline in non-list SAFETY comments and reworded to add Reported-by and newline. ] Link: https://rust-for-linux.zulipchat.com/#narrow/stream/291565-Help/topic/x/near... [1] Signed-off-by: Miguel Ojeda ojeda@kernel.org
diff --git a/rust/macros/module.rs b/rust/macros/module.rs index 27979e582e4b..acd0393b5095 100644 --- a/rust/macros/module.rs +++ b/rust/macros/module.rs @@ -199,17 +199,6 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream { /// Used by the printing macros, e.g. [`info!`]. const __LOG_PREFIX: &[u8] = b"{name}\0";
- /// The "Rust loadable module" mark. - // - // This may be best done another way later on, e.g. as a new modinfo - // key or a new section. For the moment, keep it simple. - #[cfg(MODULE)] - #[doc(hidden)] - #[used] - static __IS_RUST_MODULE: () = (); - - static mut __MOD: Option<{type_}> = None; - // SAFETY: `__this_module` is constructed by the kernel at load time and will not be // freed until the module is unloaded. #[cfg(MODULE)] @@ -221,81 +210,132 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream { kernel::ThisModule::from_ptr(core::ptr::null_mut()) }};
- // Loadable modules need to export the `{{init,cleanup}}_module` identifiers. - /// # Safety - /// - /// This function must not be called after module initialization, because it may be - /// freed after that completes. - #[cfg(MODULE)] - #[doc(hidden)] - #[no_mangle] - #[link_section = ".init.text"] - pub unsafe extern "C" fn init_module() -> core::ffi::c_int {{ - __init() - }} + // Double nested modules, since then nobody can access the public items inside. + mod __module_init {{ + mod __module_init {{ + use super::super::{type_};
- #[cfg(MODULE)] - #[doc(hidden)] - #[no_mangle] - pub extern "C" fn cleanup_module() {{ - __exit() - }} + /// The "Rust loadable module" mark. + // + // This may be best done another way later on, e.g. as a new modinfo + // key or a new section. For the moment, keep it simple. + #[cfg(MODULE)] + #[doc(hidden)] + #[used] + static __IS_RUST_MODULE: () = ();
- // Built-in modules are initialized through an initcall pointer - // and the identifiers need to be unique. - #[cfg(not(MODULE))] - #[cfg(not(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS))] - #[doc(hidden)] - #[link_section = "{initcall_section}"] - #[used] - pub static __{name}_initcall: extern "C" fn() -> core::ffi::c_int = __{name}_init; + static mut __MOD: Option<{type_}> = None;
- #[cfg(not(MODULE))] - #[cfg(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS)] - core::arch::global_asm!( - r#".section "{initcall_section}", "a" - __{name}_initcall: - .long __{name}_init - . - .previous - "# - ); + // Loadable modules need to export the `{{init,cleanup}}_module` identifiers. + /// # Safety + /// + /// This function must not be called after module initialization, because it may be + /// freed after that completes. + #[cfg(MODULE)] + #[doc(hidden)] + #[no_mangle] + #[link_section = ".init.text"] + pub unsafe extern "C" fn init_module() -> core::ffi::c_int {{ + // SAFETY: This function is inaccessible to the outside due to the double + // module wrapping it. It is called exactly once by the C side via its + // unique name. + unsafe {{ __init() }} + }}
- #[cfg(not(MODULE))] - #[doc(hidden)] - #[no_mangle] - pub extern "C" fn __{name}_init() -> core::ffi::c_int {{ - __init() - }} + #[cfg(MODULE)] + #[doc(hidden)] + #[no_mangle] + pub extern "C" fn cleanup_module() {{ + // SAFETY: + // - This function is inaccessible to the outside due to the double + // module wrapping it. It is called exactly once by the C side via its + // unique name, + // - furthermore it is only called after `init_module` has returned `0` + // (which delegates to `__init`). + unsafe {{ __exit() }} + }}
- #[cfg(not(MODULE))] - #[doc(hidden)] - #[no_mangle] - pub extern "C" fn __{name}_exit() {{ - __exit() - }} + // Built-in modules are initialized through an initcall pointer + // and the identifiers need to be unique. + #[cfg(not(MODULE))] + #[cfg(not(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS))] + #[doc(hidden)] + #[link_section = "{initcall_section}"] + #[used] + pub static __{name}_initcall: extern "C" fn() -> core::ffi::c_int = __{name}_init;
- fn __init() -> core::ffi::c_int {{ - match <{type_} as kernel::Module>::init(&THIS_MODULE) {{ - Ok(m) => {{ - unsafe {{ - __MOD = Some(m); + #[cfg(not(MODULE))] + #[cfg(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS)] + core::arch::global_asm!( + r#".section "{initcall_section}", "a" + __{name}_initcall: + .long __{name}_init - . + .previous + "# + ); + + #[cfg(not(MODULE))] + #[doc(hidden)] + #[no_mangle] + pub extern "C" fn __{name}_init() -> core::ffi::c_int {{ + // SAFETY: This function is inaccessible to the outside due to the double + // module wrapping it. It is called exactly once by the C side via its + // placement above in the initcall section. + unsafe {{ __init() }} + }} + + #[cfg(not(MODULE))] + #[doc(hidden)] + #[no_mangle] + pub extern "C" fn __{name}_exit() {{ + // SAFETY: + // - This function is inaccessible to the outside due to the double + // module wrapping it. It is called exactly once by the C side via its + // unique name, + // - furthermore it is only called after `__{name}_init` has returned `0` + // (which delegates to `__init`). + unsafe {{ __exit() }} + }} + + /// # Safety + /// + /// This function must only be called once. + unsafe fn __init() -> core::ffi::c_int {{ + match <{type_} as kernel::Module>::init(&super::super::THIS_MODULE) {{ + Ok(m) => {{ + // SAFETY: No data race, since `__MOD` can only be accessed by this + // module and there only `__init` and `__exit` access it. These + // functions are only called once and `__exit` cannot be called + // before or during `__init`. + unsafe {{ + __MOD = Some(m); + }} + return 0; + }} + Err(e) => {{ + return e.to_errno(); + }} }} - return 0; }} - Err(e) => {{ - return e.to_errno(); + + /// # Safety + /// + /// This function must + /// - only be called once, + /// - be called after `__init` has been called and returned `0`. + unsafe fn __exit() {{ + // SAFETY: No data race, since `__MOD` can only be accessed by this module + // and there only `__init` and `__exit` access it. These functions are only + // called once and `__init` was already called. + unsafe {{ + // Invokes `drop()` on `__MOD`, which should be used for cleanup. + __MOD = None; + }} }} + + {modinfo} }} }} - - fn __exit() {{ - unsafe {{ - // Invokes `drop()` on `__MOD`, which should be used for cleanup. - __MOD = None; - }} - }} - - {modinfo} ", type_ = info.type_, name = info.name,
On Mon, Apr 29, 2024 at 1:21 PM gregkh@linuxfoundation.org wrote:
Possible dependencies:
7044dcff8301 ("rust: macros: fix soundness issue in `module!` macro") 1b6170ff7a20 ("rust: module: place generated init_module() function in .init.text") 41bdc6decda0 ("btf, scripts: rust: drop is_rust_module.sh")
For 6.1, it is a bit more complex. The following sequence applies cleanly:
git cherry-pick 46384d0990bf99ed8b597e8794ea581e2a647710 git cherry-pick ccc4505454db10402d5284f22d8b7db62e636fc5 git cherry-pick 41bdc6decda074afc4d8f8ba44c69b08d0e9aff6 git cherry-pick 1b6170ff7a203a5e8354f19b7839fe8b897a9c0d git cherry-pick 7044dcff8301b29269016ebd17df27c4736140d2
i.e.
46384d0990bf ("rust: error: Rename to_kernel_errno() -> to_errno()") ccc4505454db ("rust: fix regexp in scripts/is_rust_module.sh") 41bdc6decda0 ("btf, scripts: rust: drop is_rust_module.sh") 1b6170ff7a20 ("rust: module: place generated init_module() function in .init.text") 7044dcff8301 ("rust: macros: fix soundness issue in `module!` macro")
So essentially 2 more commits needed than the dependencies quoted above: the rename (which is easy) and the regexp change so that the drop applies cleanly (which makes the regexp change a no-op). This seems easier/cleaner than crafting a custom commit for that.
I think dropping the script is OK, since it was already redundant from what we discussed last time [1], but I am Cc'ing Andrea and Daniel so that they are aware and in case I may be missing something (note that 6.1 LTS already has commit c1177979af9c ("btf, scripts: Exclude Rust CUs with pahole")).
Thanks!
Cheers, Miguel
[1] https://lore.kernel.org/rust-for-linux/CANiq72k6um58AAydgkzhkmAdd8t1quzeGaPs...
Hi Miguel,
On Wed, May 01, 2024 at 04:25:08PM GMT, Miguel Ojeda wrote:
On Mon, Apr 29, 2024 at 1:21 PM gregkh@linuxfoundation.org wrote:
Possible dependencies:
7044dcff8301 ("rust: macros: fix soundness issue in `module!` macro") 1b6170ff7a20 ("rust: module: place generated init_module() function in .init.text") 41bdc6decda0 ("btf, scripts: rust: drop is_rust_module.sh")
For 6.1, it is a bit more complex. The following sequence applies cleanly:
git cherry-pick 46384d0990bf99ed8b597e8794ea581e2a647710 git cherry-pick ccc4505454db10402d5284f22d8b7db62e636fc5 git cherry-pick 41bdc6decda074afc4d8f8ba44c69b08d0e9aff6 git cherry-pick 1b6170ff7a203a5e8354f19b7839fe8b897a9c0d git cherry-pick 7044dcff8301b29269016ebd17df27c4736140d2
i.e.
46384d0990bf ("rust: error: Rename to_kernel_errno() -> to_errno()") ccc4505454db ("rust: fix regexp in scripts/is_rust_module.sh") 41bdc6decda0 ("btf, scripts: rust: drop is_rust_module.sh") 1b6170ff7a20 ("rust: module: place generated init_module() function in .init.text") 7044dcff8301 ("rust: macros: fix soundness issue in `module!` macro")
So essentially 2 more commits needed than the dependencies quoted above: the rename (which is easy) and the regexp change so that the drop applies cleanly (which makes the regexp change a no-op). This seems easier/cleaner than crafting a custom commit for that.
I think dropping the script is OK, since it was already redundant from what we discussed last time [1], but I am Cc'ing Andrea and Daniel so that they are aware and in case I may be missing something (note that 6.1 LTS already has commit c1177979af9c ("btf, scripts: Exclude Rust CUs with pahole")).
Yep, sounds right to me.
Thanks, Daniel
On Wed, May 1, 2024 at 11:02 PM Daniel Xu dxu@dxuuu.xyz wrote:
Yep, sounds right to me.
Thanks for the confirmation! I appreciate it.
Cheers, Miguel
On Wed, May 01, 2024 at 11:34:15PM +0200, Miguel Ojeda wrote:
On Wed, May 1, 2024 at 11:02 PM Daniel Xu dxu@dxuuu.xyz wrote:
Yep, sounds right to me.
Thanks for the confirmation! I appreciate it.
Great, all now queued up, thanks.
greg k-h
linux-stable-mirror@lists.linaro.org