Re: [PATCH v6 1/5] MdePkg: TimerRngLib: Added RngLib that uses TimerLib

["Matthew Carlson" <matthewfcarlson@gmail.com>]

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

Good things to point out. Should be fixed.

The original code in CryptoPkg simply just delayed 10 microseconds and
hoped the performance counter had incremented in that time frame.
https://github.com/tianocore/edk2/blob/313d2ec991039abe24727eced80d8ece1befbc93/CryptoPkg/Library/OpensslLib/rand_pool.c#L45

This new code ensures that we are delaying for at least 1.5 performance
counter ticks, so we're guaranteed to get a different performance counter
value with some hope for randomness. As Mike K pointed out if a system has
a slow performance counter you just get the same number repeated a few
times.

 Copyright (c) Microsoft Corporation is the preferred way for the Microsoft
copyright

-Matthew Carlson


On Thu, Aug 13, 2020 at 5:09 AM Ard Biesheuvel <ard.biesheuvel@arm.com>
wrote:

> On 8/13/20 12:43 AM, matthewfcarlson@gmail.com wrote:
> > From: Matthew Carlson <macarl@microsoft.com>
> >
> > Added a new RngLib that provides random numbers from the TimerLib
> > using the performance counter. This is meant to be used for OpenSSL
> > to replicate past behavior. This should not be used in production as
> > a real source of entropy.
> >
> > Ref: https://github.com/tianocore/edk2/pull/845
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1871
> >
> > Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> > Signed-off-by: Matthew Carlson <matthewfcarlson@gmail.com>
> > ---
> >   MdePkg/Library/BaseRngLibTimerLib/RngLibTimer.c          | 187
> ++++++++++++++++++++
> >   MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf |  40 +++++
> >   MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.uni |  17 ++
> >   MdePkg/MdePkg.dsc                                        |   3 +-
> >   4 files changed, 246 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdePkg/Library/BaseRngLibTimerLib/RngLibTimer.c
> b/MdePkg/Library/BaseRngLibTimerLib/RngLibTimer.c
> > new file mode 100644
> > index 000000000000..915382fb9278
> > --- /dev/null
> > +++ b/MdePkg/Library/BaseRngLibTimerLib/RngLibTimer.c
> > @@ -0,0 +1,187 @@
> > +/** @file
> > +  BaseRng Library that uses the TimerLib to provide reasonably random
> numbers.
> > +  Do not use this on a production system.
> > +
> > +  Copyright (c) Microsoft Corporation.
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +**/
> > +
> > +#include <Base.h>
> > +#include <Library/BaseLib.h>
> > +#include <Library/DebugLib.h>
> > +#include <Library/TimerLib.h>
> > +
> > +/**
> > + * Using the TimerLib GetPerformanceCounterProperties() we delay
> > + * for enough time for the PerformanceCounter to increment.
> > + * Depending on your system
> > + *
> > + * If the return value from GetPerformanceCounterProperties (TimerLib)
> > + * is zero, this function will not delay and attempt to assert.
> > + */
>
> Make this STATIC please
>
> > +VOID
> > +EFIAPI
> > +DecentDelay(
>
> space before (
>
> > +  VOID
> > +  )
> > +{
> > +  UINT64 StartValue;
> > +  UINT64 EndValue;
> > +  UINT64 CounterHz;
> > +  UINT64 MinumumDelayInMicroSeconds;
>
> newline here
>
> > +  // Get the counter properties
> > +  CounterHz = GetPerformanceCounterProperties(&StartValue, &EndValue);
>
> space before (
>
> > +  // Make sure we won't divide by zero
> > +  if (CounterHz == 0) {
> > +    ASSERT(FALSE); // Assert so the developer knows something is wrong
>
> This will print
>
> ASSERT (FALSE)
>
> into the DEBUG log, whereas
>
> ASSERT (CounterHz != 0)
>
> will appear if you assert on the actual value, which is much more useful.
>
> > +    return;
> > +  }
> > +  // Calculate the minimum delay based on 1.5 microseconds divided by
> the hertz.
> > +  // We calculate the length of a cycle (1/CounterHz) and multiply it
> by 1.5 microseconds
> > +  // This ensures that the performance counter has increased by at
> least one
> > +  MinumumDelayInMicroSeconds = 1500000 / CounterHz;
> > +
> > +  MicroSecondDelay(MinumumDelayInMicroSeconds);
>
> Space before (
>
> > +}
> > +
> > +
> > +/**
> > +  Generates a 16-bit random number.
> > +
> > +  if Rand is NULL, then ASSERT().
> > +
> > +  @param[out] Rand     Buffer pointer to store the 16-bit random value.
> > +
> > +  @retval TRUE         Random number generated successfully.
> > +  @retval FALSE        Failed to generate the random number.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +GetRandomNumber16 (
> > +  OUT     UINT16                    *Rand
> > +  )
> > +{
> > +  UINT32  Index;
> > +  UINT8* RandPtr;
>
> Please align the variable names vertically, and put the * on the right
> hand side.
>
> > +
> > +  ASSERT (Rand != NULL);
> > +
> > +  if (NULL == Rand) {
>
> No yoda style comparisons please
>
> > +    return FALSE;
> > +  }
> > +
> > +  RandPtr = (UINT8 *) Rand;
>
> Please drop the space after )
>
> > +  // Get 2 bytes of random ish data
> > +  // This should take around 10us
> > +  for (Index = 0; Index < 2; Index ++) {
> > +    *RandPtr = (UINT8) (GetPerformanceCounter () & 0xFF);
>
> Same here
>
> > +    DecentDelay (); // delay to give chance for performance counter to
> catch up
>
> So the delay is intended to ensure that the perf counter assumes its
> prior value + 1? How does that help? I'd argue that this actually makes
> the value more predictable than simply taking 16 bits worth of
> performance counter bits.
>
> Or was this code simply adopted from CryptoPkg in some way?
>
> > +    RandPtr++;
> > +  }
> > +  return TRUE;
> > +}
> > +
> > +/**
> > +  Generates a 32-bit random number.
> > +
> > +  if Rand is NULL, then ASSERT().
> > +
> > +  @param[out] Rand     Buffer pointer to store the 32-bit random value.
> > +
> > +  @retval TRUE         Random number generated successfully.
> > +  @retval FALSE        Failed to generate the random number.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +GetRandomNumber32 (
> > +  OUT     UINT32                    *Rand
> > +  )
> > +{
> > +  UINT32  Index;
> > +  UINT8* RandPtr;
> > +
> > +  ASSERT (Rand != NULL);
> > +
> > +  if (NULL == Rand) {
> > +    return FALSE;
> > +  }
> > +
> > +  RandPtr = (UINT8 *) Rand;
> > +  // Get 4 bytes of random ish data
> > +  // This should take around 20ms
> > +  for (Index = 0; Index < 4; Index ++) {
> > +    *RandPtr = (UINT8) (GetPerformanceCounter () & 0xFF);
> > +    DecentDelay (); // delay to give chance for performance counter to
> catch up
> > +    RandPtr++;
> > +  }
> > +  return TRUE;
> > +}
> > +
> > +/**
> > +  Generates a 64-bit random number.
> > +
> > +  if Rand is NULL, then ASSERT().
> > +
> > +  @param[out] Rand     Buffer pointer to store the 64-bit random value.
> > +
> > +  @retval TRUE         Random number generated successfully.
> > +  @retval FALSE        Failed to generate the random number.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +GetRandomNumber64 (
> > +  OUT     UINT64                    *Rand
> > +  )
> > +{
> > +  UINT32  Index;
> > +  UINT8* RandPtr;
> > +
> > +  ASSERT (Rand != NULL);
> > +
> > +  if (NULL == Rand) {
> > +    return FALSE;
> > +  }
> > +
> > +  RandPtr = (UINT8 *) Rand;
> > +  // Get 8 bytes of random ish data
> > +  // This should take around 40ms
> > +  for (Index = 0; Index < 8; Index ++) {
> > +    *RandPtr = (UINT8) (GetPerformanceCounter () & 0xFF);
> > +    DecentDelay (); // delay to give chance for performance counter to
> catch up
> > +    RandPtr++;
> > +  }
> > +
> > +  return TRUE;
> > +}
> > +
> > +/**
> > +  Generates a 128-bit random number.
> > +
> > +  if Rand is NULL, then ASSERT().
> > +
> > +  @param[out] Rand     Buffer pointer to store the 128-bit random value.
> > +
> > +  @retval TRUE         Random number generated successfully.
> > +  @retval FALSE        Failed to generate the random number.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +GetRandomNumber128 (
> > +  OUT     UINT64                    *Rand
> > +  )
> > +{
> > +  ASSERT (Rand != NULL);
> > +  // This should take around 80ms
> > +
> > +  // Read first 64 bits
> > +  if (!GetRandomNumber64 (Rand)) {
> > +    return FALSE;
> > +  }
> > +
> > +  // Read second 64 bits
> > +  return GetRandomNumber64 (++Rand);
> > +}
> > diff --git a/MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
> b/MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
> > new file mode 100644
> > index 000000000000..34dea0152497
> > --- /dev/null
> > +++ b/MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
> > @@ -0,0 +1,40 @@
> > +## @file
> > +#  Instance of RNG (Random Number Generator) Library.
> > +#
> > +#  BaseRng Library that uses the TimerLib to provide reasonably random
> numbers.
> > +#  Do NOT use this on a production system as this uses the system
> performance
> > +#  counter rather than a true source of random in addition to having a
> weak
> > +#  random algorithm. This is provided primarily as a source of entropy
> for
> > +#  OpenSSL for platforms that do not have a good built in RngLib as this
> > +#  emulates what was done before (though it isn't perfect).
> > +#
> > +#  Copyright (c) Microsoft Corporation. All rights reserved.<BR>
> > +#
> > +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +#
> > +#
> > +##
> > +
> > +[Defines]
> > +  INF_VERSION                    = 0x00010005
>
> Please use the latest version (1.27 iirc, which you don't actually need
> to convert to hex format like above)
>
> > +  BASE_NAME                      = BaseRngLibTimerLib
> > +  MODULE_UNI_FILE                = BaseRngLibTimerLib.uni
> > +  FILE_GUID                      = 74950C45-10FC-4AB5-B114-49C87C17409B
> > +  MODULE_TYPE                    = BASE
> > +  VERSION_STRING                 = 1.0
> > +  LIBRARY_CLASS                  = RngLib
> > +  CONSTRUCTOR                    = BaseRngLibConstructor
> > +
> > +#
> > +#  VALID_ARCHITECTURES           = IA32 X64
>
> Drop this unless it is accurate.
>
> > +#
> > +
> > +[Sources]
> > +  RngLibTimer.c
> > +
> > +[Packages]
> > +  MdePkg/MdePkg.dec
> > +
> > +[LibraryClasses]
> > +  BaseLib
> > +  TimerLib
> > diff --git a/MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.uni
> b/MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.uni
> > new file mode 100644
> > index 000000000000..766a8e0ddf97
> > --- /dev/null
> > +++ b/MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.uni
> > @@ -0,0 +1,17 @@
> > +// /** @file
> > +// Instance of RNG (Random Number Generator) Library.
> > +//
> > +// BaseRng Library that uses TimerLib's performance counter
>
> Please refer to this library as RngLib, to avoid confusion. Adding
> 'base' to qualify it is fine.
>
> > +// to provide random numbers.
> > +//
> > +// Copyright (c) Microsoft Corporation
>
> year?
>
> > +//
> > +// SPDX-License-Identifier: BSD-2-Clause-Patent
> > +//
> > +// **/
> > +
> > +
> > +#string STR_MODULE_ABSTRACT             #language en-US "Instance of
> RNG Library"
> > +
> > +#string STR_MODULE_DESCRIPTION          #language en-US "BaseRng
> Library that uses the TimerLib to provide low-entropy random numbers"
> > +
> > diff --git a/MdePkg/MdePkg.dsc b/MdePkg/MdePkg.dsc
> > index 472fa3777412..d7ba3a730909 100644
> > --- a/MdePkg/MdePkg.dsc
> > +++ b/MdePkg/MdePkg.dsc
> > @@ -62,6 +62,8 @@
> >     MdePkg/Library/BasePostCodeLibPort80/BasePostCodeLibPort80.inf
> >     MdePkg/Library/BasePrintLib/BasePrintLib.inf
> >
>  MdePkg/Library/BaseReportStatusCodeLibNull/BaseReportStatusCodeLibNull.inf
> > +  MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
> > +  MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf
>
> Please match the indentation of the surrounding lines.
>
> >     MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf
> >     MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
> >     MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTemplate.inf
> > @@ -69,7 +71,6 @@
> >
>  MdePkg/Library/BaseUefiDecompressLib/BaseUefiTianoCustomDecompressLib.inf
> >     MdePkg/Library/BaseSmbusLibNull/BaseSmbusLibNull.inf
> >     MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
> > -  MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf
> >
> >     MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
> >     MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
> >
>
>

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