tech-crypto archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: x86 CPU RNG support, take 2
Date: Fri, 25 Dec 2015 18:27:50 -0500
From: Thor Lancelot Simon <tls%panix.com@localhost>
I've cleaned it up a little and hooked it up as a standard entropy source
per Taylor's comments. To avoid a pile of largely pointless config glue,
it is an internal source in kern_rndq.c just like the "callout" source.
I think this can probably be used for the onboard RNG on other CPUs as well.
This seems to have several functional changes in it:
- Add cpu_rng(9) and its rndsource.
- Implement cpu_rng(9) on x86 using RDRAND/RDSEED for Intel CPUs.
- Convert VIA RNG to cpu_rng(9).
Please split them into separate commits, and separate patches for
review. Here are some comments on a first round of review pending
that split:
--- arch/amd64/include/types.h 27 Aug 2015 12:30:50 -0000 1.48
+++ arch/amd64/include/types.h 19 Dec 2015 21:39:37 -0000
@@ -93,6 +93,7 @@ typedef unsigned char __cpu_simple_lock
#define __HAVE_TLS_VARIANT_II
#define __HAVE_COMMON___TLS_GET_ADDR
#define __HAVE_INTR_CONTROL
+#define __HAVE_CPU_RNG
KNF: tab, not space, like the rest.
--- arch/i386/include/types.h 27 Aug 2015 12:30:51 -0000 1.83
+++ arch/i386/include/types.h 19 Dec 2015 21:40:33 -0000
@@ -109,6 +109,8 @@ typedef unsigned char __cpu_simple_lock
#define __HAVE_SYSCALL_INTERN
#define __HAVE_MINIMAL_EMUL
#define __HAVE_OLD_DISKLABEL
+#define __HAVE_CPU_RNG
Same.
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ arch/x86/include/cpu_rng.h 25 Dec 2015 22:57:30 -0000
@@ -0,0 +1,49 @@
+/* $NetBSD: $ */
$NetBSD$
+#ifndef _X86_CPURNG_H_
+#define _X86_CPURNG_H_
File name has an underscore in it.
+#include <sys/param.h>
+#include <sys/systm.h>
+#include <sys/cpu.h>
+
+#include <x86/specialreg.h>
+
+#include <machine/cpufunc.h>
+#include <machine/cpuvar.h>
Omit needless includes. All you need is <sys/types.h> for uint64_t
and size_t.
+typedef uint64_t cpu_rng_t;
+
+void cpu_rng_init(void);
+size_t cpu_rng(cpu_rng_t *);
+
+#endif
Note /* _X86_CPU_RNG_H_ */ after #endif.
--- arch/x86/include/via_padlock.h 13 Apr 2015 16:03:51 -0000 1.8
+++ arch/x86/include/via_padlock.h 25 Dec 2015 22:10:56 -0000
@@ -74,8 +69,6 @@ struct via_padlock_softc {
#define VIAC3_SESSION(sid) ((sid) & 0x0fffffff)
#define VIAC3_SID(crd,ses) (((crd) << 28) | ((ses) & 0x0fffffff))
-#define VIAC3_RNG_BUFSIZ 16
Should comment somewhere (in the commit message, perhaps) why it is OK
to switch from a 16-byte buffer to 4-byte reads at a time.
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ arch/x86/x86/cpu_rng.c 25 Dec 2015 22:58:39 -0000
@@ -0,0 +1,166 @@
+/* $NetBSD: $ */
$NetBSD$
@@ -0,0 +1,166 @@
...
+static enum { CPU_RNG_NONE = 0,
+ CPU_RNG_RDRAND,
+ CPU_RNG_RDSEED,
+ CPU_RNG_VIA } cpu_rng_mode = CPU_RNG_NONE;
KNF and __read_mostly:
static enum {
CPU_RNG_NONE = 0,
CPU_RNG_RDRAND,
CPU_RNG_RDSEED,
CPU_RNG_VIA
} cpu_rng_mode __read_mostly = CPU_RNG_NONE;
+void
+cpu_rng_init(void)
+{
+ if (cpu_feature[5] & CPUID_SEF_RDSEED) {
KNF: blank line between declarations and statements in function body.
+ cpu_rng_mode = CPU_RNG_RDSEED;
+ aprint_normal("cpu_rng: RDSEED\n");
+ } else
+
+ if (cpu_feature[1] & CPUID2_RDRAND) {
KNF: no blank line between `else' and `if'.
+static inline size_t
+cpu_rng_rdrand(cpu_rng_t *out)
+{
+ uint8_t rndsts;
+#ifndef __x86_64__
Write positive conditionals, not negative ones:
#if defined(__i386__)
...
#elif defined(__x86_64__)
...
#else
#error Unknown x86 variant.
#endif
+ uint32_t outword[2] = out;
+ int i;
+
+ for (i = 0; i < 2; i++) {
+ __asm __volatile("rdrand %0; setc %1":"=r"(outword + i),
+ "=qm"(rndsts));
+ if (rndsts != 1) return 0;
+ }
Did you test-compile this? The initialization of outword makes no
sense. I think you meant this:
uint32_t outword[2];
unsigned i;
for (i = 0; i < 2; i++) {
__asm __volatile("rdrand %0; setc %1" : "=r"(outword[i]),
"=qm"(rndsts));
if (rndsts != 1)
return 0;
}
*out = outword[0] | ((uint64_t)outword[1] << 32);
It is tempting to write
uint32_t *outword = (uint32_t *)out;
and to avoid copying from the local buffer -- but that is incorrect
because it violates the strict-aliasing rules.
+#else
+ __asm __volatile("rdrand %0; setc %1":"=r"(out),
+ "=qm"(rndsts));
I assume you meant `*out' instead of `out' here. Otherwise you write
to the pointer itself, not to the memory that the pointer points to.
This is an example of an implementation bug that a generic statistical
test likely *will* catch, if you end up reading uninitialized garbage
off the stack -- such as the statistical tests that the rndsource code
will automatically apply. (So this bug is an argument in favour of
not bypassing rndsource.)
KNF: continuation line gets four spaces (or just place on same line --
it's short enough). I'd also put spaces around the : so it doesn't
get visually eaten by the quoted strings.
+ if (rndsts != 1) return 0;
KNF: line break after `if (...)'.
+size_t
+cpu_rng(cpu_rng_t *out)
+{
+ switch (cpu_rng_mode) {
+ case CPU_RNG_NONE:
KNF: indent the switch, don't indent the case.
--- arch/x86/x86/identcpu.c 13 Dec 2015 15:02:19 -0000 1.49
+++ arch/x86/x86/identcpu.c 25 Dec 2015 22:35:37 -0000
@@ -554,8 +554,15 @@ cpu_probe_c3(struct cpu_info *ci)
/* Actually do the enables. */
if (rng_enable) {
msr = rdmsr(MSR_VIA_RNG);
- wrmsr(MSR_VIA_RNG, msr | MSR_VIA_RNG_ENABLE);
+ /* C7 stepping 8 and subsequent CPUs have dual RNG */
+ if (model > 0xA || (model == 0xA && stepping > 0x7)) {
+ wrmsr(MSR_VIA_RNG, msr | MSR_VIA_RNG_ENABLE |
+ MSR_VIA_RNG_2NOISE);
+ } else {
+ wrmsr(MSR_VIA_RNG, msr | MSR_VIA_RNG_ENABLE);
+ }
}
Simplify:
msr = readmsr(MSR_VIA_RNG);
msr |= MSR_VIA_RNG_ENABLE;
if (model > 0xA || (model == 0xA && stepping > 0x7))
msr |= MSR_VIA_RNG_2NOISE;
wrmsr(MSR_VIA_RNG, msr);
What does MSR_VIA_RNG_2NOISE actually do? Can you add a documentation
citation?
--- kern/kern_rndq.c 29 Aug 2015 10:00:19 -0000 1.73
+++ kern/kern_rndq.c 25 Dec 2015 20:04:08 -0000
@@ -404,6 +408,31 @@ rnd_dv_estimate(krndsource_t *rs, uint32
return ret;
}
+#if defined(__HAVE_CPU_RNG)
+krndsource_t rnd_cpu_source;
+
+static void
+rnd_cpu_get(size_t bytes, void *priv)
+{
+ krndsource_t *cpusrcp = priv;
KNF: Tabs, not spaces, for indentation.
+ size_t entropy = 0, cnt = RND_POOLBITS / 2 / NBBY / sizeof(cpu_rng_t);
+ cpu_rng_t buf[cnt];
This isn't a variable-length array, but it looks like one at first
glance. I would really prefer to avoid this. I'd also be a bit more
comfortable writing the divisions as howmany: howmany(RND_POOLBITS/2,
NBBY*sizeof(cpu_rng_t)). Why /2? Why RND_POOLBITS and not, e.g., 256
or 512?
+#if defined(__HAVE_CPU_RNG)
+ {
+ cpu_rng_t test;
+
+ cpu_rng_init();
+ if (cpu_rng(&test)) {
+ rndsource_setcb(&rnd_cpu_source, rnd_cpu_get,
+ &rnd_cpu_source);
+ rnd_attach_source(&rnd_cpu_source, "cpurng",
+ RND_TYPE_RNG,
+ RND_FLAG_COLLECT_VALUE|
+ RND_FLAG_HASCB|RND_FLAG_HASENABLE);
Why attach only if it immediately works? A transient failure here
seems plausible enough to me, and there's no harm if it fails later.
KNF: four-space continuation lines.
Home |
Main Index |
Thread Index |
Old Index