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

["gaoliming" <gaoliming@byosoft.com.cn>]

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