Following Buggy AMD RdRand, The Linux Kernel Will Begin Sanity Checking Randomness At Boot Time

[phoronix]

Phoronix: Following Buggy AMD RdRand, The Linux Kernel Will Begin Sanity Checking Randomness At Boot Time

The Linux kernel will begin doing a basic sanity check of x86_64 CPUs with the RdRand instruction to see if it's at least returning "random looking" data otherwise warn the user at boot time. This stems from a recent issue where AMD's RdRand behavior with some hardware (particularly, buggy motherboards) could have borked RdRand issues...

http://www.phoronix.com/scan.php?pag...d-Sanity-Check

[tildearrow]

Here's another idea: also check for RdRand's behavior after waking up from suspend.

[johnp117]

Well, according to this post the code in question is executed on resume for all CPUs except the BSP (Bootstrap processor), so it should already do that (unless you have a single-core processor(?)). The question is more whether or not the WARN_ON_ONCE macro is reset on suspend, which I can't find any information on :/

A̶n̶d̶ ̶I̶ ̶m̶a̶y̶ ̶b̶e̶ ̶h̶a̶v̶i̶n̶g̶ ̶a̶ ̶b̶r̶a̶i̶n̶-̶f̶a̶r̶t̶ ̶r̶i̶g̶h̶t̶ ̶n̶o̶w̶,̶ ̶b̶u̶t̶ ̶d̶o̶e̶s̶n̶'̶t̶ ̶t̶h̶e̶ ̶c̶o̶d̶e̶,̶ ̶a̶s̶ ̶i̶t̶ ̶w̶a̶s̶ ̶c̶o̶m̶m̶i̶t̶t̶e̶d̶,̶ ̶a̶l̶w̶a̶y̶s̶ ̶i̶n̶c̶r̶e̶a̶s̶e̶ ̶`̶c̶h̶a̶n̶g̶e̶d̶`̶ ̶d̶u̶r̶i̶n̶g̶ ̶t̶h̶e̶ ̶f̶i̶r̶s̶t̶ ̶l̶o̶o̶p̶ ̶i̶t̶e̶r̶a̶t̶i̶o̶n̶ ̶o̶n̶c̶e̶ ̶a̶n̶d̶ ̶t̶h̶e̶r̶e̶f̶o̶r̶e̶ ̶d̶o̶e̶s̶n̶'̶t̶ ̶c̶a̶t̶c̶h̶ ̶a̶n̶ ̶u̶n̶c̶h̶a̶n̶g̶i̶n̶g̶ ̶r̶d̶r̶a̶n̶d̶?̶ ̶(̶u̶n̶l̶e̶s̶s̶ ̶t̶h̶e̶ ̶r̶a̶n̶d̶o̶m̶ ̶v̶a̶l̶u̶e̶ ̶j̶u̶s̶t̶ ̶h̶a̶p̶p̶e̶n̶s̶ ̶t̶o̶ ̶m̶a̶t̶c̶h̶ ̶t̶h̶e̶ ̶`̶p̶r̶e̶v̶`̶-̶a̶s̶s̶i̶g̶n̶e̶d̶ ̶u̶n̶i̶n̶i̶t̶i̶a̶l̶i̶z̶e̶d̶,̶ ̶d̶e̶c̶l̶a̶r̶a̶t̶i̶o̶n̶-̶t̶i̶m̶e̶ ̶v̶a̶l̶u̶e̶ ̶o̶f̶ ̶`̶t̶m̶p̶`̶)̶

[bug77]

Why single out RDRAND? Let's check MOV, JMP, CMP & co while we're at it

[euler271]

And I may be having a brain-fart right now, but doesn't the code, as it was committed, always increase `changed` during the first loop iteration once and therefore doesn't catch an unchanging rdrand? (unless the random value just happens to match the `prev`-assigned uninitialized, declaration-time value of `tmp`)

I think you're correct.

[kalomel]

And I may be having a brain-fart right now, but doesn't the code, as it was committed, always increase `changed` during the first loop iteration once and therefore doesn't catch an unchanging rdrand? (unless the random value just happens to match the `prev`-assigned uninitialized, declaration-time value of `tmp`)

Nice catch! One could also wonder why, in the first iteration, we think to get something meaningful out of comparing an uninitialized variable with an initialized variable...

[Marc.2377]

And I may be having a brain-fart right now, but doesn't the code, as it was committed, always increase `changed` during the first loop iteration once and therefore doesn't catch an unchanging rdrand? (unless the random value just happens to match the `prev`-assigned uninitialized, declaration-time value of `tmp`)

If you examine the code in the patch alone, you might conclude that. But consider the entire function after applying the patch:

void x86_init_rdrand(struct cpuinfo_x86 *c)
{
    unsigned int changed = 0;
    unsigned long tmp, prev;
    int i;

    if (!cpu_has(c, X86_FEATURE_RDRAND))
        return;

    for (i = 0; i < SANITY_CHECK_LOOPS; i++) {
        if (!rdrand_long(&tmp)) {
            clear_cpu_cap(c, X86_FEATURE_RDRAND);
            pr_warn_once("rdrand: disabled\n");
            return;
        }
    }

    /*
     * Stupid sanity-check whether RDRAND does *actually* generate
     * some at least random-looking data.
     */
    prev = tmp;
    for (i = 0; i < SANITY_CHECK_LOOPS; i++) {
        if (rdrand_long(&tmp)) {
            if (prev != tmp)
                changed++;

            prev = tmp;
        }
    }

    if (WARN_ON_ONCE(!changed))
        pr_emerg(
"RDRAND gives funky smelling output, might consider not using it by booting with \"nordrand\"");

}

Notice how 'tmp' is assigned to, at line 12 in the above code.

(Edit: formatting; had to replace tabs with spaces, sorry!)

[arQon]

Except that code's still not very good, unless I'm missing something, because all it's looking for is any change ever, but it still runs SANITY_CHECK_LOOPS instead of breaking out once it finds one...

[stormcrow]

Except that code's still not very good, unless I'm missing something, because all it's looking for is any change ever, but it still runs SANITY_CHECK_LOOPS instead of breaking out once it finds one...

It really depends on what you're looking for in the result. If you're looking for a broken register that repeats the same result over and over again, that's sufficient. If you're looking for an easily guessed weakly pseudorandom result you would indeed need to do one of the various methods for mathematical verification over a sufficient number of results. While the former might be good to check for specific type of brokenness in a random unknown CPU with RDRAND instruction, it's not sufficient to guarantee sufficient entropy to compute something where you want reasonably strong cryptographic computations (initial SSH private keys, GPG keys, etc). At that point you're getting into the ongoing argument over the behavior of syscall getrandom() in early boot stages when there's insufficient entropy to guarantee non-deterministic cryptographic output.

So long as the output does change, that should be enough where all you want is a different result each time you query it in a large number of needs eg: the problem that restarted the getrandom() discussion in the first place -- GDM causing indefinite blocked boot progression due to over engineering its X connection token generation could potentially use a working RDRAND instead of getrandom().

[johnp117]

Marc.2377 Thanks, you're right! I even searched for `tmp` and didn't see anything... Well, it was late yesterday ^^