[PATCH 1/1] MdePkg/BaseRngLib: Add a smoketest for RDRAND and check CPUID


Pedro Falcato
 

RDRAND has notoriously been broken many times over its lifespan.
Add a smoketest to RDRAND, in order to better sniff out potential
security concerns.

Also add a proper CPUID test in order to support older CPUs which may
not have it; it was previously being tested but then promptly ignored.

Signed-off-by: Pedro Falcato <pedro.falcato@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Liming Gao <gaoliming@...>
Cc: Zhiguang Liu <zhiguang.liu@...>
---
MdePkg/Library/BaseRngLib/Rand/RdRand.c | 81 ++++++++++++++++++++++---
1 file changed, 73 insertions(+), 8 deletions(-)

diff --git a/MdePkg/Library/BaseRngLib/Rand/RdRand.c b/MdePkg/Library/BaseRngLib/Rand/RdRand.c
index 070d41e2555f..ce9768955359 100644
--- a/MdePkg/Library/BaseRngLib/Rand/RdRand.c
+++ b/MdePkg/Library/BaseRngLib/Rand/RdRand.c
@@ -2,6 +2,7 @@
Random number generator services that uses RdRand instruction access
to provide high-quality random numbers.

+Copyright (c) 2022, Pedro Falcato. All rights reserved.<BR>
Copyright (c) 2021, NUVIA Inc. All rights reserved.<BR>
Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>

@@ -22,6 +23,70 @@ SPDX-License-Identifier: BSD-2-Clause-Patent

STATIC BOOLEAN mRdRandSupported;

+//
+// Intel SDM says 10 tries is good enough for reliable RDRAND usage.
+// We'll double this to 20 just to be safe, since a failure when testing
+// makes RDRAND unavailable.
+//
+#define RDRAND_RETRIES 20
+
+#define RDRAND_TEST_TRIES 10
+
+STATIC
+BOOLEAN
+TestRdRand (
+ VOID
+ )
+{
+ //
+ // Test for notoriously broken rdrand implementations that always return the same
+ // value, like the Zen 3 uarch (all-1s) or other several AMD families on suspend/resume (also all-1s).
+ // Note that this should be expanded to extensively test for other sorts of
+ // possible errata. This testing is quite naive.
+ //
+ UINT32 RandomNum;
+ BOOLEAN HasRandomNum;
+ UINT8 Idx;
+ UINT8 TestIteration;
+
+ HasRandomNum = FALSE;
+
+ for (TestIteration = 0; TestIteration < RDRAND_TEST_TRIES; TestIteration++) {
+ UINT32 Tmp;
+ //
+ // Note: We use a retry loop for rdrand. Normal users get this in BaseRng.c
+ // Any failure to get a random number will assume RDRAND does not work.
+ //
+ for (Idx = 0; Idx < RDRAND_RETRIES; Idx++) {
+ if (AsmRdRand32 (&Tmp)) {
+ break;
+ }
+ }
+
+ if (Idx == RDRAND_RETRIES) {
+ DEBUG ((DEBUG_ERROR, "BaseRngLib/x86: CPU BUG: Failed to get an RDRAND random number - disabling\n"));
+ return FALSE;
+ }
+
+ if (HasRandomNum) {
+ if (RandomNum != Tmp) {
+ //
+ // We got a different number, so take it the RNG works.
+ //
+ DEBUG ((DEBUG_INFO, "BaseRngLib/x86: RDRAND test complete.\n"));
+ return TRUE;
+ }
+ }
+
+ RandomNum = Tmp;
+ HasRandomNum = TRUE;
+ }
+
+ DEBUG ((DEBUG_ERROR, "BaseRngLib/x86: CPU BUG: RDRAND always returns the same result %x - disabling\n", RandomNum));
+
+ return FALSE;
+}
+
/**
The constructor function checks whether or not RDRAND instruction is supported
by the host hardware.
@@ -46,10 +111,13 @@ BaseRngLibConstructor (
// CPUID. A value of 1 indicates that processor support RDRAND instruction.
//
AsmCpuid (1, 0, 0, &RegEcx, 0);
- ASSERT ((RegEcx & RDRAND_MASK) == RDRAND_MASK);

mRdRandSupported = ((RegEcx & RDRAND_MASK) == RDRAND_MASK);

+ if (mRdRandSupported) {
+ mRdRandSupported = TestRdRand ();
+ }
+
return EFI_SUCCESS;
}

@@ -68,6 +136,7 @@ ArchGetRandomNumber16 (
OUT UINT16 *Rand
)
{
+ ASSERT (mRdRandSupported);
return AsmRdRand16 (Rand);
}

@@ -86,6 +155,7 @@ ArchGetRandomNumber32 (
OUT UINT32 *Rand
)
{
+ ASSERT (mRdRandSupported);
return AsmRdRand32 (Rand);
}

@@ -104,6 +174,7 @@ ArchGetRandomNumber64 (
OUT UINT64 *Rand
)
{
+ ASSERT (mRdRandSupported);
return AsmRdRand64 (Rand);
}

@@ -120,11 +191,5 @@ ArchIsRngSupported (
VOID
)
{
- /*
- Existing software depends on this always returning TRUE, so for
- now hard-code it.
-
- return mRdRandSupported;
- */
- return TRUE;
+ return mRdRandSupported;
}
--
2.38.1


Pedro Falcato
 

On Tue, Nov 22, 2022 at 2:19 PM Jason A. Donenfeld <Jason@...> wrote:
Considering our discussion an hour ago, I would have appreciated you
CC'ing me. I'm not subscribed to this list, and it's not on lore, so
this is a bit of a PITA to subscribe to.
 
Sorry about that, Cc'd you on v2.

Pedro


Jason A. Donenfeld
 

Hi,

On Tue, Nov 22, 2022 at 02:01:21PM +0000, Pedro Falcato wrote:
RDRAND has notoriously been broken many times over its lifespan.
Add a smoketest to RDRAND, in order to better sniff out potential
security concerns.

Also add a proper CPUID test in order to support older CPUs which may
not have it; it was previously being tested but then promptly ignored.

Signed-off-by: Pedro Falcato <pedro.falcato@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Liming Gao <gaoliming@...>
Cc: Zhiguang Liu <zhiguang.liu@...>
Considering our discussion an hour ago, I would have appreciated you
CC'ing me. I'm not subscribed to this list, and it's not on lore, so
this is a bit of a PITA to subscribe to.

+STATIC
+BOOLEAN
+TestRdRand (
+ VOID
+ )
+{
+ //
+ // Test for notoriously broken rdrand implementations that always return the same
+ // value, like the Zen 3 uarch (all-1s) or other several AMD families on suspend/resume (also all-1s).
+ // Note that this should be expanded to extensively test for other sorts of
+ // possible errata. This testing is quite naive.
+ //
The test that the kernel does is more robust. Maybe try doing that
instead?

void x86_init_rdrand(struct cpuinfo_x86 *c)
{
enum { SAMPLES = 8, MIN_CHANGE = 5 };
unsigned long sample, prev;
bool failure = false;
size_t i, changed;

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

for (changed = 0, i = 0; i < SAMPLES; ++i) {
if (!rdrand_long(&sample)) {
failure = true;
break;
}
changed += i && sample != prev;
prev = sample;
}
if (changed < MIN_CHANGE)
failure = true;

if (failure) {
clear_cpu_cap(c, X86_FEATURE_RDRAND);
clear_cpu_cap(c, X86_FEATURE_RDSEED);
pr_emerg("RDRAND is not reliable on this platform; disabling.\n");
}
}

Just copy and paste that and convert the Linuxisms to EDK2isms.

Jason