diff --git a/CHANGELOG.md b/CHANGELOG.md index 66a9a64f..6482821b 100644 --- a/CHANGELOG.md +++ b/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` 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..0f824444 100644 --- a/csrc/rdrand.cpp +++ b/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); diff --git a/csrc/test_rdrand.cpp b/csrc/test_rdrand.cpp index d9fc37fe..e3f8669d 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,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; }