Re: [PATCH 1/1] rust: module_param: Fix potentially incorrect access of `SetOnce<T>`

[Andreas Hindborg <a.hindborg@kernel.org>]

"Priya Bala Govindasamy" <pgovind2@uci.edu> writes:

> 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.
>
> Fixes: 0b08fc292842 ("rust: introduce module_param module")
> Reported-by: Dylan Zueck <dzueck@uci.edu>
> Assisted-by: LLM
> Signed-off-by: Priya Bala Govindasamy <pgovind2@uci.edu>
> ---
>  rust/kernel/module_param.rs | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/module_param.rs b/rust/kernel/module_param.rs
> index f9a14765a926..7ae87ffe1d6b 100644
> --- a/rust/kernel/module_param.rs
> +++ b/rust/kernel/module_param.rs
> @@ -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

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:

diff --git a/rust/kernel/module_param.rs b/rust/kernel/module_param.rs
index 7ae87ffe1d6b..1a2fcbf4a5b3 100644
--- a/rust/kernel/module_param.rs
+++ b/rust/kernel/module_param.rs
@@ -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