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

[PATCH v2 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.

Many thanks to Jason Donenfeld for relicensing his linux RDRAND detection
code to MIT and the public domain.

>On Tue, Nov 22, 2022 at 2:21 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
  <..>
>    I (re)wrote that function in Linux. I hereby relicense it as MIT, and
>    also place it into public domain. Do with it what you will now.
>
>    Jason

Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Jason A. Donenfeld <Jason@zx2c4.com>
---
 MdePkg/Library/BaseRngLib/Rand/RdRand.c | 83 ++++++++++++++++++++++---
 1 file changed, 75 insertions(+), 8 deletions(-)

diff --git a/MdePkg/Library/BaseRngLib/Rand/RdRand.c b/MdePkg/Library/BaseRngLib/Rand/RdRand.c
index 070d41e2555f..c8f837c315f1 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,72 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 STATIC BOOLEAN  mRdRandSupported;
 
+//
+// Intel SDM says 10 tries is good enough for reliable RDRAND usage.
+//
+#define RDRAND_RETRIES  10
+
+#define RDRAND_TEST_SAMPLES  8
+
+#define RDRAND_MIN_CHANGE  5
+
+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.
+  //
+
+  //
+  // Testing algorithm inspired by linux's arch/x86/kernel/cpu/rdrand.c:x86_init_rdrand
+  // as relicensed by the author, Jason Donenfeld, in the EDK2 mailing list.
+  // As is, the algorithm samples rdrand $RDRAND_TEST_SAMPLES times and expects
+  // a different result $RDRAND_MIN_CHANGE times for reliable RDRAND usage.
+  //
+  UINT32  Prev;
+  UINT8   Idx;
+  UINT8   TestIteration;
+  UINT32  Changed;
+
+  Changed = 0;
+
+  for (TestIteration = 0; TestIteration < RDRAND_TEST_SAMPLES; TestIteration++) {
+    UINT32  Sample;
+    //
+    // 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 (&Sample)) {
+        break;
+      }
+    }
+
+    if (Idx == RDRAND_RETRIES) {
+      DEBUG ((DEBUG_ERROR, "BaseRngLib/x86: CPU BUG: Failed to get an RDRAND random number - disabling\n"));
+      return FALSE;
+    }
+
+    if (TestIteration != 0) {
+      Changed += Sample != Prev;
+    }
+
+    Prev = Sample;
+  }
+
+  if (Changed < RDRAND_MIN_CHANGE) {
+    DEBUG ((DEBUG_ERROR, "BaseRngLib/x86: CPU BUG: RDRAND not reliable - disabling\n"));
+    return FALSE;
+  }
+
+  return TRUE;
+}
+
 /**
   The constructor function checks whether or not RDRAND instruction is supported
   by the host hardware.
@@ -46,10 +113,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 +138,7 @@ ArchGetRandomNumber16 (
   OUT     UINT16  *Rand
   )
 {
+  ASSERT (mRdRandSupported);
   return AsmRdRand16 (Rand);
 }
 
@@ -86,6 +157,7 @@ ArchGetRandomNumber32 (
   OUT     UINT32  *Rand
   )
 {
+  ASSERT (mRdRandSupported);
   return AsmRdRand32 (Rand);
 }
 
@@ -104,6 +176,7 @@ ArchGetRandomNumber64 (
   OUT     UINT64  *Rand
   )
 {
+  ASSERT (mRdRandSupported);
   return AsmRdRand64 (Rand);
 }
 
@@ -120,11 +193,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

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

[Jason A. Donenfeld]

On Tue, Nov 22, 2022 at 4:32 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
> +  // Testing algorithm inspired by linux's arch/x86/kernel/cpu/rdrand.c:x86_init_rdrand
> +  // as relicensed by the author, Jason Donenfeld, in the EDK2 mailing list.
> +  // As is, the algorithm samples rdrand $RDRAND_TEST_SAMPLES times and expects
> +  // a different result $RDRAND_MIN_CHANGE times for reliable RDRAND usage.

You don't need to pepper my name all over the source. :)

> +  for (TestIteration = 0; TestIteration < RDRAND_TEST_SAMPLES; TestIteration++) {
> +    UINT32  Sample;
> +    //
> +    // 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 (&Sample)) {

The linux code will use a 64bit value on 64bit machines. I suggest you
do the same here -- use native word size. I think EFI calls this a
"UINTN".

Jason

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

[Pedro Falcato]

[-- Attachment #1: Type: text/plain, Size: 1440 bytes --]

On Tue, Nov 22, 2022 at 3:39 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:

> On Tue, Nov 22, 2022 at 4:32 PM Pedro Falcato <pedro.falcato@gmail.com>
> wrote:
> > +  // Testing algorithm inspired by linux's
> arch/x86/kernel/cpu/rdrand.c:x86_init_rdrand
> > +  // as relicensed by the author, Jason Donenfeld, in the EDK2 mailing
> list.
> > +  // As is, the algorithm samples rdrand $RDRAND_TEST_SAMPLES times and
> expects
> > +  // a different result $RDRAND_MIN_CHANGE times for reliable RDRAND
> usage.
>
> You don't need to pepper my name all over the source. :)
>

I just wanted to properly credit you :) If you're not okay with it I can
remove it in a v3.

> +  for (TestIteration = 0; TestIteration < RDRAND_TEST_SAMPLES;
> TestIteration++) {
> > +    UINT32  Sample;
> > +    //
> > +    // 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 (&Sample)) {
>
> The linux code will use a 64bit value on 64bit machines. I suggest you
> do the same here -- use native word size. I think EFI calls this a
> "UINTN".
>

Hmm, do you reckon it makes a difference? I'm not intimately familiar with
HWRNG internals. Unfortunately there's no AsmRdRandUintn
so this would take some per-bitness #define's which... yeah, I'd rather not.

Pedro

[-- Attachment #2: Type: text/html, Size: 2135 bytes --]

Re: [edk2-devel] [PATCH v2 1/1] MdePkg/BaseRngLib: Add a smoketest for RDRAND and check CPUID

[Michael D Kinney]

[-- Attachment #1: Type: text/plain, Size: 2049 bytes --]

Hi Pedro,

Pointers to external content that were used to create the code
change can be captured in the BZ and the commit message.

Thanks,

Mike

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Pedro Falcato
Sent: Tuesday, November 22, 2022 7:56 AM
To: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>
Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg/BaseRngLib: Add a smoketest for RDRAND and check CPUID

On Tue, Nov 22, 2022 at 3:39 PM Jason A. Donenfeld <Jason@zx2c4.com<mailto:Jason@zx2c4.com>> wrote:
On Tue, Nov 22, 2022 at 4:32 PM Pedro Falcato <pedro.falcato@gmail.com<mailto:pedro.falcato@gmail.com>> wrote:
> +  // Testing algorithm inspired by linux's arch/x86/kernel/cpu/rdrand.c:x86_init_rdrand
> +  // as relicensed by the author, Jason Donenfeld, in the EDK2 mailing list.
> +  // As is, the algorithm samples rdrand $RDRAND_TEST_SAMPLES times and expects
> +  // a different result $RDRAND_MIN_CHANGE times for reliable RDRAND usage.

You don't need to pepper my name all over the source. :)

I just wanted to properly credit you :) If you're not okay with it I can remove it in a v3.

> +  for (TestIteration = 0; TestIteration < RDRAND_TEST_SAMPLES; TestIteration++) {
> +    UINT32  Sample;
> +    //
> +    // 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 (&Sample)) {

The linux code will use a 64bit value on 64bit machines. I suggest you
do the same here -- use native word size. I think EFI calls this a
"UINTN".

Hmm, do you reckon it makes a difference? I'm not intimately familiar with HWRNG internals. Unfortunately there's no AsmRdRandUintn
so this would take some per-bitness #define's which... yeah, I'd rather not.

Pedro


[-- Attachment #2: Type: text/html, Size: 42804 bytes --]

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

[Jason A. Donenfeld]

On Tue, Nov 22, 2022 at 4:56 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
>
> On Tue, Nov 22, 2022 at 3:39 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>>
>> On Tue, Nov 22, 2022 at 4:32 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
>> > +  // Testing algorithm inspired by linux's arch/x86/kernel/cpu/rdrand.c:x86_init_rdrand
>> > +  // as relicensed by the author, Jason Donenfeld, in the EDK2 mailing list.
>> > +  // As is, the algorithm samples rdrand $RDRAND_TEST_SAMPLES times and expects
>> > +  // a different result $RDRAND_MIN_CHANGE times for reliable RDRAND usage.
>>
>> You don't need to pepper my name all over the source. :)
>
>
> I just wanted to properly credit you :) If you're not okay with it I can remove it in a v3.

Yes, please remove.

 >> > +  for (TestIteration = 0; TestIteration < RDRAND_TEST_SAMPLES;
TestIteration++) {
>> > +    UINT32  Sample;
>> > +    //
>> > +    // 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 (&Sample)) {
>>
>> The linux code will use a 64bit value on 64bit machines. I suggest you
>> do the same here -- use native word size. I think EFI calls this a
>> "UINTN".
>
>
> Hmm, do you reckon it makes a difference?

Yes, it does. Please account for this. Use an ifdef or something if
you need. It's not very hard.

Jason