[edk2-devel] [PATCH v8 1/5] MdePkg: TimerRngLib: Added RngLib that uses TimerLib
Michael D Kinney
michael.d.kinney at intel.com
Thu Aug 20 15:21:42 UTC 2020
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 at gmail.com <matthewfcarlson at gmail.com>
> Sent: Wednesday, August 19, 2020 12:37 PM
> To: devel at edk2.groups.io
> Cc: Ard Biesheuvel <ard.biesheuvel at arm.com>; Kinney, Michael D <michael.d.kinney at intel.com>; Gao, Liming <liming.gao at intel.com>;
> Liu, Zhiguang <zhiguang.liu at intel.com>; Matthew Carlson <matthewfcarlson at gmail.com>
> Subject: [PATCH v8 1/5] MdePkg: TimerRngLib: Added RngLib that uses TimerLib
>
> From: Matthew Carlson <macarl at 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 at arm.com>
> Cc: Michael D Kinney <michael.d.kinney at intel.com>
> Cc: Liming Gao <liming.gao at intel.com>
> Cc: Zhiguang Liu <zhiguang.liu at intel.com>
> Signed-off-by: Matthew Carlson <matthewfcarlson at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list