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

["Michael D Kinney" <michael.d.kinney@intel.com>]

Hi Matt,

Some comments inline below.

I also see come comments from Ard on this series about code style.
I did not provide feedback on the code style issues here (except for
a function header comment block style).

There is a tool called ECC (EFI Code Checker) that is now enabled in
EDK II CI.  Please run this checker locally and resolve all issues in
your patch series.

Thanks,

Mike


> -----Original Message-----
> From: matthewfcarlson@gmail.com <matthewfcarlson@gmail.com>
> Sent: Wednesday, August 19, 2020 12:37 PM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>;
> Liu, Zhiguang <zhiguang.liu@intel.com>; Matthew Carlson <matthewfcarlson@gmail.com>
> Subject: [PATCH v8 1/5] MdePkg: TimerRngLib: Added RngLib that uses TimerLib
> 
> 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          | 191 ++++++++++++++++++++
>  MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf |  36 ++++
>  MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.uni |  15 ++
>  MdePkg/MdePkg.dsc                                        |   3 +-
>  4 files changed, 244 insertions(+), 1 deletion(-)
> 
> diff --git a/MdePkg/Library/BaseRngLibTimerLib/RngLibTimer.c b/MdePkg/Library/BaseRngLibTimerLib/RngLibTimer.c
> new file mode 100644
> index 000000000000..c72aa335823d
> --- /dev/null
> +++ b/MdePkg/Library/BaseRngLibTimerLib/RngLibTimer.c
> @@ -0,0 +1,191 @@
> +/** @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


Please update the comments to describe that this function returns 
delay in microseconds.  It does not actually do the delay.

> 
> + *
> 
> + * If the return value from GetPerformanceCounterProperties (TimerLib)
> 
> + * is zero, this function will not delay and attempt to assert.
> 
> + */

Comment block style does not match EDK II style.  Remove extra '*'.


> 
> +STATIC
> 
> +UINT32
> 
> +CalculateMinimumDecentDelayInMicroseconds (
> 
> +  VOID
> 
> +  )
> 
> +{
> 
> +  UINT64 StartValue;

Remove

> 
> +  UINT64 EndValue;

Remove

> 
> +  UINT64 CounterHz;
> 
> +  UINT64 MinumumDelayInMicroSeconds;
> 
> +
> 
> +  // Get the counter properties
> 
> +  CounterHz = GetPerformanceCounterProperties (&StartValue, &EndValue);


Change to:

    CounterHz = GetPerformanceCounterProperties (NULL, NULL);

> 
> +  // Make sure we won't divide by zero
> 
> +  if (CounterHz == 0) {
> 
> +    ASSERT(CounterHz != 0); // Assert so the developer knows something is wrong
> 
> +    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
> 
> +  return (UINT32)(MAX(DivU64x64Remainder(1500000 / CounterHz, NULL), 1));
> 
> +}
> 
> +
> 
> +
> 
> +/**
> 
> +  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;
> 
> +  UINT32  DelayInMicroSeconds;
> 
> +
> 
> +  ASSERT (Rand != NULL);
> 
> +
> 
> +  if (Rand == NULL) {
> 
> +    return FALSE;
> 
> +  }
> 
> +  DelayInMicroSeconds = CalculateMinimumDecentDelayInMicroseconds ();
> 
> +  RandPtr = (UINT8*)Rand;
> 
> +  // Get 2 bytes of random ish data
> 
> +  for (Index = 0; Index < 2; Index ++) {
> 
> +    *RandPtr = (UINT8)(GetPerformanceCounter () & 0xFF);
> 
> +    // Delay to give the performance counter a chance to change
> 
> +    MicroSecondDelay (DelayInMicroSeconds);
> 
> +    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;
> 
> +  UINT32  DelayInMicroSeconds;
> 
> +
> 
> +  ASSERT (Rand != NULL);
> 
> +
> 
> +  if (NULL == Rand) {
> 
> +    return FALSE;
> 
> +  }
> 
> +
> 
> +  RandPtr = (UINT8 *) Rand;
> 
> +  DelayInMicroSeconds = CalculateMinimumDecentDelayInMicroseconds ();
> 
> +  // Get 4 bytes of random ish data
> 
> +  for (Index = 0; Index < 4; Index ++) {
> 
> +    *RandPtr = (UINT8) (GetPerformanceCounter () & 0xFF);
> 
> +    // Delay to give the performance counter a chance to change
> 
> +    MicroSecondDelay (DelayInMicroSeconds);
> 
> +    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;
> 
> +  UINT32  DelayInMicroSeconds;
> 
> +
> 
> +  ASSERT (Rand != NULL);
> 
> +
> 
> +  if (NULL == Rand) {
> 
> +    return FALSE;
> 
> +  }
> 
> +
> 
> +  RandPtr = (UINT8 *) Rand;
> 
> +  DelayInMicroSeconds = CalculateMinimumDecentDelayInMicroseconds ();
> 
> +  // Get 8 bytes of random ish data
> 
> +  for (Index = 0; Index < 8; Index ++) {
> 
> +    *RandPtr = (UINT8)(GetPerformanceCounter () & 0xFF);
> 
> +    // Delay to give the performance counter a chance to change
> 
> +    MicroSecondDelay (DelayInMicroSeconds);
> 
> +    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


Update comment to remove mention of specific delay and match
similar comments in functions above.

> 
> +
> 
> +  // 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..c499e5327351
> --- /dev/null
> +++ b/MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
> @@ -0,0 +1,36 @@
> +## @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                    = 1.27
> 
> +  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
> 
> +
> 
> +[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..fde24b9f0107
> --- /dev/null
> +++ b/MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.uni
> @@ -0,0 +1,15 @@
> +// @file
> 
> +// Instance of RNG (Random Number Generator) Library.
> 
> +//
> 
> +// RngLib that uses TimerLib's performance counter to provide random numbers.
> 
> +//
> 
> +// Copyright (c) Microsoft Corporation.
> 
> +//
> 
> +// 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
> 
>    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
> 
> --
> 2.28.0.windows.1