Received-SPF: pass (google.com: domain of linux-kernel+bounces-126962-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Mon, 01 Apr 2024 18:52:50 +0000
To: Miguel Ojeda <ojeda@kernel.org>, Alex Gaynor <alex.gaynor@gmail.com>, Wedson Almeida Filho <wedsonaf@gmail.com>, Boqun Feng <boqun.feng@gmail.com>, Gary Guo <gary@garyguo.net>, =?utf-8?Q?Bj=C3=B6rn_Roy_Baron?= <bjorn3_gh@protonmail.com>, Benno Lossin <benno.lossin@proton.me>, 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>
From: Benno Lossin <benno.lossin@proton.me>
Cc: 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: [PATCH v2] rust: macros: fix soundness issue in `module!` macro
Message-ID: <20240401185222.12015-1-benno.lossin@proton.me>
Feedback-ID: 71780778:user:proton
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

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
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>
---
v1: https://lore.kernel.org/rust-for-linux/20240327160346.22442-1-benno.los=
sin@proton.me/
v1 -> v2:
- wrapped `__init` and `__exit` calls in `unsafe` blocks and added
  SAFETY comments,
- fixed safety requirement on `__exit` and `__init`,
- rebased onto rust-next.

 rust/macros/module.rs | 213 +++++++++++++++++++++++++-----------------
 1 file changed, 127 insertions(+), 86 deletions(-)

diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index 27979e582e4b..293beca0a583 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -199,103 +199,144 @@ pub(crate) fn module(ts: TokenStream) -> TokenStrea=
m {
             /// Used by the printing macros, e.g. [`info!`].
             const __LOG_PREFIX: &[u8] =3D b\"{name}\\0\";
=20
-            /// The \"Rust loadable module\" mark.
-            //
-            // This may be best done another way later on, e.g. as a new m=
odinfo
-            // key or a new section. For the moment, keep it simple.
-            #[cfg(MODULE)]
-            #[doc(hidden)]
-            #[used]
-            static __IS_RUST_MODULE: () =3D ();
-
-            static mut __MOD: Option<{type_}> =3D None;
-
-            // SAFETY: `__this_module` is constructed by the kernel at loa=
d time and will not be
-            // freed until the module is unloaded.
-            #[cfg(MODULE)]
-            static THIS_MODULE: kernel::ThisModule =3D unsafe {{
-                kernel::ThisModule::from_ptr(&kernel::bindings::__this_mod=
ule as *const _ as *mut _)
-            }};
-            #[cfg(not(MODULE))]
-            static THIS_MODULE: kernel::ThisModule =3D unsafe {{
-                kernel::ThisModule::from_ptr(core::ptr::null_mut())
-            }};
-
-            // Loadable modules need to export the `{{init,cleanup}}_modul=
e` identifiers.
-            /// # Safety
-            ///
-            /// This function must not be called after module initializati=
on, because it may be
-            /// freed after that completes.
-            #[cfg(MODULE)]
-            #[doc(hidden)]
-            #[no_mangle]
-            #[link_section =3D \".init.text\"]
-            pub unsafe extern \"C\" fn init_module() -> core::ffi::c_int {=
{
-                __init()
-            }}
+            // Double nested modules, since then nobody can access the pub=
lic 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 simpl=
e.
+                    #[cfg(MODULE)]
+                    #[doc(hidden)]
+                    #[used]
+                    static __IS_RUST_MODULE: () =3D ();
+
+                    static mut __MOD: Option<{type_}> =3D None;
+
+                    // SAFETY: `__this_module` is constructed by the kerne=
l at load time and will not be
+                    // freed until the module is unloaded.
+                    #[cfg(MODULE)]
+                    static THIS_MODULE: kernel::ThisModule =3D unsafe {{
+                        kernel::ThisModule::from_ptr(&kernel::bindings::__=
this_module as *const _ as *mut _)
+                    }};
+                    #[cfg(not(MODULE))]
+                    static THIS_MODULE: kernel::ThisModule =3D 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 init=
ialization, because it may be
+                    /// freed after that completes.
+                    #[cfg(MODULE)]
+                    #[doc(hidden)]
+                    #[no_mangle]
+                    #[link_section =3D \".init.text\"]
+                    pub unsafe extern \"C\" fn init_module() -> core::ffi:=
:c_int {{
+                        // SAFETY: this function is inaccessible to the ou=
tside due to the double
+                        // module wrapping it. It is called exactly once b=
y the C side via its
+                        // unique name.
+                        unsafe {{ __init() }}
+                    }}
=20
-            #[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_mod=
ule` has returned `0`
+                        //   (which delegates to `__init`).
+                        unsafe {{ __exit() }}
+                    }}
=20
-            // Built-in modules are initialized through an initcall pointe=
r
-            // and the identifiers need to be unique.
-            #[cfg(not(MODULE))]
-            #[cfg(not(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS))]
-            #[doc(hidden)]
-            #[link_section =3D \"{initcall_section}\"]
-            #[used]
-            pub static __{name}_initcall: extern \"C\" fn() -> core::ffi::=
c_int =3D __{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 initcal=
l pointer
+                    // and the identifiers need to be unique.
+                    #[cfg(not(MODULE))]
+                    #[cfg(not(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS))]
+                    #[doc(hidden)]
+                    #[link_section =3D \"{initcall_section}\"]
+                    #[used]
+                    pub static __{name}_initcall: extern \"C\" fn() -> cor=
e::ffi::c_int =3D __{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_in=
t {{
+                        // SAFETY: this function is inaccessible to the ou=
tside due to the double
+                        // module wrapping it. It is called exactly once b=
y the C side via its
+                        // placement above in the initcall section.
+                        unsafe {{ __init() }}
+                    }}
=20
-            #[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() }}
+                    }}
=20
-            #[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_MODU=
LE) {{
+                            Ok(m) =3D> {{
+                                // 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 call=
ed before or during `__init`.
+                                unsafe {{
+                                    __MOD =3D Some(m);
+                                }}
+                                return 0;
+                            }}
+                            Err(e) =3D> {{
+                                return e.to_errno();
+                            }}
+                        }}
+                    }}
=20
-            fn __init() -> core::ffi::c_int {{
-                match <{type_} as kernel::Module>::init(&THIS_MODULE) {{
-                    Ok(m) =3D> {{
+                    /// # Safety
+                    ///
+                    /// This function must
+                    /// - only be called once,
+                    /// - be called after `__init` has been called and ret=
urned `0`.
+                    unsafe fn __exit() {{
+                        // SAFETY:
+                        // no data race, since `__MOD` can only be accesse=
d by this module and there
+                        // only `__init` and `__exit` access it. These fun=
ctions are only called once
+                        // and `__init` was already called.
                         unsafe {{
-                            __MOD =3D Some(m);
+                            // Invokes `drop()` on `__MOD`, which should b=
e used for cleanup.
+                            __MOD =3D None;
                         }}
-                        return 0;
                     }}
-                    Err(e) =3D> {{
-                        return e.to_errno();
-                    }}
-                }}
-            }}
=20
-            fn __exit() {{
-                unsafe {{
-                    // Invokes `drop()` on `__MOD`, which should be used f=
or cleanup.
-                    __MOD =3D None;
+                    {modinfo}
                 }}
             }}
-
-            {modinfo}
         ",
         type_ =3D info.type_,
         name =3D info.name,

base-commit: 9ffe2a730313f27cebd0859ea856247ac59c576c
--=20
2.44.0