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

[Andy Lutomirski <luto@kernel.org>]

On Thu, May 2, 2019 at 2:23 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Reshetova, Elena
> > Sent: 02 May 2019 09:16
> ...
> > > I'm also guessing that get_cpu_var() disables pre-emption?
> >
> > Yes, in my understanding:
> >
> > #define get_cpu_var(var)                                              \
> > (*({                                                                  \
> >       preempt_disable();                                              \
> >       this_cpu_ptr(&var);                                             \
> > }))
> >
> > > This code could probably run 'fast and loose' and just ignore
> > > the fact that pre-emption would have odd effects.
> > > All it would do is perturb the randomness!
> >
> > Hm.. I see your point, but I am wondering what the odd effects might
> > be.. i.e. can we end up using the same random bits twice for two or more
> > different syscalls and attackers can try to trigger this situation?
>
> To trigger it you'd need to arrange for an interrupt in the right
> timing window to cause another process to run.
> There are almost certainly easier ways to break things.
>
> I think the main effects would be the increment writing to a different
> cpu local data (causing the same data to be used again and/or skipped)
> and the potential for updating the random buffer on the 'wrong cpu'.
>
> So something like:
>         /* We don't really care if the update is written to the 'wrong'
>          * cpu or if the vale comes from the wrong buffer. */
>         offset = *this_cpu_ptr(&cpu_syscall_rand_offset);
>         *this_cpu_ptr(&cpu_syscall_rand_offset) = offset + 1;
>
>         if ((offset &= 4095)) return this_cpu_ptr(&cpu_syscall_rand_buffer)[offset];
>
>         buffer = get_cpu_var((&cpu_syscall_rand_buffer);
>         get_random_bytes();
>         val = buffer[0];
>         /* maybe set cpu_syscall_rand_offset to 1 */
>         put_cpu_var();
>         return val;
>
> The whole thing might even work with a global buffer!
>

I don't see how this makes sense in the context of the actual entry
code.  The code looks like this right now:

        enter_from_user_mode();
<--- IRQs off here
        local_irq_enable();

Presumably this could become:

enter_from_user_mode();
if (the percpu buffer has enough bytes) {
  use them;
  local_irq_enable();
} else {
  local_irq_enable();
  get more bytes;
  if (get_cpu() == the old cpu) {
    refill the buffer;
  } else {
    feel rather silly;
  }
  put_cpu();
}

everything after the enter_from_user_mode() could get renamed
get_randstack_offset_and_irq_enable().

Or we decide that calling get_random_bytes() is okay with IRQs off and
this all gets a bit simpler.

--Andy