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

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: 43 bytes --]

Hi,

Ping. Is there a blocker here?

Pedro

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

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

[Benjamin Doron]

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

Hi,
Is there merit to removing the assert statement? When this instance of the RngLib class is used, the platform builder says there's RNG support. Asserts are a little easier to see than debug prints, especially when they stall the platform. I think that leaving it in is better. If asserts don't call CpuDeadLoop() or are removed from the build entirely, then ` ArchIsRngSupported()` will ensure safe behaviour (but incorrect functionality).

Also, I'm slightly concerned that the check for minimum RDRAND changes will succeed if it returns 0s, then 1s, then 0s... Though this seems like an RDRAND broken too conveniently, not a true errata.

Regards,
Benjamin

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

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

[Pedro Falcato]

On Wed, Apr 26, 2023 at 10:54 PM Benjamin Doron
<benjamin.doron00@gmail.com> wrote:
>
> Hi,
> Is there merit to removing the assert statement? When this instance of the RngLib class is used, the platform builder says there's RNG support. Asserts are a little easier to see than debug prints, especially when they stall the platform. I think that leaving it in is better. If asserts don't call CpuDeadLoop() or are removed from the build entirely, then `ArchIsRngSupported()` will ensure safe behaviour (but incorrect functionality).

Certain platforms may not know if the CPU has rdrand or if it's broken
at all. For instance, OVMF (1st case) or firmware for the AMD AM4
mobos (where they had zen1, 2 and 3 with some RDRAND breakage in
between).
Also, as you mentioned, ASSERTs can be entirely removed from the
build. So I don't think asserts are a good idea here. Unless you want
to suggest hiding the ASSERT with a PCD, in which case, yuck?

> Also, I'm slightly concerned that the check for minimum RDRAND changes will succeed if it returns 0s, then 1s, then 0s... Though this seems like an RDRAND broken too conveniently, not a true errata.

Ack, I don't think the algorithm is perfect, but it's what Jason wrote
for Linux and suggested; I guess it may be adjusted if we so need, but
at the moment it definitely detects the egregious AMD breakage.

-- 
Pedro

[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 --]