Re: x86 CPU RNG support, take 2

   Date: Fri, 25 Dec 2015 18:27:50 -0500
   From: Thor Lancelot Simon <[email protected]>

   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.