From: Boqun Feng <boqun.feng@gmail.com>
To: Benno Lossin <benno.lossin@proton.me>
Cc: "Miguel Ojeda" <ojeda@kernel.org>,
"Alex Gaynor" <alex.gaynor@gmail.com>,
"Wedson Almeida Filho" <wedsonaf@gmail.com>,
"Gary Guo" <gary@garyguo.net>,
"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
"Andreas Hindborg" <a.hindborg@samsung.com>,
"Alice Ryhl" <aliceryhl@google.com>,
"Martin Rodriguez Reboredo" <yakoyoku@gmail.com>,
"Asahi Lina" <lina@asahilina.net>,
"Sumera Priyadarsini" <sylphrenadin@gmail.com>,
"Neal Gompa" <neal@gompa.dev>,
"Thomas Bertschinger" <tahbertschinger@gmail.com>,
"Andrea Righi" <andrea.righi@canonical.com>,
"Matthew Bakhtiari" <dev@mtbk.me>,
"Adam Bratschi-Kaye" <ark.email@gmail.com>,
stable@vger.kernel.org, "Masahiro Yamada" <masahiroy@kernel.org>,
"Wedson Almeida Filho" <wedsonaf@google.com>,
"Finn Behrens" <me@kloenk.dev>,
rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] rust: macros: fix soundness issue in `module!` macro
Date: Mon, 1 Apr 2024 14:10:18 -0700 [thread overview]
Message-ID: <Zgsiumknfshjbi9J@boqun-archlinux> (raw)
In-Reply-To: <20240401185222.12015-1-benno.lossin@proton.me>
On Mon, Apr 01, 2024 at 06:52:50PM +0000, Benno Lossin wrote:
[...]
> + // Double nested modules, since then nobody can access the public items inside.
> + mod __module_init {{
> + mod __module_init {{
> + use super::super::{type_};
> +
> + /// 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)]
> + static THIS_MODULE: kernel::ThisModule = unsafe {{
> + kernel::ThisModule::from_ptr(&kernel::bindings::__this_module as *const _ as *mut _)
While we're at it, probably we want the following as well? I.e. using
`Opaque` and extern block, because __this_module is certainly something
interior mutable and !Unpin.
diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index 293beca0a583..8aa4eed6578c 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -219,7 +219,11 @@ mod __module_init {{
// freed until the module is unloaded.
#[cfg(MODULE)]
static THIS_MODULE: kernel::ThisModule = unsafe {{
- kernel::ThisModule::from_ptr(&kernel::bindings::__this_module as *const _ as *mut _)
+ extern \"C\" {{
+ static __this_module: kernel::types::Opaque<kernel::bindings::module>;
+ }}
+
+ kernel::ThisModule::from_ptr(__this_module.get())
}};
#[cfg(not(MODULE))]
static THIS_MODULE: kernel::ThisModule = unsafe {{
Thoughts?
Note this requires `Opaque::get` to be `const`, which I will send out
shortly, I think it's a good change regardless of the usage here.
Regards,
Boqun
> + }};
> + #[cfg(not(MODULE))]
> + static THIS_MODULE: kernel::ThisModule = unsafe {{
> + 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 {{
> + // 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(MODULE)]
> - #[doc(hidden)]
> - #[no_mangle]
> - pub extern \"C\" fn cleanup_module() {{
> - __exit()
> - }}
> + #[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() }}
> + }}
>
> - // 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;
> -
> - #[cfg(not(MODULE))]
> - #[cfg(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS)]
> - core::arch::global_asm!(
> - r#\".section \"{initcall_section}\", \"a\"
> - __{name}_initcall:
> - .long __{name}_init - .
> - .previous
> - \"#
> - );
> + // 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;
> +
> + #[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}_init() -> core::ffi::c_int {{
> - __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() }}
> + }}
>
> - #[cfg(not(MODULE))]
> - #[doc(hidden)]
> - #[no_mangle]
> - pub extern \"C\" fn __{name}_exit() {{
> - __exit()
> - }}
> + /// # Safety
> + ///
> + /// This function must only be called once.
> + unsafe fn __init() -> core::ffi::c_int {{
> + match <{type_} as kernel::Module>::init(&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();
> + }}
> + }}
> + }}
>
> - fn __init() -> core::ffi::c_int {{
> - match <{type_} as kernel::Module>::init(&THIS_MODULE) {{
> - Ok(m) => {{
> + /// # 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 {{
> - __MOD = Some(m);
> + // Invokes `drop()` on `__MOD`, which should be used for cleanup.
> + __MOD = None;
> }}
> - return 0;
> }}
> - Err(e) => {{
> - return e.to_errno();
> - }}
> - }}
> - }}
>
> - fn __exit() {{
> - unsafe {{
> - // Invokes `drop()` on `__MOD`, which should be used for cleanup.
> - __MOD = None;
> + {modinfo}
> }}
> }}
> -
> - {modinfo}
> ",
> type_ = info.type_,
> name = info.name,
>
> base-commit: 9ffe2a730313f27cebd0859ea856247ac59c576c
> --
> 2.44.0
>
>
next prev parent reply other threads:[~2024-04-01 21:11 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-01 18:52 [PATCH v2] rust: macros: fix soundness issue in `module!` macro Benno Lossin
2024-04-01 19:42 ` Wedson Almeida Filho
2024-04-01 21:10 ` Boqun Feng [this message]
2024-04-01 22:01 ` Benno Lossin
2024-04-01 22:17 ` Boqun Feng
2024-04-02 12:47 ` Benno Lossin
2024-04-07 20:02 ` Miguel Ojeda
2024-04-16 17:07 ` Boqun Feng
2024-04-16 19:36 ` Miguel Ojeda
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Zgsiumknfshjbi9J@boqun-archlinux \
--to=boqun.feng@gmail.com \
--cc=a.hindborg@samsung.com \
--cc=alex.gaynor@gmail.com \
--cc=aliceryhl@google.com \
--cc=andrea.righi@canonical.com \
--cc=ark.email@gmail.com \
--cc=benno.lossin@proton.me \
--cc=bjorn3_gh@protonmail.com \
--cc=dev@mtbk.me \
--cc=gary@garyguo.net \
--cc=lina@asahilina.net \
--cc=linux-kernel@vger.kernel.org \
--cc=masahiroy@kernel.org \
--cc=me@kloenk.dev \
--cc=neal@gompa.dev \
--cc=ojeda@kernel.org \
--cc=rust-for-linux@vger.kernel.org \
--cc=stable@vger.kernel.org \
--cc=sylphrenadin@gmail.com \
--cc=tahbertschinger@gmail.com \
--cc=wedsonaf@gmail.com \
--cc=wedsonaf@google.com \
--cc=yakoyoku@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox