RE: [RFC PATCH] x86/entry/64: randomize kernel stack offset upon syscall

["Reshetova, Elena" <elena.reshetova@intel.com>]

 On Fri, Mar 29, 2019 at 1:14 AM Elena Reshetova
> <elena.reshetova@intel.com> wrote:
> > diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
> > index 7bc105f47d21..28cb3687bf82 100644
> > --- a/arch/x86/entry/common.c
> > +++ b/arch/x86/entry/common.c
> > @@ -32,6 +32,10 @@
> >  #include <linux/uaccess.h>
> >  #include <asm/cpufeature.h>
> >
> > +#ifdef CONFIG_RANDOMIZE_KSTACK_OFFSET
> > +#include <linux/random.h>
> > +#endif
> > +
> >  #define CREATE_TRACE_POINTS
> >  #include <trace/events/syscalls.h>
> >
> > @@ -269,10 +273,22 @@ __visible inline void syscall_return_slowpath(struct
> pt_regs *regs)
> >  }
> >
> >  #ifdef CONFIG_X86_64
> > +
> > +#ifdef CONFIG_RANDOMIZE_KSTACK_OFFSET
> > +void *alloca(size_t size);
> > +#endif
> > +
> >  __visible void do_syscall_64(unsigned long nr, struct pt_regs *regs)
> >  {
> >         struct thread_info *ti;
> >
> > +#ifdef CONFIG_RANDOMIZE_KSTACK_OFFSET
> > +       size_t offset = ((size_t)prandom_u32()) % 256;
> > +       char *ptr = alloca(offset);
> > +
> > +       asm volatile("":"=m"(*ptr));
> > +#endif
> > +
> >         enter_from_user_mode();
> >         local_irq_enable();
> >         ti = current_thread_info();
> 
> Well this is delightfully short! 

Yes :) Looks like when you are allowed to use forbidden APIs, life might be 
suddenly much easier :) 

The alloca() definition could even be
> moved up after the #include of random.h, just to reduce the number of
> #ifdef lines, too.

Sure, can do this. 

 I patched getpid() to report stack locations for a
> given pid, just to get a sense of the entropy. On 10,000 getpid()
> calls I see counts like:
> 
>     229  ffffa58240697dbc
>     294  ffffa58240697dc4
>     315  ffffa58240697dcc
>     298  ffffa58240697dd4
>     335  ffffa58240697ddc
>     311  ffffa58240697de4
>     295  ffffa58240697dec
>     303  ffffa58240697df4
>     334  ffffa58240697dfc
>     331  ffffa58240697e04
>     321  ffffa58240697e0c
>     298  ffffa58240697e14
>     290  ffffa58240697e1c
>     306  ffffa58240697e24
>     308  ffffa58240697e2c
>     325  ffffa58240697e34
>     301  ffffa58240697e3c
>     336  ffffa58240697e44
>     328  ffffa58240697e4c
>     326  ffffa58240697e54
>     314  ffffa58240697e5c
>     305  ffffa58240697e64
>     315  ffffa58240697e6c
>     325  ffffa58240697e74
>     287  ffffa58240697e7c
>     319  ffffa58240697e84
>     309  ffffa58240697e8c
>     329  ffffa58240697e94
>     311  ffffa58240697e9c
>     306  ffffa58240697ea4
>     313  ffffa58240697eac
>     289  ffffa58240697eb4
>      94  ffffa58240697ebc
> 
> So it looks more like 5 bits of entropy in practice (here are 33
> unique stack locations), but that still looks good to me.

What I still don't fully understand here (due to my little knowledge of
compilers) and afraid of is that the asm code that alloca generates (see my version)
 and the alignment might differ on the different targets, etc. 
If you tried it on yours, can you send me the asm code that it produced for you?
Is it different from mine? 

> 
> Can you send the next version with a CC to lkml too?

I was thinking on not spamming lkml before we get some agreement here, but
I can do it if people believe this is the right way. 

Getting Andy's feedback on this version first would be great! 

Best Regards,
Elena.