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: matthewfcarl...@gmail.com <matthewfcarl...@gmail.com> > Sent: Wednesday, August 19, 2020 12:37 PM > To: devel@edk2.groups.io > Cc: Ard Biesheuvel <ard.biesheu...@arm.com>; Kinney, Michael D > <michael.d.kin...@intel.com>; Gao, Liming <liming....@intel.com>; > Liu, Zhiguang <zhiguang....@intel.com>; Matthew Carlson > <matthewfcarl...@gmail.com> > Subject: [PATCH v8 1/5] MdePkg: TimerRngLib: Added RngLib that uses TimerLib > > From: Matthew Carlson <mac...@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.biesheu...@arm.com> > Cc: Michael D Kinney <michael.d.kin...@intel.com> > Cc: Liming Gao <liming....@intel.com> > Cc: Zhiguang Liu <zhiguang....@intel.com> > Signed-off-by: Matthew Carlson <matthewfcarl...@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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#64510): https://edk2.groups.io/g/devel/message/64510 Mute This Topic: https://groups.io/mt/76294211/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
