Add check for broken Ryzen RDRAND (#67)

* Add check for broken Ryzen RDRAND

Some AMD Ryzen CPUs get into a bad state where
the output of RDRAND becomes "stuck" on a value
of all 1s (e.g., every bit in the output is a one).
Unfortunately, it doesn't correctly set the carry flag
to indicate an RNG failure.

This change detects this case and handles it as a
standard RNG error.

* Move CHANGELOG info back to 1.2.0

* Move stuck RDRAND to rng_retry_rdrand

This moves where we check for RDRAND being stuck to a better
location and also adds tests for this logic. These tests only
run with the 'coverage' target. I have validated that the
tests work by backing out the fixes and manually verifying
the failure.

* Indentation fix

* Just discard suspicious RDRAND values with no fixup

Also re-add new-line in test_rdrand.cpp

* Typo fix

CHANGELOG.md

@@ -1,10 +1,13 @@
 # Changelog
 
-## 1.2.0
+## 1.2.0 (Unreleased)
 
 ### Improvements
 * Now uses [OpenSSL 1.1.1.d](https://www.openssl.org/source/openssl-1.1.1d.tar.gz)
 
+### Patches
+* Detects stuck AMD Ryzen RDRAND and correctly treats as an error
+
 ### Maintenance
 * Add prefix to test output lines indicating if suite will fail.
 * Now colored output from tests can be disabled by setting the environment variable `ACCP_TEST_COLOR` to `false`

csrc/env.h

@@ -23,6 +23,10 @@
 #include <openssl/err.h>
 #endif
 
+#ifndef UINT64_MAX
+#define UINT64_MAX (~((uint64_t) 0))
+#endif
+
 namespace AmazonCorrettoCryptoProvider {
     void capture_trace(std::vector<void *> &trace) COLD;
     void format_trace(std::ostringstream &, const std::vector<void *> &trace) COLD;

csrc/rdrand.cpp

@@ -27,6 +27,16 @@ using namespace AmazonCorrettoCryptoProvider;
 
 namespace AmazonCorrettoCryptoProvider {
 
+// Some inline machine-code which cannot always be represented
+// by standard inline assembly mnemonics.
+
+// rdrand %rcx
+#define ASM_RDRAND_RCX ".byte 0x48, 0x0f, 0xc7, 0xf1\n"
+// rdseed %rcx
+#define ASM_RDSEED_RCX ".byte 0x48, 0x0f, 0xc7, 0xf9\n"
+// PAUSE instruction. REP NOP
+#define ASM_REP_NOP ".byte 0xf3, 0x90\n"
+
 #define CPUID_PROBE_DONE (1 << 0)
 #define CPUID_HAS_RDRAND (1 << 1)
 #define CPUID_HAS_RDSEED (1 << 2)
@@ -111,7 +121,7 @@ bool rng_rdrand(uint64_t *out) {
 
     bool success = 0;
     __asm__ __volatile__(
-        ".byte 0x48, 0x0f, 0xc7, 0xf1\n" // RDRAND %rcx
+        ASM_RDRAND_RCX
         "setc %%al\n" // rax = 1 if success, 0 if fail
         : "=c" (*out), "=a" (success)
         : "c" (0), "a" (0)
@@ -134,7 +144,7 @@ bool rng_rdseed(uint64_t *out) {
 
     bool success;
     __asm__ __volatile__(
-        ".byte 0x48, 0x0f, 0xc7, 0xf9\n" // RDSEED %rcx
+        ASM_RDSEED_RCX
         "setc %%al\n" // rax = 1 if success, 0 if fail
         : "=c" (*out), "=a" (success)
         : "c" (0), "a" (0)
@@ -154,7 +164,7 @@ void pause_and_decrement(int &counter) {
     //
     // Unfortunately our ancient compilers don't like the PAUSE instruction - even when entered
     // as "rep nop", so we need to use raw machine code here as well.
-        ".byte 0xf3, 0x90\n" // PAUSE instruction (REP NOP)
+        ASM_REP_NOP // PAUSE instruction
         "dec %0"   // decrement retry counter
         // prevent loop unrolling by hiding the loop decrement from the compiler
         : "+r" (counter)
@@ -271,7 +281,20 @@ bool rng_retry_rdrand(uint64_t *dest) {
 
     do {
         if (likely(rng_rdrand(dest))) {
-            return true;
+            // Some AMD CPUs will find that RDRAND "sticks" on all 1s but still reports success.
+            // Some other very old CPUs use all 0s as an error condition while still reporting success.
+            // If we encounter either of these suspicious values (a 1/2^63 chance) we'll treat them as
+            // a failure and generate a new value.
+            //
+            // In the future we could add CPUID checks to detect processors with these known bugs,
+            // however it does not appear worth it. The entropy loss is negligible and the
+            // corresponding likelihood that a healthy CPU generates either of these values is also
+            // negligible (1/2^63). Finally, adding processor specific logic would greatly
+            // increase the complexity and would cause us to "miss" any unknown processors with
+            // similar bugs.
+            if (likely(*dest != UINT64_MAX && *dest != 0)) {
+                return true;
+            }
         }
 
         pause_and_decrement(tries);

csrc/test_rdrand.cpp

@@ -10,6 +10,7 @@
 
 #undef NDEBUG
 
+#include "env.h"
 #include "rdrand.h"
 #include "config.h"
 #include <stdlib.h>
@@ -40,6 +41,16 @@ bool rng_alternating(uint64_t *out) {
     return true;
 }
 
+bool rng_stuck_zero(uint64_t *out) {
+    *out = 0;
+    return true;
+}
+
+bool rng_stuck_ff(uint64_t *out) {
+    *out = UINT64_MAX;
+    return true;
+}
+
 bool test_succeeded = true;
 
 #define TEST_ASSERT(x) do { \
@@ -196,6 +207,25 @@ void when_rdseed_broken_rdrand_reduction_used() {
     TEST_ASSERT(!memcmp(expected, buf, sizeof(buf)));
 }
 
+void when_rdrand_stuck_failure_returned() {
+    static const uint8_t expected[32] = { 0 };
+    static uint8_t buf[32];
+
+    hook_rdrand = rng_stuck_zero;
+
+    memset(buf, 1, sizeof(buf));
+
+    TEST_ASSERT(!rdrand(buf, sizeof(buf)));
+    TEST_ASSERT(!memcmp(expected, buf, sizeof(buf)));
+
+    memset(buf, 1, sizeof(buf));
+
+    hook_rdrand = rng_stuck_ff;
+
+    TEST_ASSERT(!rdrand(buf, sizeof(buf)));
+    TEST_ASSERT(!memcmp(expected, buf, sizeof(buf)));
+}
+
 } // anon namespace
 
 #define RUNTEST(name) do { \
@@ -220,6 +250,7 @@ int main() {
     RUNTEST(when_rng_fails_late_buffer_is_still_cleared);
     RUNTEST(when_rdrand_broken_rdseed_works_eventually);
     RUNTEST(when_rdseed_broken_rdrand_reduction_used);
+    RUNTEST(when_rdrand_stuck_failure_returned);
 
     return success ? 0 : 1;
 }