Re: [PATCH] random: make try_to_generate_entropy() more robust

[Ingo Molnar]

* Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:

> On Fri, Oct 18, 2019 at 4:42 PM Jörn Engel <joern@xxxxxxxxxxxxxxx> wrote:
> >
> > We can generate entropy on almost any CPU, even if it doesn't provide a
> > high-resolution timer for random_get_entropy().  As long as the CPU is
> > not idle, it changed the register file every few cycles.  As long as the
> > ALU isn't fully synchronized with the timer, the drift between the
> > register file and the timer is enough to generate entropy from.
> 
> >  static void entropy_timer(struct timer_list *t)
> >  {
> > +     struct pt_regs *regs = get_irq_regs();
> > +
> > +     /*
> > +      * Even if we don't have a high-resolution timer in our system,
> > +      * the register file itself is a high-resolution timer.  It
> > +      * isn't monotonic or particularly useful to read the current
> > +      * time.  But it changes with every retired instruction, which
> > +      * is enough to generate entropy from.
> > +      */
> > +     mix_pool_bytes(&input_pool, regs, sizeof(*regs));
> 
> Ok, so I still like this conceptually, but I'm not entirely sure that
> get_irq_regs() works reliably in a timer. It's done from softirq
> TIMER_SOFTIRQ context, so not necessarily _in_ an interrupt.
> 
> Now, admittedly this code doesn't really need "reliably". The odd
> occasional hickup would arguably just add more noise. And I think the
> code works fine. get_irq_regs() will return a pointer to the last
> interrupt or exception frame on the current CPU, and I guess it's all
> fine. But let's bring in Thomas, who was not only active in the
> randomness discussion, but might also have stronger opinions on this
> get_irq_regs() usage.
> 
> Thomas, opinions? Using the register state (while we're doing the
> whole entropy load with scheduling etc) looks like a good source of
> high-entropy data outside of just the TSC, so it does seem like a very
> valid model. But I want to run it past more people first, and Thomas
> is the obvious victim^Wchoice.

Not Thomas, but one potential problem I can see is that our 
set_irq_regs() use (on x86) is fundamentally nested, we restore whatever 
context we interrupt:

  dagon:~/tip> git grep set_irq_regs arch/x86
  arch/x86/include/asm/irq_regs.h:static inline struct pt_regs *set_irq_regs(struct pt_regs *new_regs)
  arch/x86/kernel/apic/apic.c:    struct pt_regs *old_regs = set_irq_regs(regs);
  arch/x86/kernel/apic/apic.c:    set_irq_regs(old_regs);
  arch/x86/kernel/cpu/acrn.c:     struct pt_regs *old_regs = set_irq_regs(regs);
  arch/x86/kernel/cpu/acrn.c:     set_irq_regs(old_regs);
  arch/x86/kernel/cpu/mshyperv.c: struct pt_regs *old_regs = set_irq_regs(regs);
  arch/x86/kernel/cpu/mshyperv.c: set_irq_regs(old_regs);
  arch/x86/kernel/cpu/mshyperv.c: struct pt_regs *old_regs = set_irq_regs(regs);
  arch/x86/kernel/cpu/mshyperv.c: set_irq_regs(old_regs);
  arch/x86/kernel/irq.c:  struct pt_regs *old_regs = set_irq_regs(regs);
  arch/x86/kernel/irq.c:  set_irq_regs(old_regs);
  arch/x86/kernel/irq.c:  struct pt_regs *old_regs = set_irq_regs(regs);
  arch/x86/kernel/irq.c:  set_irq_regs(old_regs);
  arch/x86/kernel/irq.c:  struct pt_regs *old_regs = set_irq_regs(regs);
  arch/x86/kernel/irq.c:  set_irq_regs(old_regs);
  arch/x86/kernel/irq.c:  struct pt_regs *old_regs = set_irq_regs(regs);
  arch/x86/kernel/irq.c:  set_irq_regs(old_regs);
  arch/x86/kernel/irq.c:  struct pt_regs *old_regs = set_irq_regs(regs);
  arch/x86/kernel/irq.c:  set_irq_regs(old_regs);

But from a softirq or threaded irq context that 'interrupted' regs 
context might potentially be NULL.

NULL isn't a good thing to pass to mix_pool_bytes(), because the first 
use of 'in' (='bytes') in _mix_pool_bytes() is a dereference without a 
NULL check AFAICS:

                w = rol32(*bytes++, input_rotate);

So at minimum I'd only mix this entropy into the pool if 'regs' is 
non-zero. This would automatically do the right thing and not crash the 
kernel on weird irq execution models such as threaded-only or -rt.

If irq-regs _is_ set, then I think we can generally rely on it to either 
be a valid regs pointer or NULL, inside an IRQ handler execution 
instance.

( Furthermore, if we are mixing in regs, then we might as well mix in a 
  few bytes of the interrupted stack as well if it's a kernel stack, 
  which would normally carry quite a bit of variation as well (such as 
  return addresses). Often it has more entropy than just register 
  contents, and it's also cache-hot, so a cheap source of entropy. But 
  that would require a second mix_pool_bytes() call and further 
  examination. Such an approach too would obviously require a non-NULL 
  'regs' pointer. :-) ]

Thanks,

	Ingo