On Thu, May 2, 2019 at 2:23 AM David Laight <david.lai...@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
