On Wed, 27 Mar 2024 at 13:04, Benno Lossin <benno.lossin@xxxxxxxxx> 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@xxxxxxxxxxxxxxx > Closes: https://github.com/Rust-for-Linux/linux/issues/629 > Fixes: 1fbde52bde73 ("rust: add `macros` crate") > Signed-off-by: Benno Lossin <benno.lossin@xxxxxxxxx> > --- > 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 > >
