From: "Ard Biesheuvel" <ard.biesheuvel@arm.com>
To: matthewfcarlson@gmail.com, devel@edk2.groups.io
Cc: Michael D Kinney <michael.d.kinney@intel.com>,
Liming Gao <liming.gao@intel.com>,
Zhiguang Liu <zhiguang.liu@intel.com>
Subject: Re: [PATCH v6 1/5] MdePkg: TimerRngLib: Added RngLib that uses TimerLib
Date: Thu, 13 Aug 2020 14:09:28 +0200 [thread overview]
Message-ID: <d2b05ea0-fe26-a6e5-60db-ef52ca1bc69d@arm.com> (raw)
In-Reply-To: <20200812224338.287-2-matthewfcarlson@gmail.com>
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
>
next prev parent reply other threads:[~2020-08-13 12:09 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-12 22:43 [PATCH v6 0/5] Use RngLib instead of TimerLib for OpensslLib Matthew Carlson
2020-08-12 22:43 ` [PATCH v6 1/5] MdePkg: TimerRngLib: Added RngLib that uses TimerLib Matthew Carlson
2020-08-13 12:09 ` Ard Biesheuvel [this message]
2020-08-13 18:59 ` Matthew Carlson
2020-08-12 22:43 ` [PATCH v6 2/5] MdePkg: BaseRngLibDxe: Add RngLib that uses RngDxe Matthew Carlson
2020-08-13 9:10 ` Liming Gao
2020-08-13 12:19 ` Ard Biesheuvel
2020-08-13 19:18 ` Matthew Carlson
2020-08-12 22:43 ` [PATCH v6 3/5] OvmfPkg: Add RngLib based on TimerLib for Crypto Matthew Carlson
2020-08-13 8:34 ` Laszlo Ersek
2020-08-12 22:43 ` [PATCH v6 4/5] ArmVirtPkg: Add RngLib based on TimerLib for CryptoPkg Matthew Carlson
2020-08-13 8:31 ` Laszlo Ersek
2020-08-12 22:43 ` [PATCH v6 5/5] CryptoPkg: OpensslLib: Use RngLib to generate entropy in rand_pool Matthew Carlson
2020-08-13 12:21 ` Ard Biesheuvel
2020-08-13 15:14 ` [PATCH v6 0/5] Use RngLib instead of TimerLib for OpensslLib Yao, Jiewen
2020-08-13 19:42 ` Matthew Carlson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d2b05ea0-fe26-a6e5-60db-ef52ca1bc69d@arm.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox