Received-SPF: pass (google.com: domain of linux-kernel+bounces-147280-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Feedback-ID: iad51458e:Fastmail
Date: Tue, 16 Apr 2024 10:07:24 -0700
From: Boqun Feng <boqun.feng@gmail.com>
To: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Cc: Benno Lossin <benno.lossin@proton.me>, Miguel Ojeda <ojeda@kernel.org>,
	Alex Gaynor <alex.gaynor@gmail.com>,
	Wedson Almeida Filho <wedsonaf@gmail.com>,
	Gary Guo <gary@garyguo.net>,
	=?iso-8859-1?Q?Bj=F6rn?= 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
Message-ID: <Zh6wTDoMgvjJZ7T9@boqun-archlinux>
References: <20240401185222.12015-1-benno.lossin@proton.me>
 <CANiq72=M0L+RG6v701ThedXgYj4SUgotx-BcVoWbMxOcKY5--w@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CANiq72=M0L+RG6v701ThedXgYj4SUgotx-BcVoWbMxOcKY5--w@mail.gmail.com>

On Sun, Apr 07, 2024 at 10:02:35PM +0200, Miguel Ojeda wrote:
> On Mon, Apr 1, 2024 at 8:53 PM Benno Lossin <benno.lossin@proton.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@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>
> 
> [ Capitalized comments, avoided newline in non-list SAFETY comments
>   and reworded to add Reported-by and newline. ]
> 
> Applied to `rust-fixes` -- thanks everyone!
> 

As reported by Dirk Behme:

	https://rust-for-linux.zulipchat.com/#narrow/stream/291565-Help/topic/How.20to.20use.20THIS_MODULE.20with.20.22.20rust.3A.20macros.3A.20fix.20soundness.20.2E.22/near/433512583

The following is needed to allow modules using `THIS_MODULE` as a static
variable. That being said, maybe we can merge this patch as it is, since
it doesn't break mainline, and the following change can be done in a
separate patch.

Regards,
Boqun

----------------------------->8
diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index 293beca0a583..0664b957a70a 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -199,6 +199,17 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
             /// Used by the printing macros, e.g. [`info!`].
             const __LOG_PREFIX: &[u8] = b\"{name}\\0\";
 
+            // 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())
+            }};
+
             // Double nested modules, since then nobody can access the public items inside.
             mod __module_init {{
                 mod __module_init {{
@@ -215,17 +226,6 @@ mod __module_init {{
 
                     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
                     ///
@@ -301,7 +301,7 @@ mod __module_init {{
                     ///
                     /// This function must only be called once.
                     unsafe fn __init() -> core::ffi::c_int {{
-                        match <{type_} as kernel::Module>::init(&THIS_MODULE) {{
+                        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