FAILED: patch "[PATCH] rust: macros: fix soundness issue in `module!` macro" failed to apply to 6.1-stable tree

FAILED: patch "[PATCH] rust: macros: fix soundness issue in `module!` macro" failed to apply to 6.1-stable tree

[gregkh]

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/433512583 [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,

Re: FAILED: patch "[PATCH] rust: macros: fix soundness issue in `module!` macro" failed to apply to 6.1-stable tree

[Miguel Ojeda]

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/CANiq72k6um58AAydgkzhkmAdd8t1quzeGaPsR7-pS_ZXYf0-YQ@mail.gmail.com/

Re: FAILED: patch "[PATCH] rust: macros: fix soundness issue in `module!` macro" failed to apply to 6.1-stable tree

[Daniel Xu]

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

Re: FAILED: patch "[PATCH] rust: macros: fix soundness issue in `module!` macro" failed to apply to 6.1-stable tree

[Miguel Ojeda]

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

Re: FAILED: patch "[PATCH] rust: macros: fix soundness issue in `module!` macro" failed to apply to 6.1-stable tree

[Greg KH]

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