linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] random: make irq collector clearer
@ 2022-02-10 16:09 Jason A. Donenfeld
  2022-02-10 16:09 ` [PATCH 1/2] random: move fast_pool/fast_mix definitions to site of use Jason A. Donenfeld
  2022-02-10 16:09 ` [PATCH 2/2] random: deobfuscate irq u32/u64 contributions Jason A. Donenfeld
  0 siblings, 2 replies; 9+ messages in thread
From: Jason A. Donenfeld @ 2022-02-10 16:09 UTC (permalink / raw)
  To: linux, linux-kernel; +Cc: Jason A. Donenfeld

This trivial series moves functions around and introduces a union rather
than having manual bit arithmetic.

Jason A. Donenfeld (2):
  random: move fast_pool/fast_mix definitions to site of use
  random: deobfuscate irq u32/u64 contributions

 drivers/char/random.c | 141 ++++++++++++++++++++++--------------------
 1 file changed, 74 insertions(+), 67 deletions(-)

-- 
2.35.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] random: move fast_pool/fast_mix definitions to site of use
  2022-02-10 16:09 [PATCH 0/2] random: make irq collector clearer Jason A. Donenfeld
@ 2022-02-10 16:09 ` Jason A. Donenfeld
  2022-02-10 16:46   ` Dominik Brodowski
  2022-02-10 16:09 ` [PATCH 2/2] random: deobfuscate irq u32/u64 contributions Jason A. Donenfeld
  1 sibling, 1 reply; 9+ messages in thread
From: Jason A. Donenfeld @ 2022-02-10 16:09 UTC (permalink / raw)
  To: linux, linux-kernel; +Cc: Jason A. Donenfeld, Theodore Ts'o

No actual code changes, just code position changes. Having to scroll up
and down by several hundred lines every time is confusing and hard to
follow. Instead cluster these together. Later when we're redocumenting
the file, it'll mean a smaller delta.

Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/char/random.c | 101 +++++++++++++++++++++---------------------
 1 file changed, 51 insertions(+), 50 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 77131f7b0f06..923a8f861437 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -281,45 +281,6 @@ static void mix_pool_bytes(const void *in, size_t nbytes)
 	spin_unlock_irqrestore(&input_pool.lock, flags);
 }
 
-struct fast_pool {
-	struct work_struct mix;
-	unsigned long last;
-	u32 pool[4];
-	unsigned int count;
-	u16 reg_idx;
-};
-#define FAST_POOL_MIX_INFLIGHT (1U << 31)
-
-/*
- * This is a fast mixing routine used by the interrupt randomness
- * collector.  It's hardcoded for an 128 bit pool and assumes that any
- * locks that might be needed are taken by the caller.
- */
-static void fast_mix(struct fast_pool *f)
-{
-	u32 a = f->pool[0],	b = f->pool[1];
-	u32 c = f->pool[2],	d = f->pool[3];
-
-	a += b;			c += d;
-	b = rol32(b, 6);	d = rol32(d, 27);
-	d ^= a;			b ^= c;
-
-	a += b;			c += d;
-	b = rol32(b, 16);	d = rol32(d, 14);
-	d ^= a;			b ^= c;
-
-	a += b;			c += d;
-	b = rol32(b, 6);	d = rol32(d, 27);
-	d ^= a;			b ^= c;
-
-	a += b;			c += d;
-	b = rol32(b, 16);	d = rol32(d, 14);
-	d ^= a;			b ^= c;
-
-	f->pool[0] = a;  f->pool[1] = b;
-	f->pool[2] = c;  f->pool[3] = d;
-}
-
 static void process_random_ready_list(void)
 {
 	unsigned long flags;
@@ -758,6 +719,57 @@ void add_input_randomness(unsigned int type, unsigned int code,
 }
 EXPORT_SYMBOL_GPL(add_input_randomness);
 
+#ifdef CONFIG_BLOCK
+void add_disk_randomness(struct gendisk *disk)
+{
+	if (!disk || !disk->random)
+		return;
+	/* first major is 1, so we get >= 0x200 here */
+	add_timer_randomness(disk->random, 0x100 + disk_devt(disk));
+}
+EXPORT_SYMBOL_GPL(add_disk_randomness);
+#endif
+
+struct fast_pool {
+	struct work_struct mix;
+	unsigned long last;
+	u32 pool[4];
+	unsigned int count;
+	u16 reg_idx;
+};
+#define FAST_POOL_MIX_INFLIGHT (1U << 31)
+
+/*
+ * This is a fast mixing routine used by the interrupt randomness
+ * collector. It's hardcoded for an 128 bit pool and assumes that any
+ * locks that might be needed are taken by the caller.
+ */
+static void fast_mix(struct fast_pool *f)
+{
+	u32 a = f->pool[0],	b = f->pool[1];
+	u32 c = f->pool[2],	d = f->pool[3];
+
+	a += b;			c += d;
+	b = rol32(b, 6);	d = rol32(d, 27);
+	d ^= a;			b ^= c;
+
+	a += b;			c += d;
+	b = rol32(b, 16);	d = rol32(d, 14);
+	d ^= a;			b ^= c;
+
+	a += b;			c += d;
+	b = rol32(b, 6);	d = rol32(d, 27);
+	d ^= a;			b ^= c;
+
+	a += b;			c += d;
+	b = rol32(b, 16);	d = rol32(d, 14);
+	d ^= a;			b ^= c;
+
+	f->pool[0] = a;  f->pool[1] = b;
+	f->pool[2] = c;  f->pool[3] = d;
+}
+
+
 static DEFINE_PER_CPU(struct fast_pool, irq_randomness);
 
 static u32 get_reg(struct fast_pool *f, struct pt_regs *regs)
@@ -849,17 +861,6 @@ void add_interrupt_randomness(int irq)
 }
 EXPORT_SYMBOL_GPL(add_interrupt_randomness);
 
-#ifdef CONFIG_BLOCK
-void add_disk_randomness(struct gendisk *disk)
-{
-	if (!disk || !disk->random)
-		return;
-	/* first major is 1, so we get >= 0x200 here */
-	add_timer_randomness(disk->random, 0x100 + disk_devt(disk));
-}
-EXPORT_SYMBOL_GPL(add_disk_randomness);
-#endif
-
 /*********************************************************************
  *
  * Entropy extraction routines
-- 
2.35.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] random: deobfuscate irq u32/u64 contributions
  2022-02-10 16:09 [PATCH 0/2] random: make irq collector clearer Jason A. Donenfeld
  2022-02-10 16:09 ` [PATCH 1/2] random: move fast_pool/fast_mix definitions to site of use Jason A. Donenfeld
@ 2022-02-10 16:09 ` Jason A. Donenfeld
  2022-02-10 16:55   ` Dominik Brodowski
  1 sibling, 1 reply; 9+ messages in thread
From: Jason A. Donenfeld @ 2022-02-10 16:09 UTC (permalink / raw)
  To: linux, linux-kernel; +Cc: Jason A. Donenfeld, Theodore Ts'o

In the irq handler, we fill out 16 bytes differently on 32-bit and
64-bit platforms. Whether or not you like that, it is a matter of fact.
But it might not be a fact you well realized until now, because the code
that loaded the irq info into 4 32-bit words was quite confusing.
Instead, this commit makes everything explicit by having separate
(compile-time) branches for 32-bit and 64-bit machines. In the process,
it exposed a shortcoming in in mix_interrupt_randomness() which we
rectify.

Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/char/random.c | 54 ++++++++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 923a8f861437..1eb3c059025b 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -731,9 +731,12 @@ EXPORT_SYMBOL_GPL(add_disk_randomness);
 #endif
 
 struct fast_pool {
+	union {
+		u64 pool64[2];
+		u32 pool32[4];
+	};
 	struct work_struct mix;
 	unsigned long last;
-	u32 pool[4];
 	unsigned int count;
 	u16 reg_idx;
 };
@@ -744,10 +747,10 @@ struct fast_pool {
  * collector. It's hardcoded for an 128 bit pool and assumes that any
  * locks that might be needed are taken by the caller.
  */
-static void fast_mix(struct fast_pool *f)
+static void fast_mix(u32 pool[4])
 {
-	u32 a = f->pool[0],	b = f->pool[1];
-	u32 c = f->pool[2],	d = f->pool[3];
+	u32 a = pool[0],	b = pool[1];
+	u32 c = pool[2],	d = pool[3];
 
 	a += b;			c += d;
 	b = rol32(b, 6);	d = rol32(d, 27);
@@ -765,11 +768,10 @@ static void fast_mix(struct fast_pool *f)
 	b = rol32(b, 16);	d = rol32(d, 14);
 	d ^= a;			b ^= c;
 
-	f->pool[0] = a;  f->pool[1] = b;
-	f->pool[2] = c;  f->pool[3] = d;
+	pool[0] = a;  pool[1] = b;
+	pool[2] = c;  pool[3] = d;
 }
 
-
 static DEFINE_PER_CPU(struct fast_pool, irq_randomness);
 
 static u32 get_reg(struct fast_pool *f, struct pt_regs *regs)
@@ -790,15 +792,19 @@ static u32 get_reg(struct fast_pool *f, struct pt_regs *regs)
 static void mix_interrupt_randomness(struct work_struct *work)
 {
 	struct fast_pool *fast_pool = container_of(work, struct fast_pool, mix);
-	u32 pool[ARRAY_SIZE(fast_pool->pool)];
+	u32 pool[ARRAY_SIZE(fast_pool->pool32)];
 
 	/*
 	 * Since this is the result of a trip through the scheduler, xor in
 	 * a cycle counter. It can't hurt, and might help.
 	 */
-	fast_pool->pool[3] ^= random_get_entropy();
+	if (sizeof(unsigned long) == 8)
+		fast_pool->pool64[1] ^= random_get_entropy();
+	else
+		fast_pool->pool32[3] ^= random_get_entropy();
+
 	/* Copy the pool to the stack so that the mixer always has a consistent view. */
-	memcpy(pool, fast_pool->pool, sizeof(pool));
+	memcpy(pool, fast_pool->pool32, sizeof(pool));
 	/* We take care to zero out the count only after we're done reading the pool. */
 	WRITE_ONCE(fast_pool->count, 0);
 	fast_pool->last = jiffies;
@@ -815,26 +821,26 @@ void add_interrupt_randomness(int irq)
 	unsigned long now = jiffies;
 	cycles_t cycles = random_get_entropy();
 	unsigned int new_count;
-	u32 c_high, j_high;
-	u64 ip;
 
 	if (cycles == 0)
 		cycles = get_reg(fast_pool, regs);
-	c_high = (sizeof(cycles) > 4) ? cycles >> 32 : 0;
-	j_high = (sizeof(now) > 4) ? now >> 32 : 0;
-	fast_pool->pool[0] ^= cycles ^ j_high ^ irq;
-	fast_pool->pool[1] ^= now ^ c_high;
-	ip = regs ? instruction_pointer(regs) : _RET_IP_;
-	fast_pool->pool[2] ^= ip;
-	fast_pool->pool[3] ^=
-		(sizeof(ip) > 4) ? ip >> 32 : get_reg(fast_pool, regs);
-
-	fast_mix(fast_pool);
+
+	if (sizeof(unsigned long) == 8) {
+		fast_pool->pool64[0] ^= cycles ^ rol64(now, 32) ^ irq;
+		fast_pool->pool64[1] ^= regs ? instruction_pointer(regs) : _RET_IP_;
+	} else {
+		fast_pool->pool32[0] ^= cycles ^ irq;
+		fast_pool->pool32[1] ^= now;
+		fast_pool->pool32[2] ^= regs ? instruction_pointer(regs) : _RET_IP_;
+		fast_pool->pool32[3] ^= get_reg(fast_pool, regs);
+	}
+
+	fast_mix(fast_pool->pool32);
 	new_count = ++fast_pool->count;
 
 	if (unlikely(crng_init == 0)) {
 		if (new_count >= 64 &&
-		    crng_fast_load(fast_pool->pool, sizeof(fast_pool->pool)) > 0) {
+		    crng_fast_load(fast_pool->pool32, sizeof(fast_pool->pool32)) > 0) {
 			fast_pool->count = 0;
 			fast_pool->last = now;
 
@@ -843,7 +849,7 @@ void add_interrupt_randomness(int irq)
 			 * However, this only happens during boot, and then never
 			 * again, so we live with it.
 			 */
-			mix_pool_bytes(&fast_pool->pool, sizeof(fast_pool->pool));
+			mix_pool_bytes(&fast_pool->pool32, sizeof(fast_pool->pool32));
 		}
 		return;
 	}
-- 
2.35.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] random: move fast_pool/fast_mix definitions to site of use
  2022-02-10 16:09 ` [PATCH 1/2] random: move fast_pool/fast_mix definitions to site of use Jason A. Donenfeld
@ 2022-02-10 16:46   ` Dominik Brodowski
  0 siblings, 0 replies; 9+ messages in thread
From: Dominik Brodowski @ 2022-02-10 16:46 UTC (permalink / raw)
  To: Jason A. Donenfeld; +Cc: linux-kernel, Theodore Ts'o

Am Thu, Feb 10, 2022 at 05:09:24PM +0100 schrieb Jason A. Donenfeld:
> No actual code changes, just code position changes. Having to scroll up
> and down by several hundred lines every time is confusing and hard to
> follow. Instead cluster these together. Later when we're redocumenting
> the file, it'll mean a smaller delta.
> 
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: Dominik Brodowski <linux@dominikbrodowski.net>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

	Reviewed-by: Dominik Brodowski <linux@dominikbrodowski.net>

Thanks,
	Dominik

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] random: deobfuscate irq u32/u64 contributions
  2022-02-10 16:09 ` [PATCH 2/2] random: deobfuscate irq u32/u64 contributions Jason A. Donenfeld
@ 2022-02-10 16:55   ` Dominik Brodowski
  2022-02-10 17:10     ` Jason A. Donenfeld
  0 siblings, 1 reply; 9+ messages in thread
From: Dominik Brodowski @ 2022-02-10 16:55 UTC (permalink / raw)
  To: Jason A. Donenfeld; +Cc: linux-kernel, Theodore Ts'o

Am Thu, Feb 10, 2022 at 05:09:25PM +0100 schrieb Jason A. Donenfeld:
> In the irq handler, we fill out 16 bytes differently on 32-bit and
> 64-bit platforms. Whether or not you like that, it is a matter of fact.
> But it might not be a fact you well realized until now, because the code
> that loaded the irq info into 4 32-bit words was quite confusing.
> Instead, this commit makes everything explicit by having separate
> (compile-time) branches for 32-bit and 64-bit machines. In the process,
> it exposed a shortcoming in in mix_interrupt_randomness() which we

"in in" -> "in"

> rectify.

Maybe explain the shortcoming in one sentence? I think I spotted it, but...

Thanks,
	Dominik

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] random: deobfuscate irq u32/u64 contributions
  2022-02-10 16:55   ` Dominik Brodowski
@ 2022-02-10 17:10     ` Jason A. Donenfeld
  2022-02-10 17:13       ` [PATCH v2] " Jason A. Donenfeld
  0 siblings, 1 reply; 9+ messages in thread
From: Jason A. Donenfeld @ 2022-02-10 17:10 UTC (permalink / raw)
  To: Dominik Brodowski; +Cc: LKML, Theodore Ts'o

On Thu, Feb 10, 2022 at 5:57 PM Dominik Brodowski
<linux@dominikbrodowski.net> wrote:
>
> Am Thu, Feb 10, 2022 at 05:09:25PM +0100 schrieb Jason A. Donenfeld:
> > In the irq handler, we fill out 16 bytes differently on 32-bit and
> > 64-bit platforms. Whether or not you like that, it is a matter of fact.
> > But it might not be a fact you well realized until now, because the code
> > that loaded the irq info into 4 32-bit words was quite confusing.
> > Instead, this commit makes everything explicit by having separate
> > (compile-time) branches for 32-bit and 64-bit machines. In the process,
> > it exposed a shortcoming in in mix_interrupt_randomness() which we
>
> "in in" -> "in"
>
> > rectify.
>
> Maybe explain the shortcoming in one sentence? I think I spotted it, but...

Will do. v2 incoming.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2] random: deobfuscate irq u32/u64 contributions
  2022-02-10 17:10     ` Jason A. Donenfeld
@ 2022-02-10 17:13       ` Jason A. Donenfeld
  2022-02-11  5:57         ` Dominik Brodowski
  0 siblings, 1 reply; 9+ messages in thread
From: Jason A. Donenfeld @ 2022-02-10 17:13 UTC (permalink / raw)
  To: linux, linux-kernel; +Cc: Jason A. Donenfeld, Theodore Ts'o

In the irq handler, we fill out 16 bytes differently on 32-bit and
64-bit platforms. Whether or not you like that, it is a matter of fact.
But it might not be a fact you well realized until now, because the code
that loaded the irq info into 4 32-bit words was quite confusing.
Instead, this commit makes everything explicit by having separate
(compile-time) branches for 32-bit and 64-bit machines. In the process,
we now easily see that we were truncating the contribution of
random_get_entropy() in mix_interrupt_randomness() which we rectify by
using the correct integer type.

Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Changes v1->v2:
- Expand commit message's description of mix_interrupt_randomness()
  change.

 drivers/char/random.c | 54 ++++++++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 69b2c7f078d8..324574b03120 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -767,9 +767,12 @@ EXPORT_SYMBOL_GPL(add_disk_randomness);
 #endif
 
 struct fast_pool {
+	union {
+		u64 pool64[2];
+		u32 pool32[4];
+	};
 	struct work_struct mix;
 	unsigned long last;
-	u32 pool[4];
 	unsigned int count;
 	u16 reg_idx;
 };
@@ -780,10 +783,10 @@ struct fast_pool {
  * collector. It's hardcoded for an 128 bit pool and assumes that any
  * locks that might be needed are taken by the caller.
  */
-static void fast_mix(struct fast_pool *f)
+static void fast_mix(u32 pool[4])
 {
-	u32 a = f->pool[0],	b = f->pool[1];
-	u32 c = f->pool[2],	d = f->pool[3];
+	u32 a = pool[0],	b = pool[1];
+	u32 c = pool[2],	d = pool[3];
 
 	a += b;			c += d;
 	b = rol32(b, 6);	d = rol32(d, 27);
@@ -801,11 +804,10 @@ static void fast_mix(struct fast_pool *f)
 	b = rol32(b, 16);	d = rol32(d, 14);
 	d ^= a;			b ^= c;
 
-	f->pool[0] = a;  f->pool[1] = b;
-	f->pool[2] = c;  f->pool[3] = d;
+	pool[0] = a;  pool[1] = b;
+	pool[2] = c;  pool[3] = d;
 }
 
-
 static DEFINE_PER_CPU(struct fast_pool, irq_randomness);
 
 static u32 get_reg(struct fast_pool *f, struct pt_regs *regs)
@@ -826,15 +828,19 @@ static u32 get_reg(struct fast_pool *f, struct pt_regs *regs)
 static void mix_interrupt_randomness(struct work_struct *work)
 {
 	struct fast_pool *fast_pool = container_of(work, struct fast_pool, mix);
-	u32 pool[ARRAY_SIZE(fast_pool->pool)];
+	u32 pool[ARRAY_SIZE(fast_pool->pool32)];
 
 	/*
 	 * Since this is the result of a trip through the scheduler, xor in
 	 * a cycle counter. It can't hurt, and might help.
 	 */
-	fast_pool->pool[3] ^= random_get_entropy();
+	if (sizeof(unsigned long) == 8)
+		fast_pool->pool64[1] ^= random_get_entropy();
+	else
+		fast_pool->pool32[3] ^= random_get_entropy();
+
 	/* Copy the pool to the stack so that the mixer always has a consistent view. */
-	memcpy(pool, fast_pool->pool, sizeof(pool));
+	memcpy(pool, fast_pool->pool32, sizeof(pool));
 	/* We take care to zero out the count only after we're done reading the pool. */
 	WRITE_ONCE(fast_pool->count, 0);
 	fast_pool->last = jiffies;
@@ -851,26 +857,26 @@ void add_interrupt_randomness(int irq)
 	unsigned long now = jiffies;
 	cycles_t cycles = random_get_entropy();
 	unsigned int new_count;
-	u32 c_high, j_high;
-	u64 ip;
 
 	if (cycles == 0)
 		cycles = get_reg(fast_pool, regs);
-	c_high = (sizeof(cycles) > 4) ? cycles >> 32 : 0;
-	j_high = (sizeof(now) > 4) ? now >> 32 : 0;
-	fast_pool->pool[0] ^= cycles ^ j_high ^ irq;
-	fast_pool->pool[1] ^= now ^ c_high;
-	ip = regs ? instruction_pointer(regs) : _RET_IP_;
-	fast_pool->pool[2] ^= ip;
-	fast_pool->pool[3] ^=
-		(sizeof(ip) > 4) ? ip >> 32 : get_reg(fast_pool, regs);
-
-	fast_mix(fast_pool);
+
+	if (sizeof(unsigned long) == 8) {
+		fast_pool->pool64[0] ^= cycles ^ rol64(now, 32) ^ irq;
+		fast_pool->pool64[1] ^= regs ? instruction_pointer(regs) : _RET_IP_;
+	} else {
+		fast_pool->pool32[0] ^= cycles ^ irq;
+		fast_pool->pool32[1] ^= now;
+		fast_pool->pool32[2] ^= regs ? instruction_pointer(regs) : _RET_IP_;
+		fast_pool->pool32[3] ^= get_reg(fast_pool, regs);
+	}
+
+	fast_mix(fast_pool->pool32);
 	new_count = ++fast_pool->count;
 
 	if (unlikely(crng_init == 0)) {
 		if (new_count >= 64 &&
-		    crng_fast_load(fast_pool->pool, sizeof(fast_pool->pool)) > 0) {
+		    crng_fast_load(fast_pool->pool32, sizeof(fast_pool->pool32)) > 0) {
 			fast_pool->count = 0;
 			fast_pool->last = now;
 
@@ -879,7 +885,7 @@ void add_interrupt_randomness(int irq)
 			 * However, this only happens during boot, and then never
 			 * again, so we live with it.
 			 */
-			mix_pool_bytes(&fast_pool->pool, sizeof(fast_pool->pool));
+			mix_pool_bytes(&fast_pool->pool32, sizeof(fast_pool->pool32));
 		}
 		return;
 	}
-- 
2.35.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] random: deobfuscate irq u32/u64 contributions
  2022-02-10 17:13       ` [PATCH v2] " Jason A. Donenfeld
@ 2022-02-11  5:57         ` Dominik Brodowski
  2022-02-15 11:48           ` [PATCH v3] " Jason A. Donenfeld
  0 siblings, 1 reply; 9+ messages in thread
From: Dominik Brodowski @ 2022-02-11  5:57 UTC (permalink / raw)
  To: Jason A. Donenfeld; +Cc: linux-kernel, Theodore Ts'o

Am Thu, Feb 10, 2022 at 06:13:22PM +0100 schrieb Jason A. Donenfeld:
> In the irq handler, we fill out 16 bytes differently on 32-bit and
> 64-bit platforms. Whether or not you like that, it is a matter of fact.
> But it might not be a fact you well realized until now, because the code
> that loaded the irq info into 4 32-bit words was quite confusing.
> Instead, this commit makes everything explicit by having separate
> (compile-time) branches for 32-bit and 64-bit machines. In the process,
> we now easily see that we were truncating the contribution of
> random_get_entropy() in mix_interrupt_randomness() which we rectify by
> using the correct integer type.
> 
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: Dominik Brodowski <linux@dominikbrodowski.net>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

	Reviewed-by: Dominik Brodowski <linux@dominikbrodowski.net>

Thanks,
	Dominik

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3] random: deobfuscate irq u32/u64 contributions
  2022-02-11  5:57         ` Dominik Brodowski
@ 2022-02-15 11:48           ` Jason A. Donenfeld
  0 siblings, 0 replies; 9+ messages in thread
From: Jason A. Donenfeld @ 2022-02-15 11:48 UTC (permalink / raw)
  To: linux-kernel; +Cc: Jason A. Donenfeld, Theodore Ts'o, Dominik Brodowski

In the irq handler, we fill out 16 bytes differently on 32-bit and
64-bit platforms, and for 32-bit vs 64-bit cycle counters, which doesn't
always correspond with the bitness of the platform. Whether or not you
like this strangeness, it is a matter of fact.  But it might not be a
fact you well realized until now, because the code that loaded the irq
info into 4 32-bit words was quite confusing.  Instead, this commit
makes everything explicit by having separate (compile-time) branches for
32-bit and 64-bit types.

Cc: Theodore Ts'o <tytso@mit.edu>
Reviewed-by: Dominik Brodowski <linux@dominikbrodowski.net>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Changes v2->v3:
- cycles_t is sometimes an `unsigned long long`, even on 32-bit x86, so
  separate that contribution from the other one.
- rebased on the latest.

 drivers/char/random.c | 49 ++++++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 21 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 9714d9f05a84..6a2c7db94417 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -283,7 +283,10 @@ static void mix_pool_bytes(const void *in, size_t nbytes)
 }
 
 struct fast_pool {
-	u32 pool[4];
+	union {
+		u32 pool32[4];
+		u64 pool64[2];
+	};
 	unsigned long last;
 	u16 reg_idx;
 	u8 count;
@@ -294,10 +297,10 @@ struct fast_pool {
  * collector.  It's hardcoded for an 128 bit pool and assumes that any
  * locks that might be needed are taken by the caller.
  */
-static void fast_mix(struct fast_pool *f)
+static void fast_mix(u32 pool[4])
 {
-	u32 a = f->pool[0],	b = f->pool[1];
-	u32 c = f->pool[2],	d = f->pool[3];
+	u32 a = pool[0],	b = pool[1];
+	u32 c = pool[2],	d = pool[3];
 
 	a += b;			c += d;
 	b = rol32(b, 6);	d = rol32(d, 27);
@@ -315,9 +318,8 @@ static void fast_mix(struct fast_pool *f)
 	b = rol32(b, 16);	d = rol32(d, 14);
 	d ^= a;			b ^= c;
 
-	f->pool[0] = a;  f->pool[1] = b;
-	f->pool[2] = c;  f->pool[3] = d;
-	f->count++;
+	pool[0] = a;  pool[1] = b;
+	pool[2] = c;  pool[3] = d;
 }
 
 static void process_random_ready_list(void)
@@ -778,29 +780,34 @@ void add_interrupt_randomness(int irq)
 	struct pt_regs *regs = get_irq_regs();
 	unsigned long now = jiffies;
 	cycles_t cycles = random_get_entropy();
-	u32 c_high, j_high;
-	u64 ip;
 
 	if (cycles == 0)
 		cycles = get_reg(fast_pool, regs);
-	c_high = (sizeof(cycles) > 4) ? cycles >> 32 : 0;
-	j_high = (sizeof(now) > 4) ? now >> 32 : 0;
-	fast_pool->pool[0] ^= cycles ^ j_high ^ irq;
-	fast_pool->pool[1] ^= now ^ c_high;
-	ip = regs ? instruction_pointer(regs) : _RET_IP_;
-	fast_pool->pool[2] ^= ip;
-	fast_pool->pool[3] ^=
-		(sizeof(ip) > 4) ? ip >> 32 : get_reg(fast_pool, regs);
 
-	fast_mix(fast_pool);
+	if (sizeof(cycles) == 8)
+		fast_pool->pool64[0] ^= cycles ^ rol64(now, 32) ^ irq;
+	else {
+		fast_pool->pool32[0] ^= cycles ^ irq;
+		fast_pool->pool32[1] ^= now;
+	}
+
+	if (sizeof(unsigned long) == 8)
+		fast_pool->pool64[1] ^= regs ? instruction_pointer(regs) : _RET_IP_;
+	else {
+		fast_pool->pool32[2] ^= regs ? instruction_pointer(regs) : _RET_IP_;
+		fast_pool->pool32[3] ^= get_reg(fast_pool, regs);
+	}
+
+	fast_mix(fast_pool->pool32);
+	++fast_pool->count;
 
 	if (unlikely(crng_init == 0)) {
 		if (fast_pool->count >= 64 &&
-		    crng_fast_load(fast_pool->pool, sizeof(fast_pool->pool)) > 0) {
+		    crng_fast_load(fast_pool->pool32, sizeof(fast_pool->pool32)) > 0) {
 			fast_pool->count = 0;
 			fast_pool->last = now;
 			if (spin_trylock(&input_pool.lock)) {
-				_mix_pool_bytes(&fast_pool->pool, sizeof(fast_pool->pool));
+				_mix_pool_bytes(&fast_pool->pool32, sizeof(fast_pool->pool32));
 				spin_unlock(&input_pool.lock);
 			}
 		}
@@ -814,7 +821,7 @@ void add_interrupt_randomness(int irq)
 		return;
 
 	fast_pool->last = now;
-	_mix_pool_bytes(&fast_pool->pool, sizeof(fast_pool->pool));
+	_mix_pool_bytes(&fast_pool->pool32, sizeof(fast_pool->pool32));
 	spin_unlock(&input_pool.lock);
 
 	fast_pool->count = 0;
-- 
2.35.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-02-15 11:48 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-10 16:09 [PATCH 0/2] random: make irq collector clearer Jason A. Donenfeld
2022-02-10 16:09 ` [PATCH 1/2] random: move fast_pool/fast_mix definitions to site of use Jason A. Donenfeld
2022-02-10 16:46   ` Dominik Brodowski
2022-02-10 16:09 ` [PATCH 2/2] random: deobfuscate irq u32/u64 contributions Jason A. Donenfeld
2022-02-10 16:55   ` Dominik Brodowski
2022-02-10 17:10     ` Jason A. Donenfeld
2022-02-10 17:13       ` [PATCH v2] " Jason A. Donenfeld
2022-02-11  5:57         ` Dominik Brodowski
2022-02-15 11:48           ` [PATCH v3] " Jason A. Donenfeld

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).