From: Ingo Molnar <mingo@kernel.org>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: "Jörn Engel" <joern@purestorage.com>,
"Thomas Gleixner" <tglx@linutronix.de>,
"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] random: make try_to_generate_entropy() more robust
Date: Sat, 19 Oct 2019 09:39:07 +0200 [thread overview]
Message-ID: <20191019073907.GA101301@gmail.com> (raw)
In-Reply-To: <CAHk-=wi80VJ+4cUny2kwm0RxrmVdh24VPz5ZHjY4qCWR5iQBDQ@mail.gmail.com>
* Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Fri, Oct 18, 2019 at 4:42 PM Jörn Engel <joern@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
next prev parent reply other threads:[~2019-10-19 7:40 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20191018203704.GC31027@cork>
2019-10-18 20:42 ` [PATCH] random: make try_to_generate_entropy() more robust Jörn Engel
2019-10-18 22:58 ` Linus Torvalds
2019-10-19 2:25 ` Linus Torvalds
2019-10-19 7:39 ` Ingo Molnar [this message]
2019-10-19 10:13 ` Thomas Gleixner
2019-10-19 10:49 ` Thomas Gleixner
2019-10-19 14:37 ` Jörn Engel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191019073907.GA101301@gmail.com \
--to=mingo@kernel.org \
--cc=joern@purestorage.com \
--cc=linux-kernel@vger.kernel.org \
--cc=tglx@linutronix.de \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.