| Message ID | 1495829844-69341-20-git-send-email-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 26, 2017 at 1:17 PM, Kees Cook <keescook@chromium.org> wrote: > This marks most of the layout of task_struct as randomizable, but leaves > thread_info and scheduler state untouched at the start, and thread_struct > untouched at the end. I think you want to abstract this out somehow, because this is both ugly and bad: > + /* This begins the randomizable portion of task_struct... */ > +#if GCC_VERSION >= 40600 > + struct { > +#endif when you could instead just introduce something like #if GCC_VERSION >= 40600 #define randomized_struct_fields_start struct { #define randomized_struct_fields_end } __randomize_layout; #else #define randomized_struct_fields_start #define randomized_struct_fields_end #endif and then this pattern is (a) more-or-less self-documenting (b) usable in other places too. (c) maybe some future compiler wants that struct field to have some "randomize-me attribute" or something Hmm?
On Fri, May 26, 2017 at 1:23 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Fri, May 26, 2017 at 1:17 PM, Kees Cook <keescook@chromium.org> wrote: >> This marks most of the layout of task_struct as randomizable, but leaves >> thread_info and scheduler state untouched at the start, and thread_struct >> untouched at the end. > > I think you want to abstract this out somehow, because this is both > ugly and bad: > >> + /* This begins the randomizable portion of task_struct... */ >> +#if GCC_VERSION >= 40600 >> + struct { >> +#endif > > when you could instead just introduce something like > > #if GCC_VERSION >= 40600 > #define randomized_struct_fields_start struct { > #define randomized_struct_fields_end } __randomize_layout; > #else > #define randomized_struct_fields_start > #define randomized_struct_fields_end > #endif > > and then this pattern is > (a) more-or-less self-documenting > (b) usable in other places too. > (c) maybe some future compiler wants that struct field to have some > "randomize-me attribute" or something > > Hmm? There were so many options and they all seems weird for various reason. :) I'll use your idea, it looks much cleaner, thanks! -Kees
diff --git a/include/linux/sched.h b/include/linux/sched.h index b1298ad4da63..a9f7f957169c 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -490,6 +490,12 @@ struct task_struct { #endif /* -1 unrunnable, 0 runnable, >0 stopped: */ volatile long state; + + /* This begins the randomizable portion of task_struct... */ +#if GCC_VERSION >= 40600 + struct { +#endif + void *stack; atomic_t usage; /* Per task flags (PF_*), defined further below: */ @@ -1052,6 +1058,14 @@ struct task_struct { /* Used by LSM modules for access restriction: */ void *security; #endif + + /* + * New fields for task_struct should be added above here. + */ +#if GCC_VERSION >= 40600 + } __randomize_layout; +#endif + /* CPU-specific state of this task: */ struct thread_struct thread;
This marks most of the layout of task_struct as randomizable, but leaves thread_info and scheduler state untouched at the start, and thread_struct untouched at the end. Other parts of the kernel use unnamed structures, but the 0-day builder using gcc-4.4 blows up on static initializers. Officially, it's documented as only working on gcc 4.6 and later, which further confuses me: https://gcc.gnu.org/wiki/C11Status The structure layout randomization already requires gcc 4.7, but instead of depending on the plugin being enabled, just check the gcc versions for wider build testing. (But I'd rather find a way to avoid the #ifdef entirely.) One question about formatting remains: should this patch indent all the randomized fields, due to the added unnamed struct, which would make this patch white-space huge, or should I leave the indentation level alone, to avoid massive churn? I opted for making the patch more readable, but can easily do the indentation here or as a separate patch. It's not obvious to me what is the least invasive change to make... Cc: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Kees Cook <keescook@chromium.org> --- include/linux/sched.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+)