| Message ID | 20220506215454.1671-2-Jason@zx2c4.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] random: order timer entropy functions below interrupt functions | expand |
diff --git a/drivers/char/random.c b/drivers/char/random.c index d15f2133e804..818432638c18 100644 --- a/drivers/char/random.c +++ b/drivers/char/random.c @@ -1227,6 +1227,12 @@ static void add_timer_randomness(struct timer_rand_state *state, unsigned int nu unsigned long entropy = random_get_entropy(), now = jiffies, flags; long delta, delta2, delta3; + if (in_hardirq()) { + fast_mix(this_cpu_ptr(&irq_randomness)->pool, + (unsigned long[2]){ entropy, num }); + return; + } + spin_lock_irqsave(&input_pool.lock, flags); _mix_pool_bytes(&entropy, sizeof(entropy)); _mix_pool_bytes(&num, sizeof(num));
Years ago, a separate fast pool was added for interrupts, so that the cost associated with taking the input pool spinlocks and mixing into it would be avoided in places where latency is critical. However, one oversight was that add_input_randomness() and add_disk_randomness() still sometimes are called directly from the interrupt handler, rather than being deferred to a thread. This means that some unlucky interrupts will be caught doing a blake2s_compress() call and potentially spinning on input_pool.lock, which can also be taken by unprivileged users by writing into /dev/urandom. We also observe that the entropy estimation algorithm uses jiffies, which is rather coarse. In practice what this means that add_timer_randomness() will credit the first event called from an interrupt, but will skip the subsequent ones. However, if this is triggered by an interrupt, the first event will *also* hit add_interrupt_randomness(), which does its own crediting. So in essence we double-count these inputs. So this commit not only avoids calling the expensive functions from hard IRQ but also avoids this double counting. In order to fix these issues, add_timer_randomness() now checks whether it is being called from a hard IRQ and if so, just mixes into the per-cpu IRQ fast pool using fast_mix(), which is much faster and can be done lock-free. A nice consequence of this, as well, is that it means hard IRQ context FPU support is likely no longer useful. A downside of this, however, is that the num argument is potentially attacker controlled, which puts a bit more pressure on the fast_mix() sponge to do more than it's really intended to do. As a mitigating factor, the first 96 bits of input aren't attacker controlled (a cycle counter followed by zeros), which means it's essentially two rounds of siphash rather than one, which is somewhat better. It's also not that much different from add_interrupt_randomness()'s use of the irq stack instruction pointer register. Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Filipe Manana <fdmanana@suse.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Borislav Petkov <bp@alien8.de> Cc: Theodore Ts'o <tytso@mit.edu> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> --- drivers/char/random.c | 6 ++++++ 1 file changed, 6 insertions(+)