linux-kernel - [PATCH 2/3] random: do not use input pool from hard IRQs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20220506215454.1671-2-Jason@zx2c4.com>
Date:   Fri,  6 May 2022 23:54:53 +0200
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     linux-kernel@...r.kernel.org, linux-crypto@...r.kernel.org
Cc:     "Jason A. Donenfeld" <Jason@...c4.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Filipe Manana <fdmanana@...e.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Borislav Petkov <bp@...en8.de>, Theodore Ts'o <tytso@....edu>
Subject: [PATCH 2/3] random: do not use input pool from hard IRQs

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@...utronix.de>
Cc: Filipe Manana <fdmanana@...e.com>
Cc: Peter Zijlstra <peterz@...radead.org>
Cc: Borislav Petkov <bp@...en8.de>
Cc: Theodore Ts'o <tytso@....edu>
Signed-off-by: Jason A. Donenfeld <Jason@...c4.com>
---
 drivers/char/random.c | 6 ++++++
 1 file changed, 6 insertions(+)

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));
-- 
2.35.1