| 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: <CANeycqp0o-HKBx6nuGCy9DD6mAwoGWzTR6bm5ceajsUhKcZuQg@mail.gmail.com>
Date: Sat, 30 Mar 2024 22:00:13 -0300
From: Wedson Almeida Filho <wedsonaf@...il.com>
To: Benno Lossin <benno.lossin@...ton.me>
Cc: 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>,
Andreas Hindborg <a.hindborg@...sung.com>, Alice Ryhl <aliceryhl@...gle.com>,
Martin Rodriguez Reboredo <yakoyoku@...il.com>, Asahi Lina <lina@...hilina.net>,
Eric Curtin <ecurtin@...hat.com>, Neal Gompa <neal@...pa.dev>,
Thomas Bertschinger <tahbertschinger@...il.com>, Andrea Righi <andrea.righi@...onical.com>,
Sumera Priyadarsini <sylphrenadin@...il.com>, Finn Behrens <me@...enk.dev>,
Adam Bratschi-Kaye <ark.email@...il.com>, stable@...r.kernel.org, Daniel Xu <dxu@...uu.xyz>,
rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rust: macros: fix soundness issue in `module!` macro
On Wed, 27 Mar 2024 at 13:04, Benno Lossin <benno.lossin@...ton.me> wrote:
>
> 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@...r.kernel.org
> Closes: https://github.com/Rust-for-Linux/linux/issues/629
> Fixes: 1fbde52bde73 ("rust: add `macros` crate")
> Signed-off-by: Benno Lossin <benno.lossin@...ton.me>
> ---
> This patch is best viewed with `git show --ignore-space-change`, since I
> also adjusted the indentation.
>
> rust/macros/module.rs | 198 ++++++++++++++++++++++++------------------
> 1 file changed, 112 insertions(+), 86 deletions(-)
>
> diff --git a/rust/macros/module.rs b/rust/macros/module.rs
> index 27979e582e4b..16c4921a08f2 100644
> --- a/rust/macros/module.rs
> +++ b/rust/macros/module.rs
> @@ -199,103 +199,129 @@ 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)]
> - static THIS_MODULE: kernel::ThisModule = unsafe {{
> - kernel::ThisModule::from_ptr(&kernel::bindings::__this_module as *const _ as *mut _)
> - }};
> - #[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 {{
> - __init()
> - }}
> + // 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 _)
> + }};
> + #[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 {{
> + __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() {{
> + __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 {{
> + __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() {{
> + __exit()
> + }}
>
> - #[cfg(not(MODULE))]
> - #[doc(hidden)]
> - #[no_mangle]
> - pub extern \"C\" fn __{name}_exit() {{
> - __exit()
> - }}
> + /// # Safety
> + ///
> + /// This function must
> + /// - only be called once,
> + /// - not be called concurrently with `__exit`.
I don't think the second item is needed here, it really is a
requirement on `__exit`.
> + 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`,
> + /// - not be called concurrently with `__init`.
The second item is incomplete: it must be called after `__init` *succeeds*.
With that added (which is a different precondition), I think the third
item can be dropped because if you have to wait to see whether
`__init` succeeded or failed before you can call `__exit`, then
certainly you cannot call it concurrently with `__init`.
> + 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: 4cece764965020c22cff7665b18a012006359095
> --
> 2.44.0
>
>
Powered by blists - more mailing lists