Skip to content

Commit 1b729e1

Browse files
BennoLossinBennoLossin
committed
rust: macros: fix soundness issue in module! macro
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: #629 Fixes: 1fbde52 ("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 [ Capitalized comments, avoided newline in non-list SAFETY comments and reworded to add Reported-by and newline. ] Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
1 parent a59391f commit 1b729e1

File tree

1 file changed

+126
-86
lines changed

1 file changed

+126
-86
lines changed

‎rust/macros/module.rs‎

Lines changed: 126 additions & 86 deletions
Original file line numberDiff line numberDiff line change
@@ -199,103 +199,143 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
199199
/// Used by the printing macros, e.g. [`info!`].
200200
const __LOG_PREFIX: &[u8] = b\"{name}\\0\";
201201
202-
/// The \"Rust loadable module\" mark.
203-
//
204-
// This may be best done another way later on, e.g. as a new modinfo
205-
// key or a new section. For the moment, keep it simple.
206-
#[cfg(MODULE)]
207-
#[doc(hidden)]
208-
#[used]
209-
static __IS_RUST_MODULE: () = ();
210-
211-
static mut __MOD: Option<{type_}> = None;
212-
213-
// SAFETY: `__this_module` is constructed by the kernel at load time and will not be
214-
// freed until the module is unloaded.
215-
#[cfg(MODULE)]
216-
static THIS_MODULE: kernel::ThisModule = unsafe {{
217-
kernel::ThisModule::from_ptr(&kernel::bindings::__this_module as *const _ as *mut _)
218-
}};
219-
#[cfg(not(MODULE))]
220-
static THIS_MODULE: kernel::ThisModule = unsafe {{
221-
kernel::ThisModule::from_ptr(core::ptr::null_mut())
222-
}};
223-
224-
// Loadable modules need to export the `{{init,cleanup}}_module` identifiers.
225-
/// # Safety
226-
///
227-
/// This function must not be called after module initialization, because it may be
228-
/// freed after that completes.
229-
#[cfg(MODULE)]
230-
#[doc(hidden)]
231-
#[no_mangle]
232-
#[link_section = \".init.text\"]
233-
pub unsafe extern \"C\" fn init_module() -> core::ffi::c_int {{
234-
__init()
235-
}}
202+
// Double nested modules, since then nobody can access the public items inside.
203+
mod __module_init {{
204+
mod __module_init {{
205+
use super::super::{type_};
206+
207+
/// The \"Rust loadable module\" mark.
208+
//
209+
// This may be best done another way later on, e.g. as a new modinfo
210+
// key or a new section. For the moment, keep it simple.
211+
#[cfg(MODULE)]
212+
#[doc(hidden)]
213+
#[used]
214+
static __IS_RUST_MODULE: () = ();
215+
216+
static mut __MOD: Option<{type_}> = None;
217+
218+
// SAFETY: `__this_module` is constructed by the kernel at load time and will not be
219+
// freed until the module is unloaded.
220+
#[cfg(MODULE)]
221+
static THIS_MODULE: kernel::ThisModule = unsafe {{
222+
kernel::ThisModule::from_ptr(&kernel::bindings::__this_module as *const _ as *mut _)
223+
}};
224+
#[cfg(not(MODULE))]
225+
static THIS_MODULE: kernel::ThisModule = unsafe {{
226+
kernel::ThisModule::from_ptr(core::ptr::null_mut())
227+
}};
228+
229+
// Loadable modules need to export the `{{init,cleanup}}_module` identifiers.
230+
/// # Safety
231+
///
232+
/// This function must not be called after module initialization, because it may be
233+
/// freed after that completes.
234+
#[cfg(MODULE)]
235+
#[doc(hidden)]
236+
#[no_mangle]
237+
#[link_section = \".init.text\"]
238+
pub unsafe extern \"C\" fn init_module() -> core::ffi::c_int {{
239+
// SAFETY: This function is inaccessible to the outside due to the double
240+
// module wrapping it. It is called exactly once by the C side via its
241+
// unique name.
242+
unsafe {{ __init() }}
243+
}}
236244
237-
#[cfg(MODULE)]
238-
#[doc(hidden)]
239-
#[no_mangle]
240-
pub extern \"C\" fn cleanup_module() {{
241-
__exit()
242-
}}
245+
#[cfg(MODULE)]
246+
#[doc(hidden)]
247+
#[no_mangle]
248+
pub extern \"C\" fn cleanup_module() {{
249+
// SAFETY:
250+
// - This function is inaccessible to the outside due to the double
251+
// module wrapping it. It is called exactly once by the C side via its
252+
// unique name,
253+
// - furthermore it is only called after `init_module` has returned `0`
254+
// (which delegates to `__init`).
255+
unsafe {{ __exit() }}
256+
}}
243257
244-
// Built-in modules are initialized through an initcall pointer
245-
// and the identifiers need to be unique.
246-
#[cfg(not(MODULE))]
247-
#[cfg(not(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS))]
248-
#[doc(hidden)]
249-
#[link_section = \"{initcall_section}\"]
250-
#[used]
251-
pub static __{name}_initcall: extern \"C\" fn() -> core::ffi::c_int = __{name}_init;
252-
253-
#[cfg(not(MODULE))]
254-
#[cfg(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS)]
255-
core::arch::global_asm!(
256-
r#\".section \"{initcall_section}\", \"a\"
257-
__{name}_initcall:
258-
.long __{name}_init - .
259-
.previous
260-
\"#
261-
);
258+
// Built-in modules are initialized through an initcall pointer
259+
// and the identifiers need to be unique.
260+
#[cfg(not(MODULE))]
261+
#[cfg(not(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS))]
262+
#[doc(hidden)]
263+
#[link_section = \"{initcall_section}\"]
264+
#[used]
265+
pub static __{name}_initcall: extern \"C\" fn() -> core::ffi::c_int = __{name}_init;
266+
267+
#[cfg(not(MODULE))]
268+
#[cfg(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS)]
269+
core::arch::global_asm!(
270+
r#\".section \"{initcall_section}\", \"a\"
271+
__{name}_initcall:
272+
.long __{name}_init - .
273+
.previous
274+
\"#
275+
);
276+
277+
#[cfg(not(MODULE))]
278+
#[doc(hidden)]
279+
#[no_mangle]
280+
pub extern \"C\" fn __{name}_init() -> core::ffi::c_int {{
281+
// SAFETY: This function is inaccessible to the outside due to the double
282+
// module wrapping it. It is called exactly once by the C side via its
283+
// placement above in the initcall section.
284+
unsafe {{ __init() }}
285+
}}
262286
263-
#[cfg(not(MODULE))]
264-
#[doc(hidden)]
265-
#[no_mangle]
266-
pub extern \"C\" fn __{name}_init() -> core::ffi::c_int {{
267-
__init()
268-
}}
287+
#[cfg(not(MODULE))]
288+
#[doc(hidden)]
289+
#[no_mangle]
290+
pub extern \"C\" fn __{name}_exit() {{
291+
// SAFETY:
292+
// - This function is inaccessible to the outside due to the double
293+
// module wrapping it. It is called exactly once by the C side via its
294+
// unique name,
295+
// - furthermore it is only called after `__{name}_init` has returned `0`
296+
// (which delegates to `__init`).
297+
unsafe {{ __exit() }}
298+
}}
269299
270-
#[cfg(not(MODULE))]
271-
#[doc(hidden)]
272-
#[no_mangle]
273-
pub extern \"C\" fn __{name}_exit() {{
274-
__exit()
275-
}}
300+
/// # Safety
301+
///
302+
/// This function must only be called once.
303+
unsafe fn __init() -> core::ffi::c_int {{
304+
match <{type_} as kernel::Module>::init(&THIS_MODULE) {{
305+
Ok(m) => {{
306+
// SAFETY: No data race, since `__MOD` can only be accessed by this
307+
// module and there only `__init` and `__exit` access it. These
308+
// functions are only called once and `__exit` cannot be called
309+
// before or during `__init`.
310+
unsafe {{
311+
__MOD = Some(m);
312+
}}
313+
return 0;
314+
}}
315+
Err(e) => {{
316+
return e.to_errno();
317+
}}
318+
}}
319+
}}
276320
277-
fn __init() -> core::ffi::c_int {{
278-
match <{type_} as kernel::Module>::init(&THIS_MODULE) {{
279-
Ok(m) => {{
321+
/// # Safety
322+
///
323+
/// This function must
324+
/// - only be called once,
325+
/// - be called after `__init` has been called and returned `0`.
326+
unsafe fn __exit() {{
327+
// SAFETY: No data race, since `__MOD` can only be accessed by this module
328+
// and there only `__init` and `__exit` access it. These functions are only
329+
// called once and `__init` was already called.
280330
unsafe {{
281-
__MOD = Some(m);
331+
// Invokes `drop()` on `__MOD`, which should be used for cleanup.
332+
__MOD = None;
282333
}}
283-
return 0;
284334
}}
285-
Err(e) => {{
286-
return e.to_errno();
287-
}}
288-
}}
289-
}}
290335
291-
fn __exit() {{
292-
unsafe {{
293-
// Invokes `drop()` on `__MOD`, which should be used for cleanup.
294-
__MOD = None;
336+
{modinfo}
295337
}}
296338
}}
297-
298-
{modinfo}
299339
",
300340
type_ = info.type_,
301341
name = info.name,

0 commit comments

Comments
 (0)