[RFC PATCH] rust: allow Clang-native `RANDSTRUCT` configs

[RFC PATCH] rust: allow Clang-native `RANDSTRUCT` configs

[Miguel Ojeda]

The kernel supports `RANDSTRUCT_FULL` with Clang 16+, and `bindgen`
(which uses `libclang` under the hood) inherits the information, so the
generated bindings look correct.

For instance, running:

    bindgen x.h -- -frandomize-layout-seed=100

with:

    struct S1 {
        int a;
        int b;
    };

    struct S2 {
        int a;
        int b;
    } __attribute__((randomize_layout));

    struct S3 {
        void (*a)(void);
        void (*b)(void);
    };

    struct S4 {
        void (*a)(void);
        void (*b)(void);
    } __attribute__((no_randomize_layout));

may swap `S2`'s and `S3`'s members, but not `S1`'s nor `S4`'s:

    pub struct S1 {
        pub a: ::std::os::raw::c_int,
        pub b: ::std::os::raw::c_int,
    }

    pub struct S2 {
        pub b: ::std::os::raw::c_int,
        pub a: ::std::os::raw::c_int,
    }

    pub struct S3 {
        pub b: ::std::option::Option<unsafe extern "C" fn()>,
        pub a: ::std::option::Option<unsafe extern "C" fn()>,
    }

    pub struct S4 {
        pub a: ::std::option::Option<unsafe extern "C" fn()>,
        pub b: ::std::option::Option<unsafe extern "C" fn()>,
    }

Thus allow those configurations by requiring a Clang compiler to use
`RANDSTRUCT`. In addition, remove the `!GCC_PLUGIN_RANDSTRUCT` check
since it is not needed.

A simpler alternative would be to remove the `!RANDSTRUCT` check (keeping
the `!GCC_PLUGIN_RANDSTRUCT` one). However, if in the future GCC starts
supporting `RANDSTRUCT` natively, it would be likely that it would not
work unless GCC and Clang agree on the exact semantics of the flag. And,
as far as I can see, so far the randomization in Clang does not seem to be
guaranteed to remain stable across versions or platforms, i.e. only for a
given compiler Clang binary, given it is not documented and that LLVM's
`HEAD` has the revert in place for the expected field names in the test
(LLVM commit 8dbc6b560055 ("Revert "[randstruct] Check final randomized
layout ordering"")) [1][2]. And the GCC plugin definitely does not match,
e.g. its RNG is different (`std::mt19937` vs Bob Jenkins').

And given it is not supposed to be guaranteed to remain stable across
versions, it is a good idea to match the Clang and `bindgen`'s
`libclang` versions -- we already have a warning for that in
`scripts/rust_is_available.sh`. In the future, it would also be good to
have a build test to double-check layouts do actually match (but that
is true regardless of this particular feature).

Finally, make a required small change to take into account the anonymous
struct used in `randomized_struct_fields_*` in `struct task_struct`.

Cc: Aaron Ballman <aaron@aaronballman.com>
Cc: Bill Wendling <isanbard@gmail.com>
Cc: Cole Nixon <nixontcole@gmail.com>
Cc: Connor Kuehl <cipkuehl@gmail.com>
Cc: Fangrui Song <i@maskray.me>
Cc: James Foster <jafosterja@gmail.com>
Cc: Jeff Takahashi <jeffrey.takahashi@gmail.com>
Cc: Jordan Cantrell <jordan.cantrell@mail.com>
Cc: Justin Stitt <justinstitt@google.com>
Cc: Matthew Maurer <mmaurer@google.com>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Nikk Forbus <nicholas.forbus@gmail.com>
Cc: Qing Zhao <qing.zhao@oracle.com>
Cc: Sami Tolvanen <samitolvanen@google.com>
Cc: Tim Pugh <nwtpugh@gmail.com>
Link: https://reviews.llvm.org/D121556
Link: https://github.com/llvm/llvm-project/commit/8dbc6b560055ff5068ff45b550482ba62c36b5a5 [1]
Link: https://reviews.llvm.org/D124199 [2]
Reviewed-by: Kees Cook <kees@kernel.org>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---
I tried the Clang's native `RANDSTRUCT` support with Rust, and
apparently it seems to just work.

I checked first that `bindgen` was indeed randomizing the structs in
userspace (i.e. `libclang` seems to provide the right information),
then I checked our generated bindings were indeed also randomized, and
QEMU-booted a couple times the kernel and our (few) KUnit tests pass.

I also printed the offset of a couple of the `task_struct` randomized
members from both C and Rust independently (which I thought could be a
non-trivial case since `bindgen` transforms the anonymous struct into
another type), and they seem to match as well from a couple tries.

So a patch like this one may just work already, at least in some cases.
But a fair amount of testing is probably needed for this one, so if you
have a workload and some time, please give it a go!

 init/Kconfig        |  3 +--
 rust/kernel/task.rs | 22 ++++++++++++++++++++--
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index 530a382ee0fe..d2a04c3c86ab 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1943,8 +1943,7 @@ config RUST
 	depends on HAVE_RUST
 	depends on RUST_IS_AVAILABLE
 	depends on !MODVERSIONS
-	depends on !GCC_PLUGIN_RANDSTRUCT
-	depends on !RANDSTRUCT
+	depends on !RANDSTRUCT || CC_IS_CLANG
 	depends on !DEBUG_INFO_BTF || PAHOLE_HAS_LANG_EXCLUDE
 	depends on !CFI_CLANG || RUSTC_VERSION >= 107900 && HAVE_CFI_ICALL_NORMALIZE_INTEGERS
 	select CFI_ICALL_NORMALIZE_INTEGERS if CFI_CLANG
diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
index 5bce090a3869..1012bb772f56 100644
--- a/rust/kernel/task.rs
+++ b/rust/kernel/task.rs
@@ -129,7 +129,16 @@ fn deref(&self) -> &Self::Target {
     pub fn group_leader(&self) -> &Task {
         // SAFETY: By the type invariant, we know that `self.0` is a valid task. Valid tasks always
         // have a valid `group_leader`.
-        let ptr = unsafe { *ptr::addr_of!((*self.0.get()).group_leader) };
+        let ptr = unsafe {
+            #[cfg(not(CONFIG_RANDSTRUCT))]
+            {
+                *ptr::addr_of!((*self.0.get()).group_leader)
+            }
+            #[cfg(CONFIG_RANDSTRUCT)]
+            {
+                *ptr::addr_of!((*self.0.get()).__bindgen_anon_1.group_leader)
+            }
+        };

         // SAFETY: The lifetime of the returned task reference is tied to the lifetime of `self`,
         // and given that a task has a reference to its group leader, we know it must be valid for
@@ -141,7 +150,16 @@ pub fn group_leader(&self) -> &Task {
     pub fn pid(&self) -> Pid {
         // SAFETY: By the type invariant, we know that `self.0` is a valid task. Valid tasks always
         // have a valid pid.
-        unsafe { *ptr::addr_of!((*self.0.get()).pid) }
+        unsafe {
+            #[cfg(not(CONFIG_RANDSTRUCT))]
+            {
+                *ptr::addr_of!((*self.0.get()).pid)
+            }
+            #[cfg(CONFIG_RANDSTRUCT)]
+            {
+                *ptr::addr_of!((*self.0.get()).__bindgen_anon_1.pid)
+            }
+        }
     }

     /// Determines whether the given task has pending signals.

base-commit: b2603f8ac8217bc59f5c7f248ac248423b9b99cb
--
2.47.0

Re: [RFC PATCH] rust: allow Clang-native `RANDSTRUCT` configs

[Miguel Ojeda]

On Tue, Nov 19, 2024 at 7:57 PM Miguel Ojeda <ojeda@kernel.org> wrote:
>
> So a patch like this one may just work already, at least in some cases.
> But a fair amount of testing is probably needed for this one, so if you
> have a workload and some time, please give it a go!

I may pick this very early next cycle so that we can get some testing
during the -rc's, and just drop it if something breaks for anyone.

But any other testing beforehand would be still very welcome...

Thanks!

Cheers,
Miguel

Re: [RFC PATCH] rust: allow Clang-native `RANDSTRUCT` configs

[Miguel Ojeda]

On Thu, Jan 9, 2025 at 3:51 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> I may pick this very early next cycle so that we can get some testing
> during the -rc's, and just drop it if something breaks for anyone.
>
> But any other testing beforehand would be still very welcome...

I ended up waiting on this one since I think Andreas would be able to
try it with some actual workload (the nice benchmarks he has). Anyway,
it will go in sooner or later.

On top of that, backlinking the seed discussion since I will need to
update this patch:

    https://lore.kernel.org/rust-for-linux/CANiq72mwYbg_L8u9NEfRD0Fp9E_zhCi=t=U2jyjsZe_Ls306hw@mail.gmail.com/

Cheers,
Miguel

Re: [RFC PATCH] rust: allow Clang-native `RANDSTRUCT` configs

[Andreas Hindborg]

"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:

> On Thu, Jan 9, 2025 at 3:51 PM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
>>
>> I may pick this very early next cycle so that we can get some testing
>> during the -rc's, and just drop it if something breaks for anyone.
>>
>> But any other testing beforehand would be still very welcome...
>
> I ended up waiting on this one since I think Andreas would be able to
> try it with some actual workload (the nice benchmarks he has). Anyway,
> it will go in sooner or later.

It is on my list, sorry for not getting to that yet.


Best regards,
Andreas Hindborg

Re: [RFC PATCH] rust: allow Clang-native `RANDSTRUCT` configs

[Miguel Ojeda]

On Tue, Apr 29, 2025 at 3:40 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> It is on my list, sorry for not getting to that yet.

No worries at all, and thanks again if you end up having cycles to test it.

(It does not necessarily need to be Andreas, of course, others are
welcome to test too ;)

Cheers,
Miguel

Re: [RFC PATCH] rust: allow Clang-native `RANDSTRUCT` configs

[Kees Cook]

On Tue, Nov 19, 2024 at 07:57:47PM +0100, Miguel Ojeda wrote:
> The kernel supports `RANDSTRUCT_FULL` with Clang 16+, and `bindgen`
> (which uses `libclang` under the hood) inherits the information, so the
> generated bindings look correct.
> 
> For instance, running:
> 
>     bindgen x.h -- -frandomize-layout-seed=100
> 
> with:
> 
>     struct S1 {
>         int a;
>         int b;
>     };
> 
>     struct S2 {
>         int a;
>         int b;
>     } __attribute__((randomize_layout));
> 
>     struct S3 {
>         void (*a)(void);
>         void (*b)(void);
>     };
> 
>     struct S4 {
>         void (*a)(void);
>         void (*b)(void);
>     } __attribute__((no_randomize_layout));
> 
> may swap `S2`'s and `S3`'s members, but not `S1`'s nor `S4`'s:
> 
>     pub struct S1 {
>         pub a: ::std::os::raw::c_int,
>         pub b: ::std::os::raw::c_int,
>     }
> 
>     pub struct S2 {
>         pub b: ::std::os::raw::c_int,
>         pub a: ::std::os::raw::c_int,
>     }
> 
>     pub struct S3 {
>         pub b: ::std::option::Option<unsafe extern "C" fn()>,
>         pub a: ::std::option::Option<unsafe extern "C" fn()>,
>     }
> 
>     pub struct S4 {
>         pub a: ::std::option::Option<unsafe extern "C" fn()>,
>         pub b: ::std::option::Option<unsafe extern "C" fn()>,
>     }
> 
> Thus allow those configurations by requiring a Clang compiler to use
> `RANDSTRUCT`. In addition, remove the `!GCC_PLUGIN_RANDSTRUCT` check
> since it is not needed.
> 
> A simpler alternative would be to remove the `!RANDSTRUCT` check (keeping
> the `!GCC_PLUGIN_RANDSTRUCT` one). However, if in the future GCC starts
> supporting `RANDSTRUCT` natively, it would be likely that it would not
> work unless GCC and Clang agree on the exact semantics of the flag. And,
> as far as I can see, so far the randomization in Clang does not seem to be
> guaranteed to remain stable across versions or platforms, i.e. only for a
> given compiler Clang binary, given it is not documented and that LLVM's
> `HEAD` has the revert in place for the expected field names in the test
> (LLVM commit 8dbc6b560055 ("Revert "[randstruct] Check final randomized
> layout ordering"")) [1][2]. And the GCC plugin definitely does not match,
> e.g. its RNG is different (`std::mt19937` vs Bob Jenkins').
> 
> And given it is not supposed to be guaranteed to remain stable across
> versions, it is a good idea to match the Clang and `bindgen`'s
> `libclang` versions -- we already have a warning for that in
> `scripts/rust_is_available.sh`. In the future, it would also be good to
> have a build test to double-check layouts do actually match (but that
> is true regardless of this particular feature).
> 
> Finally, make a required small change to take into account the anonymous
> struct used in `randomized_struct_fields_*` in `struct task_struct`.
> 
> Cc: Aaron Ballman <aaron@aaronballman.com>
> Cc: Bill Wendling <isanbard@gmail.com>
> Cc: Cole Nixon <nixontcole@gmail.com>
> Cc: Connor Kuehl <cipkuehl@gmail.com>
> Cc: Fangrui Song <i@maskray.me>
> Cc: James Foster <jafosterja@gmail.com>
> Cc: Jeff Takahashi <jeffrey.takahashi@gmail.com>
> Cc: Jordan Cantrell <jordan.cantrell@mail.com>
> Cc: Justin Stitt <justinstitt@google.com>
> Cc: Matthew Maurer <mmaurer@google.com>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: Nikk Forbus <nicholas.forbus@gmail.com>
> Cc: Qing Zhao <qing.zhao@oracle.com>
> Cc: Sami Tolvanen <samitolvanen@google.com>
> Cc: Tim Pugh <nwtpugh@gmail.com>
> Link: https://reviews.llvm.org/D121556
> Link: https://github.com/llvm/llvm-project/commit/8dbc6b560055ff5068ff45b550482ba62c36b5a5 [1]
> Link: https://reviews.llvm.org/D124199 [2]
> Reviewed-by: Kees Cook <kees@kernel.org>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

I played with this again, and yes it appears to be working. I think,
however, I would like to make the anon struct universal. What do you
think of this?

diff --git a/init/Kconfig b/init/Kconfig
index cab3ad28ca49..839bb5a6b187 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -2090,8 +2090,7 @@ config RUST
 	depends on RUST_IS_AVAILABLE
 	select EXTENDED_MODVERSIONS if MODVERSIONS
 	depends on !MODVERSIONS || GENDWARFKSYMS
-	depends on !GCC_PLUGIN_RANDSTRUCT
-	depends on !RANDSTRUCT
+	depends on !RANDSTRUCT || CC_IS_CLANG
 	depends on !DEBUG_INFO_BTF || (PAHOLE_HAS_LANG_EXCLUDE && !LTO)
 	depends on !CFI || HAVE_CFI_ICALL_NORMALIZE_INTEGERS_RUSTC
 	select CFI_ICALL_NORMALIZE_INTEGERS if CFI
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 1414be493738..ed17db287db1 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -437,14 +437,9 @@ struct ftrace_likely_data {
 #if defined(RANDSTRUCT) && !defined(__CHECKER__)
 # define __randomize_layout __designated_init __attribute__((randomize_layout))
 # define __no_randomize_layout __attribute__((no_randomize_layout))
-/* This anon struct can add padding, so only enable it under randstruct. */
-# define randomized_struct_fields_start	struct {
-# define randomized_struct_fields_end	} __randomize_layout;
 #else
 # define __randomize_layout __designated_init
 # define __no_randomize_layout
-# define randomized_struct_fields_start
-# define randomized_struct_fields_end
 #endif
 
 #ifndef __no_kstack_erase
diff --git a/include/linux/sched.h b/include/linux/sched.h
index b469878de25c..7da987ea7f58 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -833,7 +833,7 @@ struct task_struct {
 	 * This begins the randomizable portion of task_struct. Only
 	 * scheduling-critical items should be added above here.
 	 */
-	randomized_struct_fields_start
+	struct {
 
 	void				*stack;
 	refcount_t			usage;
@@ -1664,7 +1664,7 @@ struct task_struct {
 	 * New fields for task_struct should be added above here, so that
 	 * they are included in the randomized portion of task_struct.
 	 */
-	randomized_struct_fields_end
+	} __randomize_layout;
 } __attribute__ ((aligned (64)));
 
 #ifdef CONFIG_SCHED_PROXY_EXEC
diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
index 49fad6de0674..c52552272495 100644
--- a/rust/kernel/task.rs
+++ b/rust/kernel/task.rs
@@ -208,7 +208,7 @@ pub fn as_ptr(&self) -> *mut bindings::task_struct {
     pub fn group_leader(&self) -> &Task {
         // SAFETY: The group leader of a task never changes after initialization, so reading this
         // field is not a data race.
-        let ptr = unsafe { *ptr::addr_of!((*self.as_ptr()).group_leader) };
+        let ptr = unsafe { *ptr::addr_of!((*self.as_ptr()).__bindgen_anon_1.group_leader) };
 
         // SAFETY: The lifetime of the returned task reference is tied to the lifetime of `self`,
         // and given that a task has a reference to its group leader, we know it must be valid for
@@ -220,7 +220,7 @@ pub fn group_leader(&self) -> &Task {
     pub fn pid(&self) -> Pid {
         // SAFETY: The pid of a task never changes after initialization, so reading this field is
         // not a data race.
-        unsafe { *ptr::addr_of!((*self.as_ptr()).pid) }
+        unsafe { *ptr::addr_of!((*self.as_ptr()).__bindgen_anon_1.pid) }
     }
 
     /// Returns the UID of the given task.
@@ -291,7 +291,7 @@ impl CurrentTask {
     pub fn mm(&self) -> Option<&MmWithUser> {
         // SAFETY: The `mm` field of `current` is not modified from other threads, so reading it is
         // not a data race.
-        let mm = unsafe { (*self.as_ptr()).mm };
+        let mm = unsafe { (*self.as_ptr()).__bindgen_anon_1.mm };
 
         if mm.is_null() {
             return None;

-- 
Kees Cook