* Linus Torvalds <torva...@linux-foundation.org> wrote:
> On Fri, Oct 18, 2019 at 4:42 PM Jörn Engel <jo...@purestorage.com> 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
