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

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

Testing algorithm inspired by linux's arch/x86/kernel/cpu/rdrand.c
:x86_init_rdrand() per commit 049f9ae9..

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

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4163

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>
---
v4: Added a doxygen comment to the TestRdRand() function
v3: Addressed mailing list comments
v2: Replaced the original v1 naive detection with a better, although still not great, detection. 
 MdePkg/Library/BaseRngLib/Rand/RdRand.c | 99 +++++++++++++++++++++++--
 1 file changed, 91 insertions(+), 8 deletions(-)

diff --git a/MdePkg/Library/BaseRngLib/Rand/RdRand.c b/MdePkg/Library/BaseRngLib/Rand/RdRand.c
index 070d41e2555f..ff99436dbbbd 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,88 @@ 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
+
+//
+// Add a define for native-word RDRAND, just for the test.
+//
+#ifdef MDE_CPU_X64
+#define AsmRdRand  AsmRdRand64
+#else
+#define AsmRdRand  AsmRdRand32
+#endif
+
+/**
+  Tests RDRAND for broken implementations.
+
+  @retval TRUE         RDRAND is reliable (and hopefully safe).
+  @retval FALSE        RDRAND is unreliable and should be disabled, despite CPUID.
+
+**/
+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.
+  //
+
+  //
+  // Our algorithm samples rdrand $RDRAND_TEST_SAMPLES times and expects
+  // a different result $RDRAND_MIN_CHANGE times for reliable RDRAND usage.
+  //
+  UINTN   Prev;
+  UINT8   Idx;
+  UINT8   TestIteration;
+  UINT32  Changed;
+
+  Changed = 0;
+
+  for (TestIteration = 0; TestIteration < RDRAND_TEST_SAMPLES; TestIteration++) {
+    UINTN  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 (AsmRdRand (&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;
+}
+
+#undef AsmRdRand
+
 /**
   The constructor function checks whether or not RDRAND instruction is supported
   by the host hardware.
@@ -46,10 +129,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 +154,7 @@ ArchGetRandomNumber16 (
   OUT     UINT16  *Rand
   )
 {
+  ASSERT (mRdRandSupported);
   return AsmRdRand16 (Rand);
 }
 
@@ -86,6 +173,7 @@ ArchGetRandomNumber32 (
   OUT     UINT32  *Rand
   )
 {
+  ASSERT (mRdRandSupported);
   return AsmRdRand32 (Rand);
 }
 
@@ -104,6 +192,7 @@ ArchGetRandomNumber64 (
   OUT     UINT64  *Rand
   )
 {
+  ASSERT (mRdRandSupported);
   return AsmRdRand64 (Rand);
 }
 
@@ -120,11 +209,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

回复: [edk2-devel] [PATCH v4 1/1] MdePkg/BaseRngLib: Add a smoketest for RDRAND and check CPUID

[gaoliming]

Pedro:
  To keep the same behavior, I suggest to only update
BaseRngLibConstructor() API with TestRdRand, don't touch other APIs.

Thanks
Liming
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Pedro Falcato
> 发送时间: 2022年11月23日 6:31
> 收件人: devel@edk2.groups.io
> 抄送: Pedro Falcato <pedro.falcato@gmail.com>; Michael D Kinney
> <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
> Zhiguang Liu <zhiguang.liu@intel.com>; Jason A . Donenfeld
> <Jason@zx2c4.com>
> 主题: [edk2-devel] [PATCH v4 1/1] MdePkg/BaseRngLib: Add a smoketest for
> RDRAND and check CPUID
> 
> 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.
> 
> Testing algorithm inspired by linux's arch/x86/kernel/cpu/rdrand.c
> :x86_init_rdrand() per commit 049f9ae9..
> 
> 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
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4163
> 
> 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>
> ---
> v4: Added a doxygen comment to the TestRdRand() function
> v3: Addressed mailing list comments
> v2: Replaced the original v1 naive detection with a better, although still
not
> great, detection.
>  MdePkg/Library/BaseRngLib/Rand/RdRand.c | 99
> +++++++++++++++++++++++--
>  1 file changed, 91 insertions(+), 8 deletions(-)
> 
> diff --git a/MdePkg/Library/BaseRngLib/Rand/RdRand.c
> b/MdePkg/Library/BaseRngLib/Rand/RdRand.c
> index 070d41e2555f..ff99436dbbbd 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,88 @@ 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
> +
> +//
> +// Add a define for native-word RDRAND, just for the test.
> +//
> +#ifdef MDE_CPU_X64
> +#define AsmRdRand  AsmRdRand64
> +#else
> +#define AsmRdRand  AsmRdRand32
> +#endif
> +
> +/**
> +  Tests RDRAND for broken implementations.
> +
> +  @retval TRUE         RDRAND is reliable (and hopefully safe).
> +  @retval FALSE        RDRAND is unreliable and should be disabled,
> despite CPUID.
> +
> +**/
> +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.
> +  //
> +
> +  //
> +  // Our algorithm samples rdrand $RDRAND_TEST_SAMPLES times and
> expects
> +  // a different result $RDRAND_MIN_CHANGE times for reliable RDRAND
> usage.
> +  //
> +  UINTN   Prev;
> +  UINT8   Idx;
> +  UINT8   TestIteration;
> +  UINT32  Changed;
> +
> +  Changed = 0;
> +
> +  for (TestIteration = 0; TestIteration < RDRAND_TEST_SAMPLES;
> TestIteration++) {
> +    UINTN  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 (AsmRdRand (&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;
> +}
> +
> +#undef AsmRdRand
> +
>  /**
>    The constructor function checks whether or not RDRAND instruction is
> supported
>    by the host hardware.
> @@ -46,10 +129,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 +154,7 @@ ArchGetRandomNumber16 (
>    OUT     UINT16  *Rand
>    )
>  {
> +  ASSERT (mRdRandSupported);
>    return AsmRdRand16 (Rand);
>  }
> 
> @@ -86,6 +173,7 @@ ArchGetRandomNumber32 (
>    OUT     UINT32  *Rand
>    )
>  {
> +  ASSERT (mRdRandSupported);
>    return AsmRdRand32 (Rand);
>  }
> 
> @@ -104,6 +192,7 @@ ArchGetRandomNumber64 (
>    OUT     UINT64  *Rand
>    )
>  {
> +  ASSERT (mRdRandSupported);
>    return AsmRdRand64 (Rand);
>  }
> 
> @@ -120,11 +209,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: [edk2-devel] [PATCH v4 1/1] MdePkg/BaseRngLib: Add a smoketest for RDRAND and check CPUID

[Pedro Falcato]

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

> Pedro:
>   To keep the same behavior, I suggest to only update
> BaseRngLibConstructor() API with TestRdRand, don't touch other APIs.

Liming,
the point is to fix the library's broken behavior, as previously discussed
in the ML, and documented in the commit message and the bugzilla.
Only updating BaseRngLibConstructor would do absolutely nothing.
ArchIsRngSupported needs updating because it's bogus.
The individual ArchGetRandomNumberN() do not *need* updating, but it's a
good idea to make sure that "Existing software depends on this always
returning TRUE" isn't actually reality.

Pedro

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