From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-io1-f65.google.com (mail-io1-f65.google.com [209.85.166.65]) by mx.groups.io with SMTP id smtpd.web10.2416.1597345166543296826 for ; Thu, 13 Aug 2020 11:59:26 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20161025 header.b=SLSunJ9S; spf=pass (domain: gmail.com, ip: 209.85.166.65, mailfrom: matthewfcarlson@gmail.com) Received: by mail-io1-f65.google.com with SMTP id k23so8366696iom.10 for ; Thu, 13 Aug 2020 11:59:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Z9F1DvPlkqkUZphHe8zE73vXYwvWdhSaVxWKpnpoN8E=; b=SLSunJ9SjDp4Y2YI8uvhrR6S6orO9uQxpU4M/zDRGR3fiWLbDjtEgLva1ORzB2OG7g 5QGFTM1wW7sHBQVGNFJKw4/wxuUszNjnEZqvjVCOd96s2oVb/I7+nwxVA9cibMnl6Bd/ p6raUbSSFR6bh9HLlyKyYgfH1C0xo3o8A9J0A1XfeR5FOK9djuw7gVeTG31F/xg3oJhK V7Y+GexGyQek4rFHAMXqusC5/5qCa14Jdy7Mjn9+KvpvtATxgY6rfk/kgUx/2YOWrc5l B/+9ExOPxDEQhRmoDbCXIfdeRyKgLLMyEK69ymZzTNtfbr6mwVytYCJnQsxsF6wRiXAj sM4A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Z9F1DvPlkqkUZphHe8zE73vXYwvWdhSaVxWKpnpoN8E=; b=cKyPXBE9gHd+e6UNbl36Pj9HKsDmzeOAiazaDF0Wktg+FofM4Btgmzt5/OlPlR5y9c 7BPmV6C+322Sq3SC6gvBwME36pxaBzD7gjYyHnouERynsbPb3ekfGR0zpvlMlWoBYDhc SMCvE2DnIt/3viWaqRXbHnvbeKvt0qZ9/f1/mApUv/ONfUqcMMOZQsVwIlvNP1MSATpO d/2uRtzQW6EWnWVJMzNt/YMjAh/fV6gb3awvJiMbulR44etky2w4vYLN10Be4vlOFDg6 7eDG2iwvI+QCCzcIdsp+o/X59xCPGjrfapInANilW0cCLOZAW7lzGGTj2roPFF1t5ZtV Tpmg== X-Gm-Message-State: AOAM531mG4U5RxYA3INmfj4BdAVZcDbq0rdzbCbWrwNg55L8UKL5SOYq kXKck083V65Uj5zMKzMUqsBL1ztZgzistp5n2Ts= X-Google-Smtp-Source: ABdhPJwQ3spbIHsOE7bHCgnWx/rcUvKh44OedTQkMKlgHeNPOgx3RLeCE2nGLerxnX9QAMO1s2zdmOugghpxtBBUefs= X-Received: by 2002:a05:6638:27a:: with SMTP id x26mr6406930jaq.43.1597345165814; Thu, 13 Aug 2020 11:59:25 -0700 (PDT) MIME-Version: 1.0 References: <20200812224338.287-1-matthewfcarlson@gmail.com> <20200812224338.287-2-matthewfcarlson@gmail.com> In-Reply-To: From: "Matthew Carlson" Date: Thu, 13 Aug 2020 11:59:16 -0700 Message-ID: Subject: Re: [PATCH v6 1/5] MdePkg: TimerRngLib: Added RngLib that uses TimerLib To: Ard Biesheuvel Cc: devel@edk2.groups.io, Michael D Kinney , Liming Gao , Zhiguang Liu Content-Type: multipart/alternative; boundary="0000000000003133ac05acc6e7fd" --0000000000003133ac05acc6e7fd Content-Type: text/plain; charset="UTF-8" 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 wrote: > On 8/13/20 12:43 AM, matthewfcarlson@gmail.com wrote: > > From: Matthew Carlson > > > > 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 > > Cc: Michael D Kinney > > Cc: Liming Gao > > Cc: Zhiguang Liu > > Signed-off-by: Matthew Carlson > > --- > > 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 > > +#include > > +#include > > +#include > > + > > +/** > > + * 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.
> > +# > > +# 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 > > > > --0000000000003133ac05acc6e7fd Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Good things to point out. Should be fixed.
=
The original=C2=A0code in CryptoPkg simply just = delayed 10 microseconds and hoped the performance counter had incremented i= n that time frame.

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

=C2=A0Copyright (= c) Microsoft Corporation=C2=A0is the preferred way for the Microsoft= copyright

-Matthew Carl= son


On Thu, Aug 13, 2020= at 5:09 AM Ard Biesheuvel <ar= d.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_b= ug.cgi?id=3D1871
>
> 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>
> ---
>=C2=A0 =C2=A0MdePkg/Library/BaseRngLibTimerLib/RngLibTimer.c=C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 | 187 ++++++++++++++++++++
>=C2=A0 =C2=A0MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf |= =C2=A0 40 +++++
>=C2=A0 =C2=A0MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.uni |= =C2=A0 17 ++
>=C2=A0 =C2=A0MdePkg/MdePkg.dsc=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 |=C2=A0 =C2=A03 +-
>=C2=A0 =C2=A04 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
> +=C2=A0 BaseRng Library that uses the TimerLib to provide reasonably r= andom numbers.
> +=C2=A0 Do not use this on a production system.
> +
> +=C2=A0 Copyright (c) Microsoft Corporation.
> +=C2=A0 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 (

> +=C2=A0 VOID
> +=C2=A0 )
> +{
> +=C2=A0 UINT64 StartValue;
> +=C2=A0 UINT64 EndValue;
> +=C2=A0 UINT64 CounterHz;
> +=C2=A0 UINT64 MinumumDelayInMicroSeconds;

newline here

> +=C2=A0 // Get the counter properties
> +=C2=A0 CounterHz =3D GetPerformanceCounterProperties(&StartValue,= &EndValue);

space before (

> +=C2=A0 // Make sure we won't divide by zero
> +=C2=A0 if (CounterHz =3D=3D 0) {
> +=C2=A0 =C2=A0 ASSERT(FALSE); // Assert so the developer knows somethi= ng is wrong

This will print

ASSERT (FALSE)

into the DEBUG log, whereas

ASSERT (CounterHz !=3D 0)

will appear if you assert on the actual value, which is much more useful.
> +=C2=A0 =C2=A0 return;
> +=C2=A0 }
> +=C2=A0 // Calculate the minimum delay based on 1.5 microseconds divid= ed by the hertz.
> +=C2=A0 // We calculate the length of a cycle (1/CounterHz) and multip= ly it by 1.5 microseconds
> +=C2=A0 // This ensures that the performance counter has increased by = at least one
> +=C2=A0 MinumumDelayInMicroSeconds =3D 1500000 / CounterHz;
> +
> +=C2=A0 MicroSecondDelay(MinumumDelayInMicroSeconds);

Space before (

> +}
> +
> +
> +/**
> +=C2=A0 Generates a 16-bit random number.
> +
> +=C2=A0 if Rand is NULL, then ASSERT().
> +
> +=C2=A0 @param[out] Rand=C2=A0 =C2=A0 =C2=A0Buffer pointer to store th= e 16-bit random value.
> +
> +=C2=A0 @retval TRUE=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Random number ge= nerated successfully.
> +=C2=A0 @retval FALSE=C2=A0 =C2=A0 =C2=A0 =C2=A0 Failed to generate th= e random number.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +GetRandomNumber16 (
> +=C2=A0 OUT=C2=A0 =C2=A0 =C2=A0UINT16=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 *Rand
> +=C2=A0 )
> +{
> +=C2=A0 UINT32=C2=A0 Index;
> +=C2=A0 UINT8* RandPtr;

Please align the variable names vertically, and put the * on the right
hand side.

> +
> +=C2=A0 ASSERT (Rand !=3D NULL);
> +
> +=C2=A0 if (NULL =3D=3D Rand) {

No yoda style comparisons please

> +=C2=A0 =C2=A0 return FALSE;
> +=C2=A0 }
> +
> +=C2=A0 RandPtr =3D (UINT8 *) Rand;

Please drop the space after )

> +=C2=A0 // Get 2 bytes of random ish data
> +=C2=A0 // This should take around 10us
> +=C2=A0 for (Index =3D 0; Index < 2; Index ++) {
> +=C2=A0 =C2=A0 *RandPtr =3D (UINT8) (GetPerformanceCounter () & 0x= FF);

Same here

> +=C2=A0 =C2=A0 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?

> +=C2=A0 =C2=A0 RandPtr++;
> +=C2=A0 }
> +=C2=A0 return TRUE;
> +}
> +
> +/**
> +=C2=A0 Generates a 32-bit random number.
> +
> +=C2=A0 if Rand is NULL, then ASSERT().
> +
> +=C2=A0 @param[out] Rand=C2=A0 =C2=A0 =C2=A0Buffer pointer to store th= e 32-bit random value.
> +
> +=C2=A0 @retval TRUE=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Random number ge= nerated successfully.
> +=C2=A0 @retval FALSE=C2=A0 =C2=A0 =C2=A0 =C2=A0 Failed to generate th= e random number.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +GetRandomNumber32 (
> +=C2=A0 OUT=C2=A0 =C2=A0 =C2=A0UINT32=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 *Rand
> +=C2=A0 )
> +{
> +=C2=A0 UINT32=C2=A0 Index;
> +=C2=A0 UINT8* RandPtr;
> +
> +=C2=A0 ASSERT (Rand !=3D NULL);
> +
> +=C2=A0 if (NULL =3D=3D Rand) {
> +=C2=A0 =C2=A0 return FALSE;
> +=C2=A0 }
> +
> +=C2=A0 RandPtr =3D (UINT8 *) Rand;
> +=C2=A0 // Get 4 bytes of random ish data
> +=C2=A0 // This should take around 20ms
> +=C2=A0 for (Index =3D 0; Index < 4; Index ++) {
> +=C2=A0 =C2=A0 *RandPtr =3D (UINT8) (GetPerformanceCounter () & 0x= FF);
> +=C2=A0 =C2=A0 DecentDelay (); // delay to give chance for performance= counter to catch up
> +=C2=A0 =C2=A0 RandPtr++;
> +=C2=A0 }
> +=C2=A0 return TRUE;
> +}
> +
> +/**
> +=C2=A0 Generates a 64-bit random number.
> +
> +=C2=A0 if Rand is NULL, then ASSERT().
> +
> +=C2=A0 @param[out] Rand=C2=A0 =C2=A0 =C2=A0Buffer pointer to store th= e 64-bit random value.
> +
> +=C2=A0 @retval TRUE=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Random number ge= nerated successfully.
> +=C2=A0 @retval FALSE=C2=A0 =C2=A0 =C2=A0 =C2=A0 Failed to generate th= e random number.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +GetRandomNumber64 (
> +=C2=A0 OUT=C2=A0 =C2=A0 =C2=A0UINT64=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 *Rand
> +=C2=A0 )
> +{
> +=C2=A0 UINT32=C2=A0 Index;
> +=C2=A0 UINT8* RandPtr;
> +
> +=C2=A0 ASSERT (Rand !=3D NULL);
> +
> +=C2=A0 if (NULL =3D=3D Rand) {
> +=C2=A0 =C2=A0 return FALSE;
> +=C2=A0 }
> +
> +=C2=A0 RandPtr =3D (UINT8 *) Rand;
> +=C2=A0 // Get 8 bytes of random ish data
> +=C2=A0 // This should take around 40ms
> +=C2=A0 for (Index =3D 0; Index < 8; Index ++) {
> +=C2=A0 =C2=A0 *RandPtr =3D (UINT8) (GetPerformanceCounter () & 0x= FF);
> +=C2=A0 =C2=A0 DecentDelay (); // delay to give chance for performance= counter to catch up
> +=C2=A0 =C2=A0 RandPtr++;
> +=C2=A0 }
> +
> +=C2=A0 return TRUE;
> +}
> +
> +/**
> +=C2=A0 Generates a 128-bit random number.
> +
> +=C2=A0 if Rand is NULL, then ASSERT().
> +
> +=C2=A0 @param[out] Rand=C2=A0 =C2=A0 =C2=A0Buffer pointer to store th= e 128-bit random value.
> +
> +=C2=A0 @retval TRUE=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Random number ge= nerated successfully.
> +=C2=A0 @retval FALSE=C2=A0 =C2=A0 =C2=A0 =C2=A0 Failed to generate th= e random number.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +GetRandomNumber128 (
> +=C2=A0 OUT=C2=A0 =C2=A0 =C2=A0UINT64=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 *Rand
> +=C2=A0 )
> +{
> +=C2=A0 ASSERT (Rand !=3D NULL);
> +=C2=A0 // This should take around 80ms
> +
> +=C2=A0 // Read first 64 bits
> +=C2=A0 if (!GetRandomNumber64 (Rand)) {
> +=C2=A0 =C2=A0 return FALSE;
> +=C2=A0 }
> +
> +=C2=A0 // Read second 64 bits
> +=C2=A0 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
> +#=C2=A0 Instance of RNG (Random Number Generator) Library.
> +#
> +#=C2=A0 BaseRng Library that uses the TimerLib to provide reasonably = random numbers.
> +#=C2=A0 Do NOT use this on a production system as this uses the syste= m performance
> +#=C2=A0 counter rather than a true source of random in addition to ha= ving a weak
> +#=C2=A0 random algorithm. This is provided primarily as a source of e= ntropy for
> +#=C2=A0 OpenSSL for platforms that do not have a good built in RngLib= as this
> +#=C2=A0 emulates what was done before (though it isn't perfect).<= br> > +#
> +#=C2=A0 Copyright (c) Microsoft Corporation. All rights reserved.<= BR>
> +#
> +#=C2=A0 SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#
> +##
> +
> +[Defines]
> +=C2=A0 INF_VERSION=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =3D 0x00010005

Please use the latest version (1.27 iirc, which you don't actually need=
to convert to hex format like above)

> +=C2=A0 BASE_NAME=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =3D BaseRngLibTimerLib
> +=C2=A0 MODULE_UNI_FILE=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =3D BaseRngLibTimerLib.uni
> +=C2=A0 FILE_GUID=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =3D 74950C45-10FC-4AB5-B114-49C87C17409B
> +=C2=A0 MODULE_TYPE=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =3D BASE
> +=C2=A0 VERSION_STRING=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0=3D 1.0
> +=C2=A0 LIBRARY_CLASS=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =3D RngLib
> +=C2=A0 CONSTRUCTOR=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =3D BaseRngLibConstructor
> +
> +#
> +#=C2=A0 VALID_ARCHITECTURES=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =3D IA32 X64

Drop this unless it is accurate.

> +#
> +
> +[Sources]
> +=C2=A0 RngLibTimer.c
> +
> +[Packages]
> +=C2=A0 MdePkg/MdePkg.dec
> +
> +[LibraryClasses]
> +=C2=A0 BaseLib
> +=C2=A0 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=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0#language en-US "Instance of RNG Library"
> +
> +#string STR_MODULE_DESCRIPTION=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 #lan= guage en-US "BaseRng Library that uses the TimerLib to provide low-ent= ropy 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 @@
>=C2=A0 =C2=A0 =C2=A0MdePkg/Library/BasePostCodeLibPort80/BasePostCodeLi= bPort80.inf
>=C2=A0 =C2=A0 =C2=A0MdePkg/Library/BasePrintLib/BasePrintLib.inf
>=C2=A0 =C2=A0 =C2=A0MdePkg/Library/BaseReportStatusCodeLibNull/BaseRepo= rtStatusCodeLibNull.inf
> +=C2=A0 MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
> +=C2=A0 MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf

Please match the indentation of the surrounding lines.

>=C2=A0 =C2=A0 =C2=A0MdePkg/Library/BaseSerialPortLibNull/BaseSerialPort= LibNull.inf
>=C2=A0 =C2=A0 =C2=A0MdePkg/Library/BaseSynchronizationLib/BaseSynchroni= zationLib.inf
>=C2=A0 =C2=A0 =C2=A0MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLi= bNullTemplate.inf
> @@ -69,7 +71,6 @@
>=C2=A0 =C2=A0 =C2=A0MdePkg/Library/BaseUefiDecompressLib/BaseUefiTianoC= ustomDecompressLib.inf
>=C2=A0 =C2=A0 =C2=A0MdePkg/Library/BaseSmbusLibNull/BaseSmbusLibNull.in= f
>=C2=A0 =C2=A0 =C2=A0MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf > -=C2=A0 MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf
>=C2=A0 =C2=A0
>=C2=A0 =C2=A0 =C2=A0MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.= inf
>=C2=A0 =C2=A0 =C2=A0MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
>

--0000000000003133ac05acc6e7fd--