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.biesheu...@arm.com> wrote: > On 8/13/20 12:43 AM, matthewfcarl...@gmail.com wrote: > > 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 | 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 > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#64245): https://edk2.groups.io/g/devel/message/64245 Mute This Topic: https://groups.io/mt/76157313/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
