rust: module_param: Fix potentially incorrect access of `SetOnce<T>`

Dear Linux kernel maintainers,

We are developing a tool called FerroLens to detect potential unsound behavior in Rust code in the Linux kernel. The tool internally uses an LLM to detect bugs.
We then perform manual analysis to verify these reports. FerroLens reported the following bug in rust/kernel/module_param.rs:

The set_param function casts kernel_param.arg to *const SetOnce<T> But the Rust module macro in rust/macros/module.rs initializes arg with #param_name.as_void_ptr(), and #param_name is a ModuleParamAccess<T>, not a SetOnce<T>.

ModuleParamAccess<T> has default Rust layout but set_param accesses its first field SetOnce<T> assuming it to be at offset 0. This is not guaranteed by Rust and if SetOnce<T> is not at offset 0 it could cause type confusion leading to data corruption.

In particular, if the kernel is built with a randomized layout (e.g with KRUSTFLAGS='-Zrandomize-layout=yes -Zlayout-seed=1'), the first field of ModuleParamAccess<T> may not be at offset 0. This leads to incorrect behavior when loading the rust_minimal module from samples.

$ sudo insmod samples/rust/rust_minimal.ko test_parameter=1 test_bool_parameter=false
insmod: ERROR: could not insert module samples/rust/rust_minimal.ko: File exists

[  854.638251] rust_minimal: `1' invalid for parameter `test_parameter'

Priya Bala Govindasamy (1):
rust: module_param: Fix potentially incorrect layout for
ModuleParamAccess<T>

 rust/kernel/module_param.rs | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

2.34.1

The set_param function casts kernel_param.arg to *const SetOnce<T> But the Rust module macro in rust/macros/module.rs initializes arg with #param_name.as_void_ptr(), and #param_name is a ModuleParamAccess<T>, not a SetOnce<T>.

ModuleParamAccess<T> has default Rust layout but set_param accesses its first field SetOnce<T> assuming it to be at offset 0. This is not guaranteed by Rust and could cause type confusion leading to data corruption.

Fix this by casting kernel_param.arg to ModuleParamAccess<T> and then accessing the value: SetOnce<T> field.

---
 rust/kernel/module_param.rs | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index f9a14765a926..7ae87ffe1d6b 100644
@@ -75,7 +75,9 @@ pub trait ModuleParam: Sized + Copy {
         let new_value = T::try_from_param_arg(arg)?;
 
         // SAFETY: By function safety requirements, this access is safe.
-        let container = unsafe { &*((*param).__bindgen_anon_1.arg.cast::<SetOnce<T>>()) };
+        let param_access = unsafe { &*((*param)
+            .__bindgen_anon_1.arg.cast::<ModuleParamAccess<T>>()) };
+        let container = &param_access.value;
 
         container
             .populate(new_value)

2.34.1

Before it is forgotten:

Without taking a proper look reproducing etc., it is indeed true that Rust's default representation may reorder fields (even without the flags).

Thanks!

Cheers,
Miguel

This is indeed a bug. Nice catch.

I think the primary reason for this error is that the safety comment is lacking. It is also covering two distinct unsafe operations. I think we should rephrase as so:

index 7ae87ffe1d6b..1a2fcbf4a5b3 100644
@@ -74,9 +74,12 @@ pub trait ModuleParam: Sized + Copy {
     crate::error::from_result(|| {
         let new_value = T::try_from_param_arg(arg)?;
 
-        // SAFETY: By function safety requirements, this access is safe.
-        let param_access = unsafe { &*((*param)
-            .__bindgen_anon_1.arg.cast::<ModuleParamAccess<T>>()) };
+        // SAFETY: By function safety requirements, `param` is valid for read.
+        let arg = unsafe { (*param).__bindgen_anon_1.arg };
+        let param_access_ptr = arg.cast::<ModuleParamAccess<T>>();
+        // SAFETY: The `arg` field is initialized with a pointer to a `static ModuleAccessParam` by
+        // the rust module macro. Thus, the pointer is valid for use as a reference.
+        let param_access = unsafe { &*(param_access_ptr) };
         let container = &param_access.value;
 
         container

---

Please also remember to run make rustfmt to format the code properly.

Best regards,
Andreas Hindborg

The set_param function casts kernel_param.arg to *const SetOnce<T> But the Rust module macro in rust/macros/module.rs initializes arg with #param_name.as_void_ptr(), and #param_name is a ModuleParamAccess<T>, not a SetOnce<T>.

ModuleParamAccess<T> has default Rust layout but set_param accesses its first field SetOnce<T> assuming it to be at offset 0. This is not guaranteed by Rust and could cause type confusion leading to data corruption.

Fix this by casting kernel_param.arg to ModuleParamAccess<T> and then accessing the value: SetOnce<T> field.

---
Changes in v2:
- Split unsafe block into two separate unsafe operations
- Add safety comments explaining the unsafe operations

 rust/kernel/module_param.rs | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

index f9a14765a926..1a2fcbf4a5b3 100644
@@ -74,8 +74,13 @@ pub trait ModuleParam: Sized + Copy {
     crate::error::from_result(|| {
         let new_value = T::try_from_param_arg(arg)?;
 
-        // SAFETY: By function safety requirements, this access is safe.
-        let container = unsafe { &*((*param).__bindgen_anon_1.arg.cast::<SetOnce<T>>()) };
+        // SAFETY: By function safety requirements, `param` is valid for read.
+        let arg = unsafe { (*param).__bindgen_anon_1.arg };
+        let param_access_ptr = arg.cast::<ModuleParamAccess<T>>();
+        // SAFETY: The `arg` field is initialized with a pointer to a `static ModuleAccessParam` by
+        // the rust module macro. Thus, the pointer is valid for use as a reference.
+        let param_access = unsafe { &*(param_access_ptr) };
+        let container = &param_access.value;
 
         container
             .populate(new_value)

2.34.1

I've queued the patch (v2) on modules-next for v7.4-rc1.

Thanks,
Petr