From 3e4ce5eaa12080222f0ac7270371f89eb7182193 Mon Sep 17 00:00:00 2001 From: Greg Rubin Date: Wed, 30 Oct 2019 19:56:47 +0000 Subject: [PATCH 1/6] 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. --- CHANGELOG.md | 5 ++++ csrc/env.h | 4 +++ csrc/rdrand.cpp | 76 +++++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 79 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 66a9a64f..313c1bc6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Changelog +## 1.2.1 (Unreleased) + +### Patches +* Detects stuck AMD Ryzen RDRAND and correctly treats as an error + ## 1.2.0 ### Improvements diff --git a/csrc/env.h b/csrc/env.h index b9c5d280..03e4db3a 100644 --- a/csrc/env.h +++ b/csrc/env.h @@ -23,6 +23,10 @@ #include #endif +#ifndef UINT64_MAX +#define UINT64_MAX (~((uint64_t) 0)) +#endif + namespace AmazonCorrettoCryptoProvider { void capture_trace(std::vector &trace) COLD; void format_trace(std::ostringstream &, const std::vector &trace) COLD; diff --git a/csrc/rdrand.cpp b/csrc/rdrand.cpp index 6fe216a4..9456f969 100644 --- a/csrc/rdrand.cpp +++ b/csrc/rdrand.cpp @@ -27,11 +27,24 @@ 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" + +// We reuse these same macros for the test results +// of our RNGs stored in rng_tested #define CPUID_PROBE_DONE (1 << 0) #define CPUID_HAS_RDRAND (1 << 1) #define CPUID_HAS_RDSEED (1 << 2) volatile static uint32_t cpuid_info = 0; +volatile static uint32_t rng_tested = 0; static inline int getCpuid( unsigned int level, @@ -111,13 +124,37 @@ 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) : "cc" // clobbers condition codes ); + // Some AMD CPUs will find that RDRAND "sticks" on all 1s but still reports success. + // If we encounter this suspicious value (a 1/2^64 chance) we'll generate a second + // value and compare it to the first. If they are equal (indicating it is stuck) then + // we'll return an error. Else, we'll return the first value. + // This reduces the risk of a false positive to 1/2^128 (negligible) and avoids biasing + // the results at all as all 1s can still be returned by a valid RNG. + // We also check for 0 as some old/non-standard systems may use that as an error value. + + if (likely(success)) { + if (unlikely(*out == UINT64_MAX || *out == 0)) { + uint64_t tmp; + __asm__ __volatile__( + ASM_RDRAND_RCX + "setc %%al\n" // rax = 1 if success, 0 if fail + : "=c" (tmp), "=a" (success) + : "c" (0), "a" (0) + : "cc" // clobbers condition codes + ); + if (tmp == *out) { + *out = 0; + success = 0; + } + } + } return success; } @@ -126,7 +163,9 @@ bool rng_rdseed(uint64_t *out) { return (*hook_rdseed)(out); } - if (unlikely(!supportsRdSeed())) { + // We don't call supportsRdSeed() here to avoid circular dependencies + // during RNG testing. + if (unlikely(!(get_cpuinfo() & CPUID_HAS_RDSEED))) { // We'll allow rdseed_fallback to poll rdrand instead *out = 0; return false; @@ -134,7 +173,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 +193,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) @@ -332,12 +371,37 @@ bool rd_into_buf(bool (*rng)(uint64_t *), unsigned char *buf, int len) { // C++ Exported methods: +void testRngs() COLD NOINLINE; +void testRngs() { + uint32_t result = CPUID_PROBE_DONE; + uint64_t scratch = 0; // No need to actually read this + + if (rng_rdrand(&scratch)) { + result |= CPUID_HAS_RDRAND; + } + + if (rng_rdseed(&scratch)) { + result |= CPUID_HAS_RDSEED; + } + rng_tested = result; +} + +uint32_t get_rng_tests() { + uint32_t info = rng_tested; + + if (unlikely(!info)) { + testRngs(); + info = rng_tested; + } + return info; +} + bool supportsRdRand() { - return !!(get_cpuinfo() & CPUID_HAS_RDRAND); + return !!(get_cpuinfo() & CPUID_HAS_RDRAND) && !!(get_rng_tests() & CPUID_HAS_RDRAND); } bool supportsRdSeed() { - return !!(get_cpuinfo() & CPUID_HAS_RDSEED); + return !!(get_cpuinfo() & CPUID_HAS_RDSEED) && !!(get_rng_tests() & CPUID_HAS_RDSEED); } bool rdseed(unsigned char *buf, int len) { From ecc36b7d2a3d08db59ef07e043cbae953ddcbd25 Mon Sep 17 00:00:00 2001 From: Greg Rubin Date: Wed, 30 Oct 2019 21:18:51 +0000 Subject: [PATCH 2/6] Move CHANGELOG info back to 1.2.0 --- CHANGELOG.md | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 313c1bc6..6482821b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,15 +1,13 @@ # Changelog -## 1.2.1 (Unreleased) - -### Patches -* Detects stuck AMD Ryzen RDRAND and correctly treats as an error - -## 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` From 845af993c78a3b2c64c54261b58904bf373bec6c Mon Sep 17 00:00:00 2001 From: Greg Rubin Date: Wed, 30 Oct 2019 22:22:18 +0000 Subject: [PATCH 3/6] 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. --- csrc/rdrand.cpp | 89 +++++++++++++------------------------------- csrc/test_rdrand.cpp | 32 +++++++++++++++- 2 files changed, 57 insertions(+), 64 deletions(-) diff --git a/csrc/rdrand.cpp b/csrc/rdrand.cpp index 9456f969..75476663 100644 --- a/csrc/rdrand.cpp +++ b/csrc/rdrand.cpp @@ -37,14 +37,11 @@ namespace AmazonCorrettoCryptoProvider { // PAUSE instruction. REP NOP #define ASM_REP_NOP ".byte 0xf3, 0x90\n" -// We reuse these same macros for the test results -// of our RNGs stored in rng_tested #define CPUID_PROBE_DONE (1 << 0) #define CPUID_HAS_RDRAND (1 << 1) #define CPUID_HAS_RDSEED (1 << 2) volatile static uint32_t cpuid_info = 0; -volatile static uint32_t rng_tested = 0; static inline int getCpuid( unsigned int level, @@ -131,30 +128,6 @@ bool rng_rdrand(uint64_t *out) { : "cc" // clobbers condition codes ); - // Some AMD CPUs will find that RDRAND "sticks" on all 1s but still reports success. - // If we encounter this suspicious value (a 1/2^64 chance) we'll generate a second - // value and compare it to the first. If they are equal (indicating it is stuck) then - // we'll return an error. Else, we'll return the first value. - // This reduces the risk of a false positive to 1/2^128 (negligible) and avoids biasing - // the results at all as all 1s can still be returned by a valid RNG. - // We also check for 0 as some old/non-standard systems may use that as an error value. - - if (likely(success)) { - if (unlikely(*out == UINT64_MAX || *out == 0)) { - uint64_t tmp; - __asm__ __volatile__( - ASM_RDRAND_RCX - "setc %%al\n" // rax = 1 if success, 0 if fail - : "=c" (tmp), "=a" (success) - : "c" (0), "a" (0) - : "cc" // clobbers condition codes - ); - if (tmp == *out) { - *out = 0; - success = 0; - } - } - } return success; } @@ -163,9 +136,7 @@ bool rng_rdseed(uint64_t *out) { return (*hook_rdseed)(out); } - // We don't call supportsRdSeed() here to avoid circular dependencies - // during RNG testing. - if (unlikely(!(get_cpuinfo() & CPUID_HAS_RDSEED))) { + if (unlikely(!supportsRdSeed())) { // We'll allow rdseed_fallback to poll rdrand instead *out = 0; return false; @@ -307,13 +278,30 @@ bool rdseed_fallback(uint64_t *dest) { bool rng_retry_rdrand(uint64_t *dest) { int tries = 10; - + uint64_t oldValue = 13; // Must not be initialized to 0 or UINT64_MAX. Anything else is fine. do { - if (likely(rng_rdrand(dest))) { - return true; - } - - pause_and_decrement(tries); + if (likely(rng_rdrand(dest))) { + // Some AMD CPUs will find that RDRAND "sticks" on all 1s but still reports success. + // If we encounter this suspicious value (a 1/2^64 chance) we'll generate a second + // value and compare it to the first. If they are equal (indicating it is stuck) then + // we'll return an error. Else, we'll return the first value. + // This reduces the risk of a false positive to 1/2^128 (negligible) and avoids biasing + // the results at all as all 1s can still be returned by a valid RNG. + // We also check for 0 as some old/non-standard systems may use that as an error value. + + if (*dest == UINT64_MAX || *dest == 0) { + if (*dest == oldValue) { + // We've gotten the same suspicious value twice in a row. We're likely stuck + return false; + } else { + // This is the first time we've seen this suspicious value. Save it and loop again. + oldValue = *dest; + } + } else { + return true; + } + } + pause_and_decrement(tries); } while(tries); return false; @@ -371,37 +359,12 @@ bool rd_into_buf(bool (*rng)(uint64_t *), unsigned char *buf, int len) { // C++ Exported methods: -void testRngs() COLD NOINLINE; -void testRngs() { - uint32_t result = CPUID_PROBE_DONE; - uint64_t scratch = 0; // No need to actually read this - - if (rng_rdrand(&scratch)) { - result |= CPUID_HAS_RDRAND; - } - - if (rng_rdseed(&scratch)) { - result |= CPUID_HAS_RDSEED; - } - rng_tested = result; -} - -uint32_t get_rng_tests() { - uint32_t info = rng_tested; - - if (unlikely(!info)) { - testRngs(); - info = rng_tested; - } - return info; -} - bool supportsRdRand() { - return !!(get_cpuinfo() & CPUID_HAS_RDRAND) && !!(get_rng_tests() & CPUID_HAS_RDRAND); + return !!(get_cpuinfo() & CPUID_HAS_RDRAND); } bool supportsRdSeed() { - return !!(get_cpuinfo() & CPUID_HAS_RDSEED) && !!(get_rng_tests() & CPUID_HAS_RDSEED); + return !!(get_cpuinfo() & CPUID_HAS_RDSEED); } bool rdseed(unsigned char *buf, int len) { diff --git a/csrc/test_rdrand.cpp b/csrc/test_rdrand.cpp index d9fc37fe..0ae4097d 100644 --- a/csrc/test_rdrand.cpp +++ b/csrc/test_rdrand.cpp @@ -10,6 +10,7 @@ #undef NDEBUG +#include "env.h" #include "rdrand.h" #include "config.h" #include @@ -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,7 +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; } From 9bb1811bdcd7b0e80cc048f6b38eb5f08439acf5 Mon Sep 17 00:00:00 2001 From: Greg Rubin Date: Wed, 30 Oct 2019 22:26:35 +0000 Subject: [PATCH 4/6] Indentation fix --- csrc/rdrand.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/csrc/rdrand.cpp b/csrc/rdrand.cpp index 75476663..f14a0ddf 100644 --- a/csrc/rdrand.cpp +++ b/csrc/rdrand.cpp @@ -144,7 +144,7 @@ bool rng_rdseed(uint64_t *out) { bool success; __asm__ __volatile__( - ASM_RDSEED_RCX + ASM_RDSEED_RCX "setc %%al\n" // rax = 1 if success, 0 if fail : "=c" (*out), "=a" (success) : "c" (0), "a" (0) From 06bf2a048ba3f2bf15b3db866766ed9f33c07c73 Mon Sep 17 00:00:00 2001 From: Greg Rubin Date: Thu, 31 Oct 2019 16:51:50 +0000 Subject: [PATCH 5/6] Just discard suspicious RDRAND values with no fixup Also re-add new-line in test_rdrand.cpp --- csrc/rdrand.cpp | 42 +++++++++++++++++++----------------------- csrc/test_rdrand.cpp | 1 + 2 files changed, 20 insertions(+), 23 deletions(-) diff --git a/csrc/rdrand.cpp b/csrc/rdrand.cpp index f14a0ddf..61699f93 100644 --- a/csrc/rdrand.cpp +++ b/csrc/rdrand.cpp @@ -278,30 +278,26 @@ bool rdseed_fallback(uint64_t *dest) { bool rng_retry_rdrand(uint64_t *dest) { int tries = 10; - uint64_t oldValue = 13; // Must not be initialized to 0 or UINT64_MAX. Anything else is fine. + do { - if (likely(rng_rdrand(dest))) { - // Some AMD CPUs will find that RDRAND "sticks" on all 1s but still reports success. - // If we encounter this suspicious value (a 1/2^64 chance) we'll generate a second - // value and compare it to the first. If they are equal (indicating it is stuck) then - // we'll return an error. Else, we'll return the first value. - // This reduces the risk of a false positive to 1/2^128 (negligible) and avoids biasing - // the results at all as all 1s can still be returned by a valid RNG. - // We also check for 0 as some old/non-standard systems may use that as an error value. - - if (*dest == UINT64_MAX || *dest == 0) { - if (*dest == oldValue) { - // We've gotten the same suspicious value twice in a row. We're likely stuck - return false; - } else { - // This is the first time we've seen this suspicious value. Save it and loop again. - oldValue = *dest; - } - } else { - return true; - } - } - pause_and_decrement(tries); + if (likely(rng_rdrand(dest))) { + // 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 (unlikely(*dest != UINT64_MAX || *dest != 0)) { + return true; + } + } + + pause_and_decrement(tries); } while(tries); return false; diff --git a/csrc/test_rdrand.cpp b/csrc/test_rdrand.cpp index 0ae4097d..e3f8669d 100644 --- a/csrc/test_rdrand.cpp +++ b/csrc/test_rdrand.cpp @@ -251,6 +251,7 @@ int main() { RUNTEST(when_rdrand_broken_rdseed_works_eventually); RUNTEST(when_rdseed_broken_rdrand_reduction_used); RUNTEST(when_rdrand_stuck_failure_returned); + return success ? 0 : 1; } From 47df9b20b1879325f94ec5de4fb942bcd391f73b Mon Sep 17 00:00:00 2001 From: Greg Rubin Date: Thu, 31 Oct 2019 17:32:18 +0000 Subject: [PATCH 6/6] Typo fix --- csrc/rdrand.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/csrc/rdrand.cpp b/csrc/rdrand.cpp index 61699f93..0f824444 100644 --- a/csrc/rdrand.cpp +++ b/csrc/rdrand.cpp @@ -292,7 +292,7 @@ bool rng_retry_rdrand(uint64_t *dest) { // 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 (unlikely(*dest != UINT64_MAX || *dest != 0)) { + if (likely(*dest != UINT64_MAX && *dest != 0)) { return true; } }