* [PATCH v6 1/5] MdePkg: TimerRngLib: Added RngLib that uses TimerLib
2020-08-12 22:43 [PATCH v6 0/5] Use RngLib instead of TimerLib for OpensslLib Matthew Carlson
@ 2020-08-12 22:43 ` Matthew Carlson
2020-08-13 12:09 ` Ard Biesheuvel
2020-08-12 22:43 ` [PATCH v6 2/5] MdePkg: BaseRngLibDxe: Add RngLib that uses RngDxe Matthew Carlson
` (4 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: Matthew Carlson @ 2020-08-12 22:43 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Michael D Kinney, Liming Gao, Zhiguang Liu,
Matthew Carlson
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.
+ */
+VOID
+EFIAPI
+DecentDelay(
+ VOID
+ )
+{
+ UINT64 StartValue;
+ UINT64 EndValue;
+ UINT64 CounterHz;
+ UINT64 MinumumDelayInMicroSeconds;
+ // Get the counter properties
+ CounterHz = GetPerformanceCounterProperties(&StartValue, &EndValue);
+ // Make sure we won't divide by zero
+ if (CounterHz == 0) {
+ ASSERT(FALSE); // 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
+ MinumumDelayInMicroSeconds = 1500000 / CounterHz;
+
+ MicroSecondDelay(MinumumDelayInMicroSeconds);
+}
+
+
+/**
+ 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;
+
+ ASSERT (Rand != NULL);
+
+ if (NULL == Rand) {
+ return FALSE;
+ }
+
+ RandPtr = (UINT8 *) Rand;
+ // Get 2 bytes of random ish data
+ // This should take around 10us
+ for (Index = 0; Index < 2; Index ++) {
+ *RandPtr = (UINT8) (GetPerformanceCounter () & 0xFF);
+ DecentDelay (); // delay to give chance for performance counter to catch up
+ 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
+ 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
+#
+
+[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
+// 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.27.0.windows.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v6 1/5] MdePkg: TimerRngLib: Added RngLib that uses TimerLib
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
2020-08-13 18:59 ` Matthew Carlson
0 siblings, 1 reply; 16+ messages in thread
From: Ard Biesheuvel @ 2020-08-13 12:09 UTC (permalink / raw)
To: matthewfcarlson, devel; +Cc: Michael D Kinney, Liming Gao, Zhiguang Liu
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
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v6 1/5] MdePkg: TimerRngLib: Added RngLib that uses TimerLib
2020-08-13 12:09 ` Ard Biesheuvel
@ 2020-08-13 18:59 ` Matthew Carlson
0 siblings, 0 replies; 16+ messages in thread
From: Matthew Carlson @ 2020-08-13 18:59 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: devel, Michael D Kinney, Liming Gao, Zhiguang Liu
[-- Attachment #1: Type: text/plain, Size: 12233 bytes --]
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.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_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
> >
>
>
[-- Attachment #2: Type: text/html, Size: 16295 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v6 2/5] MdePkg: BaseRngLibDxe: Add RngLib that uses RngDxe
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-12 22:43 ` Matthew Carlson
2020-08-13 9:10 ` Liming Gao
2020-08-13 12:19 ` Ard Biesheuvel
2020-08-12 22:43 ` [PATCH v6 3/5] OvmfPkg: Add RngLib based on TimerLib for Crypto Matthew Carlson
` (3 subsequent siblings)
5 siblings, 2 replies; 16+ messages in thread
From: Matthew Carlson @ 2020-08-12 22:43 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Michael D Kinney, Liming Gao, Zhiguang Liu,
Matthew Carlson
From: Matthew Carlson <macarl@microsoft.com>
This adds a RngLib that uses the RngProtocol to provide randomness.
This means that the RngLib is meant to be used with DXE_DRIVERS.
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/BaseRngLibDxe/RngDxeLib.c | 200 ++++++++++++++++++++
MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf | 38 ++++
MdePkg/MdePkg.dsc | 4 +-
3 files changed, 241 insertions(+), 1 deletion(-)
diff --git a/MdePkg/Library/BaseRngLibDxe/RngDxeLib.c b/MdePkg/Library/BaseRngLibDxe/RngDxeLib.c
new file mode 100644
index 000000000000..8ee29329de13
--- /dev/null
+++ b/MdePkg/Library/BaseRngLibDxe/RngDxeLib.c
@@ -0,0 +1,200 @@
+/** @file
+ Provides an implementation of the library class RngLib that uses the Rng protocol.
+
+Copyright (c) Microsoft Corporation. All rights reserved.
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+#include <Uefi.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/DebugLib.h>
+#include <Library/RngLib.h>
+#include <Protocol/Rng.h>
+
+/**
+Routine Description:
+
+ Generates a random number via the NIST
+ 800-9A algorithm. Refer to
+ http://csrc.nist.gov/groups/STM/cavp/documents/drbg/DRBGVS.pdf
+ for more information.
+
+ Arguments:
+
+ Buffer -- Buffer to receive the random number.
+ BufferSize -- Number of bytes in Buffer.
+
+Return Value:
+
+ EFI_SUCCESS or underlying failure code.
+
+**/
+EFI_STATUS
+EFIAPI
+GenerateRandomNumberViaNist800Algorithm(
+ OUT UINT8* Buffer,
+ IN UINTN BufferSize
+ )
+{
+ EFI_STATUS Status;
+ EFI_RNG_PROTOCOL* RngProtocol;
+
+ RngProtocol = NULL;
+
+ if (Buffer == NULL) {
+ DEBUG((DEBUG_ERROR, "[%a] Buffer == NULL.\n", __FUNCTION__));
+ return EFI_INVALID_PARAMETER;
+ }
+
+ Status = gBS->LocateProtocol(&gEfiRngProtocolGuid, NULL, (VOID **)&RngProtocol);
+ if (EFI_ERROR(Status) || RngProtocol == NULL) {
+ DEBUG((DEBUG_ERROR, "[%a] Could not locate RNG prototocol, Status = %r\n", __FUNCTION__, Status));
+ return Status;
+ }
+
+ Status = RngProtocol->GetRNG(RngProtocol, &gEfiRngAlgorithmSp80090Ctr256Guid, BufferSize, Buffer);
+ DEBUG((DEBUG_INFO, "[%a] GetRNG algorithm CTR-256 - Status = %r\n", __FUNCTION__, Status));
+ if(!EFI_ERROR(Status)) {
+ return Status;
+ }
+
+ Status = RngProtocol->GetRNG(RngProtocol, &gEfiRngAlgorithmSp80090Hmac256Guid, BufferSize, Buffer);
+ DEBUG((DEBUG_INFO, "[%a] GetRNG algorithm HMAC-256 - Status = %r\n", __FUNCTION__, Status));
+ if(!EFI_ERROR(Status)) {
+ return Status;
+ }
+
+ Status = RngProtocol->GetRNG(RngProtocol, &gEfiRngAlgorithmSp80090Hash256Guid, BufferSize, Buffer);
+ DEBUG((DEBUG_INFO, "[%a] GetRNG algorithm Hash-256 - Status = %r\n", __FUNCTION__, Status));
+ if(!EFI_ERROR(Status)) {
+ return Status;
+ }
+ // If we get to this point, we have failed
+ DEBUG((DEBUG_ERROR, "[%a] GetRNG() failed, staus = %r\n", __FUNCTION__, Status));
+
+ return Status;
+}// GenerateRandomNumberViaNist800Algorithm()
+
+
+/**
+ Generates a 16-bit random number.
+
+ if Rand is NULL, return FALSE.
+
+ @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
+ )
+{
+ EFI_STATUS Status;
+
+ if (Rand == NULL)
+ {
+ return FALSE;
+ }
+
+ Status = GenerateRandomNumberViaNist800Algorithm ((UINT8 *)Rand, 2);
+ if (EFI_ERROR(Status))
+ {
+ return FALSE;
+ }
+ return TRUE;
+}
+
+/**
+ Generates a 32-bit random number.
+
+ if Rand is NULL, return FALSE.
+
+ @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
+ )
+{
+ EFI_STATUS Status;
+
+ if (Rand == NULL) {
+ return FALSE;
+ }
+
+ Status = GenerateRandomNumberViaNist800Algorithm ((UINT8 *)Rand, 4);
+ if (EFI_ERROR(Status)) {
+ return FALSE;
+ }
+ return TRUE;
+}
+
+/**
+ Generates a 64-bit random number.
+
+ if Rand is NULL, return FALSE.
+
+ @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
+ )
+{
+ EFI_STATUS Status;
+
+ if (Rand == NULL)
+ {
+ return FALSE;
+ }
+
+ Status = GenerateRandomNumberViaNist800Algorithm ((UINT8 *)Rand, 8);
+ if (EFI_ERROR(Status)) {
+ return FALSE;
+ }
+ return TRUE;
+}
+
+/**
+ Generates a 128-bit random number.
+
+ if Rand is NULL, return FALSE.
+
+ @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
+ )
+{
+ EFI_STATUS Status;
+
+ if (Rand == NULL) {
+ return FALSE;
+ }
+
+ Status = GenerateRandomNumberViaNist800Algorithm ((UINT8 *)Rand, 16);
+ if (EFI_ERROR(Status)) {
+ return FALSE;
+ }
+ return TRUE;
+}
diff --git a/MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf b/MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf
new file mode 100644
index 000000000000..819a106b1376
--- /dev/null
+++ b/MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf
@@ -0,0 +1,38 @@
+# @file
+# Provides implementation of the library class RngLib that uses the RngProtocol
+#
+# @copyright
+# Copyright (c) Microsoft Corporation. All rights reserved.
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+# MU_CHANGE: New file
+##
+
+[Defines]
+ INF_VERSION = 0x00010017
+ BASE_NAME = BaseRngLibDxe
+ FILE_GUID = FF9F84C5-A33E-44E3-9BB5-0D654B2D4149
+ MODULE_TYPE = DXE_DRIVER
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = RngLib|DXE_DRIVER UEFI_APPLICATION UEFI_DRIVER
+
+[Packages]
+ MdePkg/MdePkg.dec
+
+[Sources]
+ RngDxeLib.c
+
+[LibraryClasses]
+ DebugLib
+ UefiBootServicesTableLib
+
+[Protocols]
+ gEfiRngProtocolGuid ## CONSUMES
+
+[Depex]
+ gEfiRngProtocolGuid
+
+[Guids]
+ gEfiRngAlgorithmSp80090Ctr256Guid ## CONSUMES
+ gEfiRngAlgorithmSp80090Hash256Guid ## CONSUMES
+ gEfiRngAlgorithmSp80090Hmac256Guid ## CONSUMES
diff --git a/MdePkg/MdePkg.dsc b/MdePkg/MdePkg.dsc
index d7ba3a730909..837a0047400e 100644
--- a/MdePkg/MdePkg.dsc
+++ b/MdePkg/MdePkg.dsc
@@ -62,8 +62,10 @@
MdePkg/Library/BasePostCodeLibPort80/BasePostCodeLibPort80.inf
MdePkg/Library/BasePrintLib/BasePrintLib.inf
MdePkg/Library/BaseReportStatusCodeLibNull/BaseReportStatusCodeLibNull.inf
- MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
+ MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf
MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf
+ MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
+
MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf
MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTemplate.inf
--
2.27.0.windows.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v6 2/5] MdePkg: BaseRngLibDxe: Add RngLib that uses RngDxe
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
1 sibling, 0 replies; 16+ messages in thread
From: Liming Gao @ 2020-08-13 9:10 UTC (permalink / raw)
To: matthewfcarlson@gmail.com, devel@edk2.groups.io
Cc: Ard Biesheuvel, Kinney, Michael D, Liu, Zhiguang, Gao, Liming
Matthew:
Based on the naming rule for the libraries in MdePkg, this new library instance name should be DxeRngLib or DxeRngLibRngProtocol.
Thanks
Liming
-----Original Message-----
From: matthewfcarlson@gmail.com <matthewfcarlson@gmail.com>
Sent: 2020年8月13日 6:44
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>; Matthew Carlson <matthewfcarlson@gmail.com>
Subject: [PATCH v6 2/5] MdePkg: BaseRngLibDxe: Add RngLib that uses RngDxe
From: Matthew Carlson <macarl@microsoft.com>
This adds a RngLib that uses the RngProtocol to provide randomness.
This means that the RngLib is meant to be used with DXE_DRIVERS.
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/BaseRngLibDxe/RngDxeLib.c | 200 ++++++++++++++++++++
MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf | 38 ++++
MdePkg/MdePkg.dsc | 4 +-
3 files changed, 241 insertions(+), 1 deletion(-)
diff --git a/MdePkg/Library/BaseRngLibDxe/RngDxeLib.c b/MdePkg/Library/BaseRngLibDxe/RngDxeLib.c
new file mode 100644
index 000000000000..8ee29329de13
--- /dev/null
+++ b/MdePkg/Library/BaseRngLibDxe/RngDxeLib.c
@@ -0,0 +1,200 @@
+/** @file
+ Provides an implementation of the library class RngLib that uses the Rng protocol.
+
+Copyright (c) Microsoft Corporation. All rights reserved.
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+#include <Uefi.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/DebugLib.h>
+#include <Library/RngLib.h>
+#include <Protocol/Rng.h>
+
+/**
+Routine Description:
+
+ Generates a random number via the NIST
+ 800-9A algorithm. Refer to
+ http://csrc.nist.gov/groups/STM/cavp/documents/drbg/DRBGVS.pdf
+ for more information.
+
+ Arguments:
+
+ Buffer -- Buffer to receive the random number.
+ BufferSize -- Number of bytes in Buffer.
+
+Return Value:
+
+ EFI_SUCCESS or underlying failure code.
+
+**/
+EFI_STATUS
+EFIAPI
+GenerateRandomNumberViaNist800Algorithm(
+ OUT UINT8* Buffer,
+ IN UINTN BufferSize
+ )
+{
+ EFI_STATUS Status;
+ EFI_RNG_PROTOCOL* RngProtocol;
+
+ RngProtocol = NULL;
+
+ if (Buffer == NULL) {
+ DEBUG((DEBUG_ERROR, "[%a] Buffer == NULL.\n", __FUNCTION__));
+ return EFI_INVALID_PARAMETER;
+ }
+
+ Status = gBS->LocateProtocol(&gEfiRngProtocolGuid, NULL, (VOID **)&RngProtocol);
+ if (EFI_ERROR(Status) || RngProtocol == NULL) {
+ DEBUG((DEBUG_ERROR, "[%a] Could not locate RNG prototocol, Status = %r\n", __FUNCTION__, Status));
+ return Status;
+ }
+
+ Status = RngProtocol->GetRNG(RngProtocol, &gEfiRngAlgorithmSp80090Ctr256Guid, BufferSize, Buffer);
+ DEBUG((DEBUG_INFO, "[%a] GetRNG algorithm CTR-256 - Status = %r\n", __FUNCTION__, Status));
+ if(!EFI_ERROR(Status)) {
+ return Status;
+ }
+
+ Status = RngProtocol->GetRNG(RngProtocol, &gEfiRngAlgorithmSp80090Hmac256Guid, BufferSize, Buffer);
+ DEBUG((DEBUG_INFO, "[%a] GetRNG algorithm HMAC-256 - Status = %r\n", __FUNCTION__, Status));
+ if(!EFI_ERROR(Status)) {
+ return Status;
+ }
+
+ Status = RngProtocol->GetRNG(RngProtocol, &gEfiRngAlgorithmSp80090Hash256Guid, BufferSize, Buffer);
+ DEBUG((DEBUG_INFO, "[%a] GetRNG algorithm Hash-256 - Status = %r\n", __FUNCTION__, Status));
+ if(!EFI_ERROR(Status)) {
+ return Status;
+ }
+ // If we get to this point, we have failed
+ DEBUG((DEBUG_ERROR, "[%a] GetRNG() failed, staus = %r\n", __FUNCTION__, Status));
+
+ return Status;
+}// GenerateRandomNumberViaNist800Algorithm()
+
+
+/**
+ Generates a 16-bit random number.
+
+ if Rand is NULL, return FALSE.
+
+ @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
+ )
+{
+ EFI_STATUS Status;
+
+ if (Rand == NULL)
+ {
+ return FALSE;
+ }
+
+ Status = GenerateRandomNumberViaNist800Algorithm ((UINT8 *)Rand, 2);
+ if (EFI_ERROR(Status))
+ {
+ return FALSE;
+ }
+ return TRUE;
+}
+
+/**
+ Generates a 32-bit random number.
+
+ if Rand is NULL, return FALSE.
+
+ @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
+ )
+{
+ EFI_STATUS Status;
+
+ if (Rand == NULL) {
+ return FALSE;
+ }
+
+ Status = GenerateRandomNumberViaNist800Algorithm ((UINT8 *)Rand, 4);
+ if (EFI_ERROR(Status)) {
+ return FALSE;
+ }
+ return TRUE;
+}
+
+/**
+ Generates a 64-bit random number.
+
+ if Rand is NULL, return FALSE.
+
+ @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
+ )
+{
+ EFI_STATUS Status;
+
+ if (Rand == NULL)
+ {
+ return FALSE;
+ }
+
+ Status = GenerateRandomNumberViaNist800Algorithm ((UINT8 *)Rand, 8);
+ if (EFI_ERROR(Status)) {
+ return FALSE;
+ }
+ return TRUE;
+}
+
+/**
+ Generates a 128-bit random number.
+
+ if Rand is NULL, return FALSE.
+
+ @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
+ )
+{
+ EFI_STATUS Status;
+
+ if (Rand == NULL) {
+ return FALSE;
+ }
+
+ Status = GenerateRandomNumberViaNist800Algorithm ((UINT8 *)Rand, 16);
+ if (EFI_ERROR(Status)) {
+ return FALSE;
+ }
+ return TRUE;
+}
diff --git a/MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf b/MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf
new file mode 100644
index 000000000000..819a106b1376
--- /dev/null
+++ b/MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf
@@ -0,0 +1,38 @@
+# @file
+# Provides implementation of the library class RngLib that uses the RngProtocol
+#
+# @copyright
+# Copyright (c) Microsoft Corporation. All rights reserved.
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+# MU_CHANGE: New file
+##
+
+[Defines]
+ INF_VERSION = 0x00010017
+ BASE_NAME = BaseRngLibDxe
+ FILE_GUID = FF9F84C5-A33E-44E3-9BB5-0D654B2D4149
+ MODULE_TYPE = DXE_DRIVER
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = RngLib|DXE_DRIVER UEFI_APPLICATION UEFI_DRIVER
+
+[Packages]
+ MdePkg/MdePkg.dec
+
+[Sources]
+ RngDxeLib.c
+
+[LibraryClasses]
+ DebugLib
+ UefiBootServicesTableLib
+
+[Protocols]
+ gEfiRngProtocolGuid ## CONSUMES
+
+[Depex]
+ gEfiRngProtocolGuid
+
+[Guids]
+ gEfiRngAlgorithmSp80090Ctr256Guid ## CONSUMES
+ gEfiRngAlgorithmSp80090Hash256Guid ## CONSUMES
+ gEfiRngAlgorithmSp80090Hmac256Guid ## CONSUMES
diff --git a/MdePkg/MdePkg.dsc b/MdePkg/MdePkg.dsc
index d7ba3a730909..837a0047400e 100644
--- a/MdePkg/MdePkg.dsc
+++ b/MdePkg/MdePkg.dsc
@@ -62,8 +62,10 @@
MdePkg/Library/BasePostCodeLibPort80/BasePostCodeLibPort80.inf
MdePkg/Library/BasePrintLib/BasePrintLib.inf
MdePkg/Library/BaseReportStatusCodeLibNull/BaseReportStatusCodeLibNull.inf
- MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
+ MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf
MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf
+ MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
+
MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf
MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTemplate.inf
--
2.27.0.windows.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v6 2/5] MdePkg: BaseRngLibDxe: Add RngLib that uses RngDxe
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
1 sibling, 1 reply; 16+ messages in thread
From: Ard Biesheuvel @ 2020-08-13 12:19 UTC (permalink / raw)
To: matthewfcarlson, devel; +Cc: Michael D Kinney, Liming Gao, Zhiguang Liu
On 8/13/20 12:43 AM, matthewfcarlson@gmail.com wrote:
> From: Matthew Carlson <macarl@microsoft.com>
>
> This adds a RngLib that uses the RngProtocol to provide randomness.
> This means that the RngLib is meant to be used with DXE_DRIVERS.
>
> 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/BaseRngLibDxe/RngDxeLib.c | 200 ++++++++++++++++++++
> MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf | 38 ++++
> MdePkg/MdePkg.dsc | 4 +-
> 3 files changed, 241 insertions(+), 1 deletion(-)
>
> diff --git a/MdePkg/Library/BaseRngLibDxe/RngDxeLib.c b/MdePkg/Library/BaseRngLibDxe/RngDxeLib.c
> new file mode 100644
> index 000000000000..8ee29329de13
> --- /dev/null
> +++ b/MdePkg/Library/BaseRngLibDxe/RngDxeLib.c
> @@ -0,0 +1,200 @@
> +/** @file
> + Provides an implementation of the library class RngLib that uses the Rng protocol.
> +
> +Copyright (c) Microsoft Corporation. All rights reserved.
> +SPDX-License-Identifier: BSD-2-Clause-Patent
Please use matching indentation
> +
> +**/
> +#include <Uefi.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/RngLib.h>
> +#include <Protocol/Rng.h>
> +
> +/**
> +Routine Description:
> +
> + Generates a random number via the NIST
> + 800-9A algorithm. Refer to
> + http://csrc.nist.gov/groups/STM/cavp/documents/drbg/DRBGVS.pdf
> + for more information.
> +
> + Arguments:
> +
> + Buffer -- Buffer to receive the random number.
> + BufferSize -- Number of bytes in Buffer.
> +
> +Return Value:
> +
> + EFI_SUCCESS or underlying failure code.
> +
> +**/
STATIC ?
> +EFI_STATUS
> +EFIAPI
> +GenerateRandomNumberViaNist800Algorithm(
space before (
> + OUT UINT8* Buffer,
put * on the rhs
> + IN UINTN BufferSize
> + )
> +{
> + EFI_STATUS Status;
> + EFI_RNG_PROTOCOL* RngProtocol;
likewise
> +
> + RngProtocol = NULL;
> +
> + if (Buffer == NULL) {
> + DEBUG((DEBUG_ERROR, "[%a] Buffer == NULL.\n", __FUNCTION__));
Could you drop the [] around the function name? This is rather
unidiomatic for EDK2
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + Status = gBS->LocateProtocol(&gEfiRngProtocolGuid, NULL, (VOID **)&RngProtocol);
Space before (
> + if (EFI_ERROR(Status) || RngProtocol == NULL) {
Space before (. Also, I think the second condition could be an ASSERT()
> + DEBUG((DEBUG_ERROR, "[%a] Could not locate RNG prototocol, Status = %r\n", __FUNCTION__, Status));
> + return Status;
> + }
> +
> + Status = RngProtocol->GetRNG(RngProtocol, &gEfiRngAlgorithmSp80090Ctr256Guid, BufferSize, Buffer);
> + DEBUG((DEBUG_INFO, "[%a] GetRNG algorithm CTR-256 - Status = %r\n", __FUNCTION__, Status)); > + if(!EFI_ERROR(Status)) {
Space after 'if' and before (.
Please do a pass over all the patches, I will stop pointing out the
spacing around ( from this point.
> + return Status;
> + }
> +
> + Status = RngProtocol->GetRNG(RngProtocol, &gEfiRngAlgorithmSp80090Hmac256Guid, BufferSize, Buffer);
> + DEBUG((DEBUG_INFO, "[%a] GetRNG algorithm HMAC-256 - Status = %r\n", __FUNCTION__, Status));
> + if(!EFI_ERROR(Status)) {
> + return Status;
> + }
> +
> + Status = RngProtocol->GetRNG(RngProtocol, &gEfiRngAlgorithmSp80090Hash256Guid, BufferSize, Buffer);
> + DEBUG((DEBUG_INFO, "[%a] GetRNG algorithm Hash-256 - Status = %r\n", __FUNCTION__, Status));
> + if(!EFI_ERROR(Status)) {
> + return Status;
> + }
I don't like this 'success handling' pattern tbh. Also, why are these
algorithms singled out like this? EFI_RNG_PROTOCOL typically has a
default algorithm, and even raw entropy is perfectly suitable for key
generation (although perhaps slightly wasteful in some case)
I am aware there is a check in the RdRand RngDxe that refuses to return
32 bytes from the raw algorithm, but this is simply a misinterpretation
of the spec that we should fix at some point,
> + // If we get to this point, we have failed
> + DEBUG((DEBUG_ERROR, "[%a] GetRNG() failed, staus = %r\n", __FUNCTION__, Status));
> +
> + return Status;
> +}// GenerateRandomNumberViaNist800Algorithm()
> +
> +
> +/**
> + Generates a 16-bit random number.
> +
> + if Rand is NULL, return FALSE.
> +
Can't we simply stipulate that Rand != NULL? Is there ever a point to
calling this function with a NULL buffer?
> + @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
> + )
> +{
> + EFI_STATUS Status;
> +
> + if (Rand == NULL)
> + {
> + return FALSE;
> + }
> +
> + Status = GenerateRandomNumberViaNist800Algorithm ((UINT8 *)Rand, 2);
> + if (EFI_ERROR(Status))
> + {
> + return FALSE;
> + }
> + return TRUE;
> +}
> +
> +/**
> + Generates a 32-bit random number.
> +
> + if Rand is NULL, return FALSE.
> +
> + @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
> + )
> +{
> + EFI_STATUS Status;
> +
> + if (Rand == NULL) {
> + return FALSE;
> + }
> +
> + Status = GenerateRandomNumberViaNist800Algorithm ((UINT8 *)Rand, 4);
> + if (EFI_ERROR(Status)) {
> + return FALSE;
> + }
> + return TRUE;
> +}
> +
> +/**
> + Generates a 64-bit random number.
> +
> + if Rand is NULL, return FALSE.
> +
> + @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
> + )
> +{
> + EFI_STATUS Status;
> +
> + if (Rand == NULL)
> + {
> + return FALSE;
> + }
> +
> + Status = GenerateRandomNumberViaNist800Algorithm ((UINT8 *)Rand, 8);
> + if (EFI_ERROR(Status)) {
> + return FALSE;
> + }
> + return TRUE;
> +}
> +
> +/**
> + Generates a 128-bit random number.
> +
> + if Rand is NULL, return FALSE.
> +
> + @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
> + )
> +{
> + EFI_STATUS Status;
> +
> + if (Rand == NULL) {
> + return FALSE;
> + }
> +
> + Status = GenerateRandomNumberViaNist800Algorithm ((UINT8 *)Rand, 16);
> + if (EFI_ERROR(Status)) {
> + return FALSE;
> + }
> + return TRUE;
> +}
> diff --git a/MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf b/MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf
> new file mode 100644
> index 000000000000..819a106b1376
> --- /dev/null
> +++ b/MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf
> @@ -0,0 +1,38 @@
> +# @file
> +# Provides implementation of the library class RngLib that uses the RngProtocol
> +#
> +# @copyright
> +# Copyright (c) Microsoft Corporation. All rights reserved.
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +# MU_CHANGE: New file
Drop this please
> +##
> +
> +[Defines]
> + INF_VERSION = 0x00010017
> + BASE_NAME = BaseRngLibDxe
> + FILE_GUID = FF9F84C5-A33E-44E3-9BB5-0D654B2D4149
> + MODULE_TYPE = DXE_DRIVER
> + VERSION_STRING = 1.0
> + LIBRARY_CLASS = RngLib|DXE_DRIVER UEFI_APPLICATION UEFI_DRIVER
> +
> +[Packages]
> + MdePkg/MdePkg.dec
> +
> +[Sources]
> + RngDxeLib.c
> +
> +[LibraryClasses]
> + DebugLib
> + UefiBootServicesTableLib
> +
> +[Protocols]
> + gEfiRngProtocolGuid ## CONSUMES
> +
> +[Depex]
> + gEfiRngProtocolGuid
> +
> +[Guids]
> + gEfiRngAlgorithmSp80090Ctr256Guid ## CONSUMES
> + gEfiRngAlgorithmSp80090Hash256Guid ## CONSUMES
> + gEfiRngAlgorithmSp80090Hmac256Guid ## CONSUMES
I'm not sure these annotations are appropriate for the [Guids] section
> diff --git a/MdePkg/MdePkg.dsc b/MdePkg/MdePkg.dsc
> index d7ba3a730909..837a0047400e 100644
> --- a/MdePkg/MdePkg.dsc
> +++ b/MdePkg/MdePkg.dsc
> @@ -62,8 +62,10 @@
> MdePkg/Library/BasePostCodeLibPort80/BasePostCodeLibPort80.inf
> MdePkg/Library/BasePrintLib/BasePrintLib.inf
> MdePkg/Library/BaseReportStatusCodeLibNull/BaseReportStatusCodeLibNull.inf
> - MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
> + MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf
> MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf
> + MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
> +
> MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf
> MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
> MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTemplate.inf
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v6 2/5] MdePkg: BaseRngLibDxe: Add RngLib that uses RngDxe
2020-08-13 12:19 ` Ard Biesheuvel
@ 2020-08-13 19:18 ` Matthew Carlson
0 siblings, 0 replies; 16+ messages in thread
From: Matthew Carlson @ 2020-08-13 19:18 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: devel, Michael D Kinney, Liming Gao, Zhiguang Liu
[-- Attachment #1: Type: text/plain, Size: 10931 bytes --]
Thanks for the feedback.
I've addressed all the comments except the one about the success handling
pattern. I think the algorithms it requests are made in a specific order so
that it can make some promising regarding the validity of its random number
generation. That said, this is code that another coworker at Microsoft
wrote, so I'm not 100% sure why it does that this particular way.
Do you have a suggestion about what sort of algorithm should be selected?
Perhaps just using the default every time? Keep the pattern as it stands
now but add a final check to use the default if the previous ones fail?
I kept in the check for NULL since any inputs should be
sanitized regardless of where they're coming from. I'm open to adding an
assert there as well to help debugability.
-Matthew Carlson
On Thu, Aug 13, 2020 at 5:19 AM Ard Biesheuvel <ard.biesheuvel@arm.com>
wrote:
> On 8/13/20 12:43 AM, matthewfcarlson@gmail.com wrote:
> > From: Matthew Carlson <macarl@microsoft.com>
> >
> > This adds a RngLib that uses the RngProtocol to provide randomness.
> > This means that the RngLib is meant to be used with DXE_DRIVERS.
> >
> > 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/BaseRngLibDxe/RngDxeLib.c | 200
> ++++++++++++++++++++
> > MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf | 38 ++++
> > MdePkg/MdePkg.dsc | 4 +-
> > 3 files changed, 241 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdePkg/Library/BaseRngLibDxe/RngDxeLib.c
> b/MdePkg/Library/BaseRngLibDxe/RngDxeLib.c
> > new file mode 100644
> > index 000000000000..8ee29329de13
> > --- /dev/null
> > +++ b/MdePkg/Library/BaseRngLibDxe/RngDxeLib.c
> > @@ -0,0 +1,200 @@
> > +/** @file
> > + Provides an implementation of the library class RngLib that uses the
> Rng protocol.
> > +
> > +Copyright (c) Microsoft Corporation. All rights reserved.
> > +SPDX-License-Identifier: BSD-2-Clause-Patent
>
> Please use matching indentation
>
> > +
> > +**/
> > +#include <Uefi.h>
> > +#include <Library/UefiBootServicesTableLib.h>
> > +#include <Library/DebugLib.h>
> > +#include <Library/RngLib.h>
> > +#include <Protocol/Rng.h>
> > +
> > +/**
> > +Routine Description:
> > +
> > + Generates a random number via the NIST
> > + 800-9A algorithm. Refer to
> > + http://csrc.nist.gov/groups/STM/cavp/documents/drbg/DRBGVS.pdf
> > + for more information.
> > +
> > + Arguments:
> > +
> > + Buffer -- Buffer to receive the random number.
> > + BufferSize -- Number of bytes in Buffer.
> > +
> > +Return Value:
> > +
> > + EFI_SUCCESS or underlying failure code.
> > +
> > +**/
>
> STATIC ?
>
> > +EFI_STATUS
> > +EFIAPI
> > +GenerateRandomNumberViaNist800Algorithm(
>
> space before (
>
> > + OUT UINT8* Buffer,
>
> put * on the rhs
>
> > + IN UINTN BufferSize
> > + )
> > +{
> > + EFI_STATUS Status;
> > + EFI_RNG_PROTOCOL* RngProtocol;
>
> likewise
>
> > +
> > + RngProtocol = NULL;
> > +
> > + if (Buffer == NULL) {
> > + DEBUG((DEBUG_ERROR, "[%a] Buffer == NULL.\n", __FUNCTION__));
>
> Could you drop the [] around the function name? This is rather
> unidiomatic for EDK2
>
> > + return EFI_INVALID_PARAMETER;
> > + }
> > +
> > + Status = gBS->LocateProtocol(&gEfiRngProtocolGuid, NULL, (VOID
> **)&RngProtocol);
>
> Space before (
>
> > + if (EFI_ERROR(Status) || RngProtocol == NULL) {
>
> Space before (. Also, I think the second condition could be an ASSERT()
>
> > + DEBUG((DEBUG_ERROR, "[%a] Could not locate RNG prototocol, Status
> = %r\n", __FUNCTION__, Status));
> > + return Status;
> > + }
> > +
> > + Status = RngProtocol->GetRNG(RngProtocol,
> &gEfiRngAlgorithmSp80090Ctr256Guid, BufferSize, Buffer);
> > + DEBUG((DEBUG_INFO, "[%a] GetRNG algorithm CTR-256 - Status = %r\n",
> __FUNCTION__, Status)); > + if(!EFI_ERROR(Status)) {
>
> Space after 'if' and before (.
>
> Please do a pass over all the patches, I will stop pointing out the
> spacing around ( from this point.
>
>
> > + return Status;
> > + }
> > +
> > + Status = RngProtocol->GetRNG(RngProtocol,
> &gEfiRngAlgorithmSp80090Hmac256Guid, BufferSize, Buffer);
> > + DEBUG((DEBUG_INFO, "[%a] GetRNG algorithm HMAC-256 - Status = %r\n",
> __FUNCTION__, Status));
> > + if(!EFI_ERROR(Status)) {
> > + return Status;
> > + }
> > +
> > + Status = RngProtocol->GetRNG(RngProtocol,
> &gEfiRngAlgorithmSp80090Hash256Guid, BufferSize, Buffer);
> > + DEBUG((DEBUG_INFO, "[%a] GetRNG algorithm Hash-256 - Status = %r\n",
> __FUNCTION__, Status));
> > + if(!EFI_ERROR(Status)) {
> > + return Status;
> > + }
>
> I don't like this 'success handling' pattern tbh. Also, why are these
> algorithms singled out like this? EFI_RNG_PROTOCOL typically has a
> default algorithm, and even raw entropy is perfectly suitable for key
> generation (although perhaps slightly wasteful in some case)
>
> I am aware there is a check in the RdRand RngDxe that refuses to return
> 32 bytes from the raw algorithm, but this is simply a misinterpretation
> of the spec that we should fix at some point,
>
> > + // If we get to this point, we have failed
> > + DEBUG((DEBUG_ERROR, "[%a] GetRNG() failed, staus = %r\n",
> __FUNCTION__, Status));
> > +
> > + return Status;
> > +}// GenerateRandomNumberViaNist800Algorithm()
> > +
> > +
> > +/**
> > + Generates a 16-bit random number.
> > +
> > + if Rand is NULL, return FALSE.
> > +
>
> Can't we simply stipulate that Rand != NULL? Is there ever a point to
> calling this function with a NULL buffer?
>
> > + @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
> > + )
> > +{
> > + EFI_STATUS Status;
> > +
> > + if (Rand == NULL)
> > + {
> > + return FALSE;
> > + }
> > +
> > + Status = GenerateRandomNumberViaNist800Algorithm ((UINT8 *)Rand, 2);
> > + if (EFI_ERROR(Status))
> > + {
> > + return FALSE;
> > + }
> > + return TRUE;
> > +}
> > +
> > +/**
> > + Generates a 32-bit random number.
> > +
> > + if Rand is NULL, return FALSE.
> > +
> > + @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
> > + )
> > +{
> > + EFI_STATUS Status;
> > +
> > + if (Rand == NULL) {
> > + return FALSE;
> > + }
> > +
> > + Status = GenerateRandomNumberViaNist800Algorithm ((UINT8 *)Rand, 4);
> > + if (EFI_ERROR(Status)) {
> > + return FALSE;
> > + }
> > + return TRUE;
> > +}
> > +
> > +/**
> > + Generates a 64-bit random number.
> > +
> > + if Rand is NULL, return FALSE.
> > +
> > + @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
> > + )
> > +{
> > + EFI_STATUS Status;
> > +
> > + if (Rand == NULL)
> > + {
> > + return FALSE;
> > + }
> > +
> > + Status = GenerateRandomNumberViaNist800Algorithm ((UINT8 *)Rand, 8);
> > + if (EFI_ERROR(Status)) {
> > + return FALSE;
> > + }
> > + return TRUE;
> > +}
> > +
> > +/**
> > + Generates a 128-bit random number.
> > +
> > + if Rand is NULL, return FALSE.
> > +
> > + @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
> > + )
> > +{
> > + EFI_STATUS Status;
> > +
> > + if (Rand == NULL) {
> > + return FALSE;
> > + }
> > +
> > + Status = GenerateRandomNumberViaNist800Algorithm ((UINT8 *)Rand, 16);
> > + if (EFI_ERROR(Status)) {
> > + return FALSE;
> > + }
> > + return TRUE;
> > +}
> > diff --git a/MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf
> b/MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf
> > new file mode 100644
> > index 000000000000..819a106b1376
> > --- /dev/null
> > +++ b/MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf
> > @@ -0,0 +1,38 @@
> > +# @file
> > +# Provides implementation of the library class RngLib that uses the
> RngProtocol
> > +#
> > +# @copyright
> > +# Copyright (c) Microsoft Corporation. All rights reserved.
> > +# SPDX-License-Identifier: BSD-2-Clause-Patent
> > +#
> > +# MU_CHANGE: New file
>
> Drop this please
>
> > +##
> > +
> > +[Defines]
> > + INF_VERSION = 0x00010017
> > + BASE_NAME = BaseRngLibDxe
> > + FILE_GUID = FF9F84C5-A33E-44E3-9BB5-0D654B2D4149
> > + MODULE_TYPE = DXE_DRIVER
> > + VERSION_STRING = 1.0
> > + LIBRARY_CLASS = RngLib|DXE_DRIVER UEFI_APPLICATION UEFI_DRIVER
> > +
> > +[Packages]
> > + MdePkg/MdePkg.dec
> > +
> > +[Sources]
> > + RngDxeLib.c
> > +
> > +[LibraryClasses]
> > + DebugLib
> > + UefiBootServicesTableLib
> > +
> > +[Protocols]
> > + gEfiRngProtocolGuid ## CONSUMES
> > +
> > +[Depex]
> > + gEfiRngProtocolGuid
> > +
> > +[Guids]
> > + gEfiRngAlgorithmSp80090Ctr256Guid ## CONSUMES
> > + gEfiRngAlgorithmSp80090Hash256Guid ## CONSUMES
> > + gEfiRngAlgorithmSp80090Hmac256Guid ## CONSUMES
>
> I'm not sure these annotations are appropriate for the [Guids] section
>
> > diff --git a/MdePkg/MdePkg.dsc b/MdePkg/MdePkg.dsc
> > index d7ba3a730909..837a0047400e 100644
> > --- a/MdePkg/MdePkg.dsc
> > +++ b/MdePkg/MdePkg.dsc
> > @@ -62,8 +62,10 @@
> > MdePkg/Library/BasePostCodeLibPort80/BasePostCodeLibPort80.inf
> > MdePkg/Library/BasePrintLib/BasePrintLib.inf
> >
> MdePkg/Library/BaseReportStatusCodeLibNull/BaseReportStatusCodeLibNull.inf
> > - MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
> > + MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf
> > MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf
> > + MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
> > +
> > MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf
> > MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
> > MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTemplate.inf
> >
>
>
[-- Attachment #2: Type: text/html, Size: 14604 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v6 3/5] OvmfPkg: Add RngLib based on TimerLib for Crypto
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-12 22:43 ` [PATCH v6 2/5] MdePkg: BaseRngLibDxe: Add RngLib that uses RngDxe Matthew Carlson
@ 2020-08-12 22:43 ` 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
` (2 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: Matthew Carlson @ 2020-08-12 22:43 UTC (permalink / raw)
To: devel
Cc: Jordan Justen, Laszlo Ersek, Ard Biesheuvel, Anthony Perard,
Julien Grall, Matthew Carlson
From: Matthew Carlson <macarl@microsoft.com>
Updates the DSC's for Ovmf based platforms to add a RngLib that uses the
TimerLib. This is due to a later change that adds TimerLib as a dependency
for OpenSSL. The TimerLib based RngLib mimics the behavior of OpenSSL
previously and it is recommended to switch to a better source of
entropy than the system's performance counter.
Ref: https://github.com/tianocore/edk2/pull/845
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1871
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Julien Grall <julien@xen.org>
Signed-off-by: Matthew Carlson <matthewfcarlson@gmail.com>
Reviewed-by: Laszlo Ersek <lersek@...>
---
OvmfPkg/Bhyve/BhyvePkgX64.dsc | 1 +
OvmfPkg/OvmfPkgIa32.dsc | 1 +
OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
OvmfPkg/OvmfPkgX64.dsc | 1 +
OvmfPkg/OvmfXen.dsc | 1 +
5 files changed, 5 insertions(+)
diff --git a/OvmfPkg/Bhyve/BhyvePkgX64.dsc b/OvmfPkg/Bhyve/BhyvePkgX64.dsc
index 8eba48d109a3..4b1fbb361b28 100644
--- a/OvmfPkg/Bhyve/BhyvePkgX64.dsc
+++ b/OvmfPkg/Bhyve/BhyvePkgX64.dsc
@@ -185,6 +185,7 @@
!else
OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
!endif
+ RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
!if $(SECURE_BOOT_ENABLE) == TRUE
PlatformSecureLib|OvmfPkg/Bhyve/Library/PlatformSecureLib/PlatformSecureLib.inf
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 9178ffeb71cb..fc1c8014eba1 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -189,6 +189,7 @@
!else
OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
!endif
+ RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
!if $(SECURE_BOOT_ENABLE) == TRUE
PlatformSecureLib|OvmfPkg/Library/PlatformSecureLib/PlatformSecureLib.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index a665f78f0dc7..2365dc2fa98d 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -193,6 +193,7 @@
!else
OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
!endif
+ RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
!if $(SECURE_BOOT_ENABLE) == TRUE
PlatformSecureLib|OvmfPkg/Library/PlatformSecureLib/PlatformSecureLib.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 17f345acf4ee..6992cfd98b70 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -193,6 +193,7 @@
!else
OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
!endif
+ RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
!if $(SECURE_BOOT_ENABLE) == TRUE
PlatformSecureLib|OvmfPkg/Library/PlatformSecureLib/PlatformSecureLib.inf
diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 782803cb2787..416f81f06a04 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -179,6 +179,7 @@
!else
OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
!endif
+ RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf
VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf
--
2.27.0.windows.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v6 3/5] OvmfPkg: Add RngLib based on TimerLib for Crypto
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
0 siblings, 0 replies; 16+ messages in thread
From: Laszlo Ersek @ 2020-08-13 8:34 UTC (permalink / raw)
To: matthewfcarlson, devel
Cc: Jordan Justen, Ard Biesheuvel, Anthony Perard, Julien Grall
Hi Matthew,
On 08/13/20 00:43, matthewfcarlson@gmail.com wrote:
> From: Matthew Carlson <macarl@microsoft.com>
>
> Updates the DSC's for Ovmf based platforms to add a RngLib that uses the
> TimerLib. This is due to a later change that adds TimerLib as a dependency
> for OpenSSL. The TimerLib based RngLib mimics the behavior of OpenSSL
> previously and it is recommended to switch to a better source of
> entropy than the system's performance counter.
>
> Ref: https://github.com/tianocore/edk2/pull/845
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1871
>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Julien Grall <julien@xen.org>
> Signed-off-by: Matthew Carlson <matthewfcarlson@gmail.com>
> Reviewed-by: Laszlo Ersek <lersek@...>
I think you must have copied my R-b from a mailing list archive (on the
web) into this commit message, and not from an email of mine. That's
because my email address is truncated above, similarly to how the
archives display email addresses (for fighting spam).
Please don't repost the series just for this; the maintainer that merges
this series can (and should) fix up this wart just in time. However, if
a v7 becomes necessary, please do refresh my email address above. For
convenience, I'll repeat my R-b here:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks!
Laszlo
> ---
> OvmfPkg/Bhyve/BhyvePkgX64.dsc | 1 +
> OvmfPkg/OvmfPkgIa32.dsc | 1 +
> OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
> OvmfPkg/OvmfPkgX64.dsc | 1 +
> OvmfPkg/OvmfXen.dsc | 1 +
> 5 files changed, 5 insertions(+)
>
> diff --git a/OvmfPkg/Bhyve/BhyvePkgX64.dsc b/OvmfPkg/Bhyve/BhyvePkgX64.dsc
> index 8eba48d109a3..4b1fbb361b28 100644
> --- a/OvmfPkg/Bhyve/BhyvePkgX64.dsc
> +++ b/OvmfPkg/Bhyve/BhyvePkgX64.dsc
> @@ -185,6 +185,7 @@
> !else
> OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
> !endif
> + RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
>
> !if $(SECURE_BOOT_ENABLE) == TRUE
> PlatformSecureLib|OvmfPkg/Bhyve/Library/PlatformSecureLib/PlatformSecureLib.inf
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 9178ffeb71cb..fc1c8014eba1 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -189,6 +189,7 @@
> !else
> OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
> !endif
> + RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
>
> !if $(SECURE_BOOT_ENABLE) == TRUE
> PlatformSecureLib|OvmfPkg/Library/PlatformSecureLib/PlatformSecureLib.inf
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index a665f78f0dc7..2365dc2fa98d 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -193,6 +193,7 @@
> !else
> OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
> !endif
> + RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
>
> !if $(SECURE_BOOT_ENABLE) == TRUE
> PlatformSecureLib|OvmfPkg/Library/PlatformSecureLib/PlatformSecureLib.inf
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 17f345acf4ee..6992cfd98b70 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -193,6 +193,7 @@
> !else
> OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
> !endif
> + RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
>
> !if $(SECURE_BOOT_ENABLE) == TRUE
> PlatformSecureLib|OvmfPkg/Library/PlatformSecureLib/PlatformSecureLib.inf
> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
> index 782803cb2787..416f81f06a04 100644
> --- a/OvmfPkg/OvmfXen.dsc
> +++ b/OvmfPkg/OvmfXen.dsc
> @@ -179,6 +179,7 @@
> !else
> OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
> !endif
> + RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
>
> AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf
> VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v6 4/5] ArmVirtPkg: Add RngLib based on TimerLib for CryptoPkg
2020-08-12 22:43 [PATCH v6 0/5] Use RngLib instead of TimerLib for OpensslLib Matthew Carlson
` (2 preceding siblings ...)
2020-08-12 22:43 ` [PATCH v6 3/5] OvmfPkg: Add RngLib based on TimerLib for Crypto Matthew Carlson
@ 2020-08-12 22:43 ` 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 15:14 ` [PATCH v6 0/5] Use RngLib instead of TimerLib for OpensslLib Yao, Jiewen
5 siblings, 1 reply; 16+ messages in thread
From: Matthew Carlson @ 2020-08-12 22:43 UTC (permalink / raw)
To: devel; +Cc: Laszlo Ersek, Ard Biesheuvel, Leif Lindholm, Matthew Carlson
From: Matthew Carlson <macarl@microsoft.com>
Updates the DSC for the ArmVirtPkg platform to add a RngLib that uses the
TimerLib. This is due to a later change that adds TimerLib as a dependency
for OpenSSL. The TimerLib based RngLib mimics the behavior of OpenSSL
previously and it is recommended to switch to a better source of
entropy than the system's performance counter.
Ref: https://github.com/tianocore/edk2/pull/845
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1871
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Signed-off-by: Matthew Carlson <matthewfcarlson@gmail.com>
---
ArmVirtPkg/ArmVirt.dsc.inc | 1 +
1 file changed, 1 insertion(+)
diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index cf44fc73890b..cb3845d2bd37 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -160,6 +160,7 @@
OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
!endif
BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
+ RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
#
# Secure Boot dependencies
--
2.27.0.windows.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v6 4/5] ArmVirtPkg: Add RngLib based on TimerLib for CryptoPkg
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
0 siblings, 0 replies; 16+ messages in thread
From: Laszlo Ersek @ 2020-08-13 8:31 UTC (permalink / raw)
To: matthewfcarlson, devel; +Cc: Ard Biesheuvel, Leif Lindholm
On 08/13/20 00:43, matthewfcarlson@gmail.com wrote:
> From: Matthew Carlson <macarl@microsoft.com>
>
> Updates the DSC for the ArmVirtPkg platform to add a RngLib that uses the
> TimerLib. This is due to a later change that adds TimerLib as a dependency
> for OpenSSL. The TimerLib based RngLib mimics the behavior of OpenSSL
> previously and it is recommended to switch to a better source of
> entropy than the system's performance counter.
>
> Ref: https://github.com/tianocore/edk2/pull/845
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1871
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Signed-off-by: Matthew Carlson <matthewfcarlson@gmail.com>
> ---
> ArmVirtPkg/ArmVirt.dsc.inc | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index cf44fc73890b..cb3845d2bd37 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -160,6 +160,7 @@
> OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
> !endif
> BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> + RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
>
> #
> # Secure Boot dependencies
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v6 5/5] CryptoPkg: OpensslLib: Use RngLib to generate entropy in rand_pool
2020-08-12 22:43 [PATCH v6 0/5] Use RngLib instead of TimerLib for OpensslLib Matthew Carlson
` (3 preceding siblings ...)
2020-08-12 22:43 ` [PATCH v6 4/5] ArmVirtPkg: Add RngLib based on TimerLib for CryptoPkg Matthew Carlson
@ 2020-08-12 22:43 ` 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
5 siblings, 1 reply; 16+ messages in thread
From: Matthew Carlson @ 2020-08-12 22:43 UTC (permalink / raw)
To: devel; +Cc: Ard Biesheuvel, Jiewen Yao, Jian J Wang, Xiaoyu Lu,
Matthew Carlson
From: Matthew Carlson <macarl@microsoft.com>
Ref: https://github.com/tianocore/edk2/pull/845
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1871
Changes OpenSSL to no longer depend on TimerLib and instead use RngLib.
This allows platforms to decide for themsevles what sort of entropy source
they provide to OpenSSL and TlsLib.
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
Signed-off-by: Matthew Carlson <matthewfcarlson@gmail.com>
---
CryptoPkg/Library/OpensslLib/rand_pool.c | 203 ++------------------
CryptoPkg/Library/OpensslLib/rand_pool_noise.c | 29 ---
CryptoPkg/Library/OpensslLib/rand_pool_noise_tsc.c | 43 -----
CryptoPkg/CryptoPkg.dsc | 1 +
CryptoPkg/Library/OpensslLib/OpensslLib.inf | 15 +-
CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf | 15 +-
CryptoPkg/Library/OpensslLib/rand_pool_noise.h | 29 ---
7 files changed, 22 insertions(+), 313 deletions(-)
diff --git a/CryptoPkg/Library/OpensslLib/rand_pool.c b/CryptoPkg/Library/OpensslLib/rand_pool.c
index 9e0179b03490..3da92699fef6 100644
--- a/CryptoPkg/Library/OpensslLib/rand_pool.c
+++ b/CryptoPkg/Library/OpensslLib/rand_pool.c
@@ -11,53 +11,18 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <openssl/aes.h>
#include <Uefi.h>
-#include <Library/TimerLib.h>
-
-#include "rand_pool_noise.h"
-
-/**
- Get some randomness from low-order bits of GetPerformanceCounter results.
- And combine them to the 64-bit value
-
- @param[out] Rand Buffer pointer to store the 64-bit random value.
-
- @retval TRUE Random number generated successfully.
- @retval FALSE Failed to generate.
-**/
-STATIC
-BOOLEAN
-EFIAPI
-GetRandNoise64FromPerformanceCounter(
- OUT UINT64 *Rand
- )
-{
- UINT32 Index;
- UINT32 *RandPtr;
-
- if (NULL == Rand) {
- return FALSE;
- }
-
- RandPtr = (UINT32 *) Rand;
-
- for (Index = 0; Index < 2; Index ++) {
- *RandPtr = (UINT32) (GetPerformanceCounter () & 0xFF);
- MicroSecondDelay (10);
- RandPtr++;
- }
-
- return TRUE;
-}
+#include <Library/RngLib.h>
/**
Calls RandomNumber64 to fill
a buffer of arbitrary size with random bytes.
+ This is a shim layer to RngLib.
@param[in] Length Size of the buffer, in bytes, to fill with.
@param[out] RandBuffer Pointer to the buffer to store the random result.
- @retval EFI_SUCCESS Random bytes generation succeeded.
- @retval EFI_NOT_READY Failed to request random bytes.
+ @retval TRUE Random bytes generation succeeded.
+ @retval FALSE Failed to request random bytes.
**/
STATIC
@@ -73,17 +38,17 @@ RandGetBytes (
Ret = FALSE;
+ if (RandBuffer == NULL) {
+ DEBUG((DEBUG_ERROR, "[OPENSSL_RAND_POOL] NULL RandBuffer. No random numbers are generated and your system is not secure\n"));
+ ASSERT(FALSE); // Since we can't generate random numbers, we should assert. Otherwise we will just blow up later.
+ return Ret;
+ }
+
+
while (Length > 0) {
- //
- // Get random noise from platform.
- // If it failed, fallback to PerformanceCounter
- // If you really care about security, you must override
- // GetRandomNoise64FromPlatform.
- //
- Ret = GetRandomNoise64 (&TempRand);
- if (Ret == FALSE) {
- Ret = GetRandNoise64FromPerformanceCounter (&TempRand);
- }
+ // Use RngLib to get random number
+ Ret = GetRandomNumber64(&TempRand);
+
if (!Ret) {
return Ret;
}
@@ -100,125 +65,6 @@ RandGetBytes (
return Ret;
}
-/**
- Creates a 128bit random value that is fully forward and backward prediction resistant,
- suitable for seeding a NIST SP800-90 Compliant.
- This function takes multiple random numbers from PerformanceCounter to ensure reseeding
- and performs AES-CBC-MAC over the data to compute the seed value.
-
- @param[out] SeedBuffer Pointer to a 128bit buffer to store the random seed.
-
- @retval TRUE Random seed generation succeeded.
- @retval FALSE Failed to request random bytes.
-
-**/
-STATIC
-BOOLEAN
-EFIAPI
-RandGetSeed128 (
- OUT UINT8 *SeedBuffer
- )
-{
- BOOLEAN Ret;
- UINT8 RandByte[16];
- UINT8 Key[16];
- UINT8 Ffv[16];
- UINT8 Xored[16];
- UINT32 Index;
- UINT32 Index2;
- AES_KEY AESKey;
-
- //
- // Chose an arbitrary key and zero the feed_forward_value (FFV)
- //
- for (Index = 0; Index < 16; Index++) {
- Key[Index] = (UINT8) Index;
- Ffv[Index] = 0;
- }
-
- AES_set_encrypt_key (Key, 16 * 8, &AESKey);
-
- //
- // Perform CBC_MAC over 32 * 128 bit values, with 10us gaps between 128 bit value
- // The 10us gaps will ensure multiple reseeds within the system time with a large
- // design margin.
- //
- for (Index = 0; Index < 32; Index++) {
- MicroSecondDelay (10);
- Ret = RandGetBytes (16, RandByte);
- if (!Ret) {
- return Ret;
- }
-
- //
- // Perform XOR operations on two 128-bit value.
- //
- for (Index2 = 0; Index2 < 16; Index2++) {
- Xored[Index2] = RandByte[Index2] ^ Ffv[Index2];
- }
-
- AES_encrypt (Xored, Ffv, &AESKey);
- }
-
- for (Index = 0; Index < 16; Index++) {
- SeedBuffer[Index] = Ffv[Index];
- }
-
- return Ret;
-}
-
-/**
- Generate high-quality entropy source.
-
- @param[in] Length Size of the buffer, in bytes, to fill with.
- @param[out] Entropy Pointer to the buffer to store the entropy data.
-
- @retval EFI_SUCCESS Entropy generation succeeded.
- @retval EFI_NOT_READY Failed to request random data.
-
-**/
-STATIC
-BOOLEAN
-EFIAPI
-RandGenerateEntropy (
- IN UINTN Length,
- OUT UINT8 *Entropy
- )
-{
- BOOLEAN Ret;
- UINTN BlockCount;
- UINT8 Seed[16];
- UINT8 *Ptr;
-
- BlockCount = Length / 16;
- Ptr = (UINT8 *) Entropy;
-
- //
- // Generate high-quality seed for DRBG Entropy
- //
- while (BlockCount > 0) {
- Ret = RandGetSeed128 (Seed);
- if (!Ret) {
- return Ret;
- }
- CopyMem (Ptr, Seed, 16);
-
- BlockCount--;
- Ptr = Ptr + 16;
- }
-
- //
- // Populate the remained data as request.
- //
- Ret = RandGetSeed128 (Seed);
- if (!Ret) {
- return Ret;
- }
- CopyMem (Ptr, Seed, (Length % 16));
-
- return Ret;
-}
-
/*
* Add random bytes to the pool to acquire requested amount of entropy
*
@@ -238,7 +84,7 @@ size_t rand_pool_acquire_entropy(RAND_POOL *pool)
buffer = rand_pool_add_begin(pool, bytes_needed);
if (buffer != NULL) {
- Ret = RandGenerateEntropy(bytes_needed, buffer);
+ Ret = RandGetBytes(bytes_needed, buffer);
if (FALSE == Ret) {
rand_pool_add_end(pool, 0, 0);
} else {
@@ -257,13 +103,8 @@ size_t rand_pool_acquire_entropy(RAND_POOL *pool)
*/
int rand_pool_add_nonce_data(RAND_POOL *pool)
{
- struct {
- UINT64 Rand;
- UINT64 TimerValue;
- } data = { 0 };
-
- RandGetBytes(8, (UINT8 *)&(data.Rand));
- data.TimerValue = GetPerformanceCounter();
+ UINT8 data[16];
+ RandGetBytes(sizeof(data), data);
return rand_pool_add(pool, (unsigned char*)&data, sizeof(data), 0);
}
@@ -275,13 +116,8 @@ int rand_pool_add_nonce_data(RAND_POOL *pool)
*/
int rand_pool_add_additional_data(RAND_POOL *pool)
{
- struct {
- UINT64 Rand;
- UINT64 TimerValue;
- } data = { 0 };
-
- RandGetBytes(8, (UINT8 *)&(data.Rand));
- data.TimerValue = GetPerformanceCounter();
+ UINT8 data[16];
+ RandGetBytes(sizeof(data), data);
return rand_pool_add(pool, (unsigned char*)&data, sizeof(data), 0);
}
@@ -313,4 +149,3 @@ void rand_pool_cleanup(void)
void rand_pool_keep_random_devices_open(int keep)
{
}
-
diff --git a/CryptoPkg/Library/OpensslLib/rand_pool_noise.c b/CryptoPkg/Library/OpensslLib/rand_pool_noise.c
deleted file mode 100644
index 212834e27acc..000000000000
--- a/CryptoPkg/Library/OpensslLib/rand_pool_noise.c
+++ /dev/null
@@ -1,29 +0,0 @@
-/** @file
- Provide rand noise source.
-
-Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
-SPDX-License-Identifier: BSD-2-Clause-Patent
-
-**/
-
-#include <Library/BaseLib.h>
-
-/**
- Get 64-bit noise source
-
- @param[out] Rand Buffer pointer to store 64-bit noise source
-
- @retval FALSE Failed to generate
-**/
-BOOLEAN
-EFIAPI
-GetRandomNoise64 (
- OUT UINT64 *Rand
- )
-{
- //
- // Return FALSE will fallback to use PerformanceCounter to
- // generate noise.
- //
- return FALSE;
-}
diff --git a/CryptoPkg/Library/OpensslLib/rand_pool_noise_tsc.c b/CryptoPkg/Library/OpensslLib/rand_pool_noise_tsc.c
deleted file mode 100644
index 4158106231fd..000000000000
--- a/CryptoPkg/Library/OpensslLib/rand_pool_noise_tsc.c
+++ /dev/null
@@ -1,43 +0,0 @@
-/** @file
- Provide rand noise source.
-
-Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
-SPDX-License-Identifier: BSD-2-Clause-Patent
-
-**/
-
-#include <Library/BaseLib.h>
-#include <Library/DebugLib.h>
-#include <Library/TimerLib.h>
-
-/**
- Get 64-bit noise source
-
- @param[out] Rand Buffer pointer to store 64-bit noise source
-
- @retval TRUE Get randomness successfully.
- @retval FALSE Failed to generate
-**/
-BOOLEAN
-EFIAPI
-GetRandomNoise64 (
- OUT UINT64 *Rand
- )
-{
- UINT32 Index;
- UINT32 *RandPtr;
-
- if (NULL == Rand) {
- return FALSE;
- }
-
- RandPtr = (UINT32 *)Rand;
-
- for (Index = 0; Index < 2; Index ++) {
- *RandPtr = (UINT32) ((AsmReadTsc ()) & 0xFF);
- RandPtr++;
- MicroSecondDelay (10);
- }
-
- return TRUE;
-}
diff --git a/CryptoPkg/CryptoPkg.dsc b/CryptoPkg/CryptoPkg.dsc
index 1af78468a19c..0490eeb7e22f 100644
--- a/CryptoPkg/CryptoPkg.dsc
+++ b/CryptoPkg/CryptoPkg.dsc
@@ -60,6 +60,7 @@
BaseCryptLib|CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
TlsLib|CryptoPkg/Library/TlsLibNull/TlsLibNull.inf
HashApiLib|CryptoPkg/Library/BaseHashApiLib/BaseHashApiLib.inf
+ RngLib|MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf
[LibraryClasses.ARM, LibraryClasses.AARCH64]
#
diff --git a/CryptoPkg/Library/OpensslLib/OpensslLib.inf b/CryptoPkg/Library/OpensslLib/OpensslLib.inf
index dbbe5386a10c..4baad565564c 100644
--- a/CryptoPkg/Library/OpensslLib/OpensslLib.inf
+++ b/CryptoPkg/Library/OpensslLib/OpensslLib.inf
@@ -571,22 +571,9 @@
$(OPENSSL_PATH)/ssl/statem/statem_local.h
# Autogenerated files list ends here
buildinf.h
- rand_pool_noise.h
ossl_store.c
rand_pool.c
-[Sources.Ia32]
- rand_pool_noise_tsc.c
-
-[Sources.X64]
- rand_pool_noise_tsc.c
-
-[Sources.ARM]
- rand_pool_noise.c
-
-[Sources.AARCH64]
- rand_pool_noise.c
-
[Packages]
MdePkg/MdePkg.dec
CryptoPkg/CryptoPkg.dec
@@ -594,7 +581,7 @@
[LibraryClasses]
BaseLib
DebugLib
- TimerLib
+ RngLib
PrintLib
[LibraryClasses.ARM]
diff --git a/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf b/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
index 616ccd9f62d1..3557711bd85a 100644
--- a/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
+++ b/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
@@ -520,22 +520,9 @@
$(OPENSSL_PATH)/crypto/x509v3/v3_admis.h
# Autogenerated files list ends here
buildinf.h
- rand_pool_noise.h
ossl_store.c
rand_pool.c
-[Sources.Ia32]
- rand_pool_noise_tsc.c
-
-[Sources.X64]
- rand_pool_noise_tsc.c
-
-[Sources.ARM]
- rand_pool_noise.c
-
-[Sources.AARCH64]
- rand_pool_noise.c
-
[Packages]
MdePkg/MdePkg.dec
CryptoPkg/CryptoPkg.dec
@@ -543,7 +530,7 @@
[LibraryClasses]
BaseLib
DebugLib
- TimerLib
+ RngLib
PrintLib
[LibraryClasses.ARM]
diff --git a/CryptoPkg/Library/OpensslLib/rand_pool_noise.h b/CryptoPkg/Library/OpensslLib/rand_pool_noise.h
deleted file mode 100644
index 75acc686a9f1..000000000000
--- a/CryptoPkg/Library/OpensslLib/rand_pool_noise.h
+++ /dev/null
@@ -1,29 +0,0 @@
-/** @file
- Provide rand noise source.
-
-Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
-SPDX-License-Identifier: BSD-2-Clause-Patent
-
-**/
-
-#ifndef __RAND_POOL_NOISE_H__
-#define __RAND_POOL_NOISE_H__
-
-#include <Uefi/UefiBaseType.h>
-
-/**
- Get 64-bit noise source.
-
- @param[out] Rand Buffer pointer to store 64-bit noise source
-
- @retval TRUE Get randomness successfully.
- @retval FALSE Failed to generate
-**/
-BOOLEAN
-EFIAPI
-GetRandomNoise64 (
- OUT UINT64 *Rand
- );
-
-
-#endif // __RAND_POOL_NOISE_H__
--
2.27.0.windows.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v6 5/5] CryptoPkg: OpensslLib: Use RngLib to generate entropy in rand_pool
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
0 siblings, 0 replies; 16+ messages in thread
From: Ard Biesheuvel @ 2020-08-13 12:21 UTC (permalink / raw)
To: matthewfcarlson, devel; +Cc: Jiewen Yao, Jian J Wang, Xiaoyu Lu
On 8/13/20 12:43 AM, matthewfcarlson@gmail.com wrote:
> From: Matthew Carlson <macarl@microsoft.com>
>
> Ref: https://github.com/tianocore/edk2/pull/845
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1871
>
> Changes OpenSSL to no longer depend on TimerLib and instead use RngLib.
> This allows platforms to decide for themsevles what sort of entropy source
> they provide to OpenSSL and TlsLib.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> Signed-off-by: Matthew Carlson <matthewfcarlson@gmail.com>
With the coding style/whitespace violations fixed:
Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
> CryptoPkg/Library/OpensslLib/rand_pool.c | 203 ++------------------
> CryptoPkg/Library/OpensslLib/rand_pool_noise.c | 29 ---
> CryptoPkg/Library/OpensslLib/rand_pool_noise_tsc.c | 43 -----
> CryptoPkg/CryptoPkg.dsc | 1 +
> CryptoPkg/Library/OpensslLib/OpensslLib.inf | 15 +-
> CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf | 15 +-
> CryptoPkg/Library/OpensslLib/rand_pool_noise.h | 29 ---
> 7 files changed, 22 insertions(+), 313 deletions(-)
>
> diff --git a/CryptoPkg/Library/OpensslLib/rand_pool.c b/CryptoPkg/Library/OpensslLib/rand_pool.c
> index 9e0179b03490..3da92699fef6 100644
> --- a/CryptoPkg/Library/OpensslLib/rand_pool.c
> +++ b/CryptoPkg/Library/OpensslLib/rand_pool.c
> @@ -11,53 +11,18 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #include <openssl/aes.h>
>
> #include <Uefi.h>
> -#include <Library/TimerLib.h>
> -
> -#include "rand_pool_noise.h"
> -
> -/**
> - Get some randomness from low-order bits of GetPerformanceCounter results.
> - And combine them to the 64-bit value
> -
> - @param[out] Rand Buffer pointer to store the 64-bit random value.
> -
> - @retval TRUE Random number generated successfully.
> - @retval FALSE Failed to generate.
> -**/
> -STATIC
> -BOOLEAN
> -EFIAPI
> -GetRandNoise64FromPerformanceCounter(
> - OUT UINT64 *Rand
> - )
> -{
> - UINT32 Index;
> - UINT32 *RandPtr;
> -
> - if (NULL == Rand) {
> - return FALSE;
> - }
> -
> - RandPtr = (UINT32 *) Rand;
> -
> - for (Index = 0; Index < 2; Index ++) {
> - *RandPtr = (UINT32) (GetPerformanceCounter () & 0xFF);
> - MicroSecondDelay (10);
> - RandPtr++;
> - }
> -
> - return TRUE;
> -}
> +#include <Library/RngLib.h>
>
> /**
> Calls RandomNumber64 to fill
> a buffer of arbitrary size with random bytes.
> + This is a shim layer to RngLib.
>
> @param[in] Length Size of the buffer, in bytes, to fill with.
> @param[out] RandBuffer Pointer to the buffer to store the random result.
>
> - @retval EFI_SUCCESS Random bytes generation succeeded.
> - @retval EFI_NOT_READY Failed to request random bytes.
> + @retval TRUE Random bytes generation succeeded.
> + @retval FALSE Failed to request random bytes.
>
> **/
> STATIC
> @@ -73,17 +38,17 @@ RandGetBytes (
>
> Ret = FALSE;
>
> + if (RandBuffer == NULL) {
> + DEBUG((DEBUG_ERROR, "[OPENSSL_RAND_POOL] NULL RandBuffer. No random numbers are generated and your system is not secure\n"));
> + ASSERT(FALSE); // Since we can't generate random numbers, we should assert. Otherwise we will just blow up later.
> + return Ret;
> + }
> +
> +
> while (Length > 0) {
> - //
> - // Get random noise from platform.
> - // If it failed, fallback to PerformanceCounter
> - // If you really care about security, you must override
> - // GetRandomNoise64FromPlatform.
> - //
> - Ret = GetRandomNoise64 (&TempRand);
> - if (Ret == FALSE) {
> - Ret = GetRandNoise64FromPerformanceCounter (&TempRand);
> - }
> + // Use RngLib to get random number
> + Ret = GetRandomNumber64(&TempRand);
> +
> if (!Ret) {
> return Ret;
> }
> @@ -100,125 +65,6 @@ RandGetBytes (
> return Ret;
> }
>
> -/**
> - Creates a 128bit random value that is fully forward and backward prediction resistant,
> - suitable for seeding a NIST SP800-90 Compliant.
> - This function takes multiple random numbers from PerformanceCounter to ensure reseeding
> - and performs AES-CBC-MAC over the data to compute the seed value.
> -
> - @param[out] SeedBuffer Pointer to a 128bit buffer to store the random seed.
> -
> - @retval TRUE Random seed generation succeeded.
> - @retval FALSE Failed to request random bytes.
> -
> -**/
> -STATIC
> -BOOLEAN
> -EFIAPI
> -RandGetSeed128 (
> - OUT UINT8 *SeedBuffer
> - )
> -{
> - BOOLEAN Ret;
> - UINT8 RandByte[16];
> - UINT8 Key[16];
> - UINT8 Ffv[16];
> - UINT8 Xored[16];
> - UINT32 Index;
> - UINT32 Index2;
> - AES_KEY AESKey;
> -
> - //
> - // Chose an arbitrary key and zero the feed_forward_value (FFV)
> - //
> - for (Index = 0; Index < 16; Index++) {
> - Key[Index] = (UINT8) Index;
> - Ffv[Index] = 0;
> - }
> -
> - AES_set_encrypt_key (Key, 16 * 8, &AESKey);
> -
> - //
> - // Perform CBC_MAC over 32 * 128 bit values, with 10us gaps between 128 bit value
> - // The 10us gaps will ensure multiple reseeds within the system time with a large
> - // design margin.
> - //
> - for (Index = 0; Index < 32; Index++) {
> - MicroSecondDelay (10);
> - Ret = RandGetBytes (16, RandByte);
> - if (!Ret) {
> - return Ret;
> - }
> -
> - //
> - // Perform XOR operations on two 128-bit value.
> - //
> - for (Index2 = 0; Index2 < 16; Index2++) {
> - Xored[Index2] = RandByte[Index2] ^ Ffv[Index2];
> - }
> -
> - AES_encrypt (Xored, Ffv, &AESKey);
> - }
> -
> - for (Index = 0; Index < 16; Index++) {
> - SeedBuffer[Index] = Ffv[Index];
> - }
> -
> - return Ret;
> -}
> -
> -/**
> - Generate high-quality entropy source.
> -
> - @param[in] Length Size of the buffer, in bytes, to fill with.
> - @param[out] Entropy Pointer to the buffer to store the entropy data.
> -
> - @retval EFI_SUCCESS Entropy generation succeeded.
> - @retval EFI_NOT_READY Failed to request random data.
> -
> -**/
> -STATIC
> -BOOLEAN
> -EFIAPI
> -RandGenerateEntropy (
> - IN UINTN Length,
> - OUT UINT8 *Entropy
> - )
> -{
> - BOOLEAN Ret;
> - UINTN BlockCount;
> - UINT8 Seed[16];
> - UINT8 *Ptr;
> -
> - BlockCount = Length / 16;
> - Ptr = (UINT8 *) Entropy;
> -
> - //
> - // Generate high-quality seed for DRBG Entropy
> - //
> - while (BlockCount > 0) {
> - Ret = RandGetSeed128 (Seed);
> - if (!Ret) {
> - return Ret;
> - }
> - CopyMem (Ptr, Seed, 16);
> -
> - BlockCount--;
> - Ptr = Ptr + 16;
> - }
> -
> - //
> - // Populate the remained data as request.
> - //
> - Ret = RandGetSeed128 (Seed);
> - if (!Ret) {
> - return Ret;
> - }
> - CopyMem (Ptr, Seed, (Length % 16));
> -
> - return Ret;
> -}
> -
> /*
> * Add random bytes to the pool to acquire requested amount of entropy
> *
> @@ -238,7 +84,7 @@ size_t rand_pool_acquire_entropy(RAND_POOL *pool)
> buffer = rand_pool_add_begin(pool, bytes_needed);
>
> if (buffer != NULL) {
> - Ret = RandGenerateEntropy(bytes_needed, buffer);
> + Ret = RandGetBytes(bytes_needed, buffer);
> if (FALSE == Ret) {
> rand_pool_add_end(pool, 0, 0);
> } else {
> @@ -257,13 +103,8 @@ size_t rand_pool_acquire_entropy(RAND_POOL *pool)
> */
> int rand_pool_add_nonce_data(RAND_POOL *pool)
> {
> - struct {
> - UINT64 Rand;
> - UINT64 TimerValue;
> - } data = { 0 };
> -
> - RandGetBytes(8, (UINT8 *)&(data.Rand));
> - data.TimerValue = GetPerformanceCounter();
> + UINT8 data[16];
> + RandGetBytes(sizeof(data), data);
>
> return rand_pool_add(pool, (unsigned char*)&data, sizeof(data), 0);
> }
> @@ -275,13 +116,8 @@ int rand_pool_add_nonce_data(RAND_POOL *pool)
> */
> int rand_pool_add_additional_data(RAND_POOL *pool)
> {
> - struct {
> - UINT64 Rand;
> - UINT64 TimerValue;
> - } data = { 0 };
> -
> - RandGetBytes(8, (UINT8 *)&(data.Rand));
> - data.TimerValue = GetPerformanceCounter();
> + UINT8 data[16];
> + RandGetBytes(sizeof(data), data);
>
> return rand_pool_add(pool, (unsigned char*)&data, sizeof(data), 0);
> }
> @@ -313,4 +149,3 @@ void rand_pool_cleanup(void)
> void rand_pool_keep_random_devices_open(int keep)
> {
> }
> -
> diff --git a/CryptoPkg/Library/OpensslLib/rand_pool_noise.c b/CryptoPkg/Library/OpensslLib/rand_pool_noise.c
> deleted file mode 100644
> index 212834e27acc..000000000000
> --- a/CryptoPkg/Library/OpensslLib/rand_pool_noise.c
> +++ /dev/null
> @@ -1,29 +0,0 @@
> -/** @file
> - Provide rand noise source.
> -
> -Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> -SPDX-License-Identifier: BSD-2-Clause-Patent
> -
> -**/
> -
> -#include <Library/BaseLib.h>
> -
> -/**
> - Get 64-bit noise source
> -
> - @param[out] Rand Buffer pointer to store 64-bit noise source
> -
> - @retval FALSE Failed to generate
> -**/
> -BOOLEAN
> -EFIAPI
> -GetRandomNoise64 (
> - OUT UINT64 *Rand
> - )
> -{
> - //
> - // Return FALSE will fallback to use PerformanceCounter to
> - // generate noise.
> - //
> - return FALSE;
> -}
> diff --git a/CryptoPkg/Library/OpensslLib/rand_pool_noise_tsc.c b/CryptoPkg/Library/OpensslLib/rand_pool_noise_tsc.c
> deleted file mode 100644
> index 4158106231fd..000000000000
> --- a/CryptoPkg/Library/OpensslLib/rand_pool_noise_tsc.c
> +++ /dev/null
> @@ -1,43 +0,0 @@
> -/** @file
> - Provide rand noise source.
> -
> -Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> -SPDX-License-Identifier: BSD-2-Clause-Patent
> -
> -**/
> -
> -#include <Library/BaseLib.h>
> -#include <Library/DebugLib.h>
> -#include <Library/TimerLib.h>
> -
> -/**
> - Get 64-bit noise source
> -
> - @param[out] Rand Buffer pointer to store 64-bit noise source
> -
> - @retval TRUE Get randomness successfully.
> - @retval FALSE Failed to generate
> -**/
> -BOOLEAN
> -EFIAPI
> -GetRandomNoise64 (
> - OUT UINT64 *Rand
> - )
> -{
> - UINT32 Index;
> - UINT32 *RandPtr;
> -
> - if (NULL == Rand) {
> - return FALSE;
> - }
> -
> - RandPtr = (UINT32 *)Rand;
> -
> - for (Index = 0; Index < 2; Index ++) {
> - *RandPtr = (UINT32) ((AsmReadTsc ()) & 0xFF);
> - RandPtr++;
> - MicroSecondDelay (10);
> - }
> -
> - return TRUE;
> -}
> diff --git a/CryptoPkg/CryptoPkg.dsc b/CryptoPkg/CryptoPkg.dsc
> index 1af78468a19c..0490eeb7e22f 100644
> --- a/CryptoPkg/CryptoPkg.dsc
> +++ b/CryptoPkg/CryptoPkg.dsc
> @@ -60,6 +60,7 @@
> BaseCryptLib|CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf
> TlsLib|CryptoPkg/Library/TlsLibNull/TlsLibNull.inf
> HashApiLib|CryptoPkg/Library/BaseHashApiLib/BaseHashApiLib.inf
> + RngLib|MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf
>
> [LibraryClasses.ARM, LibraryClasses.AARCH64]
> #
> diff --git a/CryptoPkg/Library/OpensslLib/OpensslLib.inf b/CryptoPkg/Library/OpensslLib/OpensslLib.inf
> index dbbe5386a10c..4baad565564c 100644
> --- a/CryptoPkg/Library/OpensslLib/OpensslLib.inf
> +++ b/CryptoPkg/Library/OpensslLib/OpensslLib.inf
> @@ -571,22 +571,9 @@
> $(OPENSSL_PATH)/ssl/statem/statem_local.h
> # Autogenerated files list ends here
> buildinf.h
> - rand_pool_noise.h
> ossl_store.c
> rand_pool.c
>
> -[Sources.Ia32]
> - rand_pool_noise_tsc.c
> -
> -[Sources.X64]
> - rand_pool_noise_tsc.c
> -
> -[Sources.ARM]
> - rand_pool_noise.c
> -
> -[Sources.AARCH64]
> - rand_pool_noise.c
> -
> [Packages]
> MdePkg/MdePkg.dec
> CryptoPkg/CryptoPkg.dec
> @@ -594,7 +581,7 @@
> [LibraryClasses]
> BaseLib
> DebugLib
> - TimerLib
> + RngLib
> PrintLib
>
> [LibraryClasses.ARM]
> diff --git a/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf b/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
> index 616ccd9f62d1..3557711bd85a 100644
> --- a/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
> +++ b/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
> @@ -520,22 +520,9 @@
> $(OPENSSL_PATH)/crypto/x509v3/v3_admis.h
> # Autogenerated files list ends here
> buildinf.h
> - rand_pool_noise.h
> ossl_store.c
> rand_pool.c
>
> -[Sources.Ia32]
> - rand_pool_noise_tsc.c
> -
> -[Sources.X64]
> - rand_pool_noise_tsc.c
> -
> -[Sources.ARM]
> - rand_pool_noise.c
> -
> -[Sources.AARCH64]
> - rand_pool_noise.c
> -
> [Packages]
> MdePkg/MdePkg.dec
> CryptoPkg/CryptoPkg.dec
> @@ -543,7 +530,7 @@
> [LibraryClasses]
> BaseLib
> DebugLib
> - TimerLib
> + RngLib
> PrintLib
>
> [LibraryClasses.ARM]
> diff --git a/CryptoPkg/Library/OpensslLib/rand_pool_noise.h b/CryptoPkg/Library/OpensslLib/rand_pool_noise.h
> deleted file mode 100644
> index 75acc686a9f1..000000000000
> --- a/CryptoPkg/Library/OpensslLib/rand_pool_noise.h
> +++ /dev/null
> @@ -1,29 +0,0 @@
> -/** @file
> - Provide rand noise source.
> -
> -Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> -SPDX-License-Identifier: BSD-2-Clause-Patent
> -
> -**/
> -
> -#ifndef __RAND_POOL_NOISE_H__
> -#define __RAND_POOL_NOISE_H__
> -
> -#include <Uefi/UefiBaseType.h>
> -
> -/**
> - Get 64-bit noise source.
> -
> - @param[out] Rand Buffer pointer to store 64-bit noise source
> -
> - @retval TRUE Get randomness successfully.
> - @retval FALSE Failed to generate
> -**/
> -BOOLEAN
> -EFIAPI
> -GetRandomNoise64 (
> - OUT UINT64 *Rand
> - );
> -
> -
> -#endif // __RAND_POOL_NOISE_H__
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v6 0/5] Use RngLib instead of TimerLib for OpensslLib
2020-08-12 22:43 [PATCH v6 0/5] Use RngLib instead of TimerLib for OpensslLib Matthew Carlson
` (4 preceding siblings ...)
2020-08-12 22:43 ` [PATCH v6 5/5] CryptoPkg: OpensslLib: Use RngLib to generate entropy in rand_pool Matthew Carlson
@ 2020-08-13 15:14 ` Yao, Jiewen
2020-08-13 19:42 ` Matthew Carlson
5 siblings, 1 reply; 16+ messages in thread
From: Yao, Jiewen @ 2020-08-13 15:14 UTC (permalink / raw)
To: matthewfcarlson@gmail.com, devel@edk2.groups.io
Cc: Ard Biesheuvel, Anthony Perard, Wang, Jian J, Julien Grall,
Justen, Jordan L, Laszlo Ersek, Gao, Liming, Leif Lindholm,
Kinney, Michael D, Lu, XiaoyuX, Liu, Zhiguang, Sean Brogan
Thanks Matthew.
I am OK, if you want to address the RDSEED in follow-up patch series.
Would you please file a new Bugzilla to record this, so we won't lose the information ?
> -----Original Message-----
> From: matthewfcarlson@gmail.com <matthewfcarlson@gmail.com>
> Sent: Thursday, August 13, 2020 6:44 AM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>; Anthony Perard
> <anthony.perard@citrix.com>; Yao, Jiewen <jiewen.yao@intel.com>; Wang,
> Jian J <jian.j.wang@intel.com>; Julien Grall <julien@xen.org>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Gao, Liming
> <liming.gao@intel.com>; Leif Lindholm <leif@nuviainc.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>; Liu,
> Zhiguang <zhiguang.liu@intel.com>; Sean Brogan
> <sean.brogan@microsoft.com>; Matthew Carlson
> <matthewfcarlson@gmail.com>
> Subject: [PATCH v6 0/5] Use RngLib instead of TimerLib for OpensslLib
>
> From: Matthew Carlson <macarl@microsoft.com>
>
> Hello all,
>
> This patch contains a fix for Bugzilla 1871.
> There's been a good bit of community discussion around the topic,
> so below follows a general overview of the discussion and what this patch does.
>
> Back in Devel message#40590 (https://edk2.groups.io/g/devel/message/40590)
> around the patch series that updates OpenSSL to 1.1.1b, a comment was made
> that suggested that platforms be in charge of the entropy/randomness that
> is provided to OpenSSL as currently the entropry source seems to be a
> hand-rolled random number generator that uses the PerformanceCounter from
> TimerLib. This causes OpenSSL to depend on TimerLib, which is often platform
> specific. In addition to being a potentially weaker source of randomness,
> this also poses a challenge to compile BaseCryptLibOnProtocol with a platform-
> agnostic version of TimerLib that works universally.
>
> The solution here is to allow platform to specify their source of entropy in
> addition to providing two new RngLibs: one that uses the TimerLib as well as
> one that uses RngProtocol to provide randomness. Then the decision to use
> RDRAND or other entropy sources is up to the platform. Mixing various entropy
> sources is the onus of the platform. It has been suggested on Devel#40590 and
> BZ#1871 that there should be mixing of the PerformanceCounter and RDRAND
> using
> something similar to the yarrow alogirthm that FreeBSD uses for example. This
> patch series doesn't offer an RngLib that offers that sort of mixing as the
> ultimate source of random is defined by the platform.
>
> This patch series offers three benefits:
> 1. Dependency reduction: Removes the need for a platform specific timer
> library. We publish a single binary used on numerous platforms for
> crypto and the introduced timer lib dependency caused issues because we
> could not fulfill our platform needs with one library instance.
>
> 2. Code maintenance: Removing this additional code and leveraging an existing
> library within Edk2 means less code to maintain.
>
> 3. Platform defined quality: A platform can choose which instance to use and
> the implications of that instance.
>
> This patch series seeks to address five seperate issues.
> 1) Use RngLib interface to generate random entropy in rand_pool
> 2) Remove dependency on TimerLib in OpensslLib
> 3) Add a new version of RngLib implemented by TimerLib
> 4) Add a new version of RngLib implemented by EFI_RNG_PROTOCOL
> 5) Add RngLib to platforms in EDK2 such as ArmVirtPkg and OvmfPkg
>
> Since this changes the dependencies of OpenSSL, this has the potential of being
> a breaking change for platforms in edk2-platforms. The easiest solution is just
> to use the RngLib that uses the TimerLib as this closely mimics the behavior of
> OpenSSL prior to this patch series. There is also a null version of RngLib for
> CI environments that need this change
> (https://edk2.groups.io/g/devel/message/50432). Though it should be pointed
> out
> that in CI environments, the null version of BaseCryptLib or OpenSSL should be
> used.
>
> In addition, it has been suggested that
> 1) Add AsmRdSeed to BaseLib.
> 2) Update BaseRngLib to use AsmRdSeed() for the random number,
> if RdSeed is supported (CPUID BIT18)
>
> However, this is largely out of scope for this particular patch series and
> will likely need to be in a follow-up series later.
>
> It is my understanding that the OpenSSL code uses the values provided as a
> randomness pool rather than a seed or random numbers itself, so the
> requirements for randomness are not quite as stringent as other applications.
>
> For the ArmVirtPkg and OvmfPkg platforms, the patch series here just adds in
> the TimerLib based RngLib as that is similar to the functionality of before.
> It is added as a common library so any custom RngLib defined in the DSC
> should take precedence over the TimerLibRngLib.
>
> 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: Anthony Perard <anthony.perard@citrix.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Julien Grall <julien@xen.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
>
> Signed-off-by: Matthew Carlson <matthewfcarlson@gmail.com>
>
>
> Matthew Carlson (5):
> MdePkg: TimerRngLib: Added RngLib that uses TimerLib
> MdePkg: BaseRngLibDxe: Add RngLib that uses RngDxe
> OvmfPkg: Add RngLib based on TimerLib for Crypto
> ArmVirtPkg: Add RngLib based on TimerLib for CryptoPkg
> CryptoPkg: OpensslLib: Use RngLib to generate entropy in rand_pool
>
> CryptoPkg/Library/OpensslLib/rand_pool.c | 203 ++------------------
> CryptoPkg/Library/OpensslLib/rand_pool_noise.c | 29 ---
> CryptoPkg/Library/OpensslLib/rand_pool_noise_tsc.c | 43 -----
> MdePkg/Library/BaseRngLibDxe/RngDxeLib.c | 200
> +++++++++++++++++++
> MdePkg/Library/BaseRngLibTimerLib/RngLibTimer.c | 187
> ++++++++++++++++++
> ArmVirtPkg/ArmVirt.dsc.inc | 1 +
> CryptoPkg/CryptoPkg.dsc | 1 +
> CryptoPkg/Library/OpensslLib/OpensslLib.inf | 15 +-
> CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf | 15 +-
> CryptoPkg/Library/OpensslLib/rand_pool_noise.h | 29 ---
> MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf | 38 ++++
> MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf | 40 ++++
> MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.uni | 17 ++
> MdePkg/MdePkg.dsc | 5 +-
> OvmfPkg/Bhyve/BhyvePkgX64.dsc | 1 +
> OvmfPkg/OvmfPkgIa32.dsc | 1 +
> OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
> OvmfPkg/OvmfPkgX64.dsc | 1 +
> OvmfPkg/OvmfXen.dsc | 1 +
> 19 files changed, 514 insertions(+), 314 deletions(-)
> delete mode 100644 CryptoPkg/Library/OpensslLib/rand_pool_noise.c
> delete mode 100644 CryptoPkg/Library/OpensslLib/rand_pool_noise_tsc.c
> create mode 100644 MdePkg/Library/BaseRngLibDxe/RngDxeLib.c
> create mode 100644 MdePkg/Library/BaseRngLibTimerLib/RngLibTimer.c
> delete mode 100644 CryptoPkg/Library/OpensslLib/rand_pool_noise.h
> create mode 100644 MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf
> create mode 100644
> MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
> create mode 100644
> MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.uni
>
> --
> 2.27.0.windows.1
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v6 0/5] Use RngLib instead of TimerLib for OpensslLib
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
0 siblings, 0 replies; 16+ messages in thread
From: Matthew Carlson @ 2020-08-13 19:42 UTC (permalink / raw)
To: Yao, Jiewen
Cc: devel@edk2.groups.io, Ard Biesheuvel, Anthony Perard,
Wang, Jian J, Julien Grall, Justen, Jordan L, Laszlo Ersek,
Gao, Liming, Leif Lindholm, Kinney, Michael D, Lu, XiaoyuX,
Liu, Zhiguang, Sean Brogan
[-- Attachment #1: Type: text/plain, Size: 8571 bytes --]
I'll file a new bugzilla.
https://bugzilla.tianocore.org/show_bug.cgi?id=2897
-Matthew Carlson
On Thu, Aug 13, 2020 at 8:15 AM Yao, Jiewen <jiewen.yao@intel.com> wrote:
> Thanks Matthew.
>
> I am OK, if you want to address the RDSEED in follow-up patch series.
>
> Would you please file a new Bugzilla to record this, so we won't lose the
> information ?
>
>
>
> > -----Original Message-----
> > From: matthewfcarlson@gmail.com <matthewfcarlson@gmail.com>
> > Sent: Thursday, August 13, 2020 6:44 AM
> > To: devel@edk2.groups.io
> > Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>; Anthony Perard
> > <anthony.perard@citrix.com>; Yao, Jiewen <jiewen.yao@intel.com>; Wang,
> > Jian J <jian.j.wang@intel.com>; Julien Grall <julien@xen.org>; Justen,
> Jordan L
> > <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Gao,
> Liming
> > <liming.gao@intel.com>; Leif Lindholm <leif@nuviainc.com>; Kinney,
> Michael D
> > <michael.d.kinney@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>; Liu,
> > Zhiguang <zhiguang.liu@intel.com>; Sean Brogan
> > <sean.brogan@microsoft.com>; Matthew Carlson
> > <matthewfcarlson@gmail.com>
> > Subject: [PATCH v6 0/5] Use RngLib instead of TimerLib for OpensslLib
> >
> > From: Matthew Carlson <macarl@microsoft.com>
> >
> > Hello all,
> >
> > This patch contains a fix for Bugzilla 1871.
> > There's been a good bit of community discussion around the topic,
> > so below follows a general overview of the discussion and what this
> patch does.
> >
> > Back in Devel message#40590 (
> https://edk2.groups.io/g/devel/message/40590)
> > around the patch series that updates OpenSSL to 1.1.1b, a comment was
> made
> > that suggested that platforms be in charge of the entropy/randomness that
> > is provided to OpenSSL as currently the entropry source seems to be a
> > hand-rolled random number generator that uses the PerformanceCounter from
> > TimerLib. This causes OpenSSL to depend on TimerLib, which is often
> platform
> > specific. In addition to being a potentially weaker source of randomness,
> > this also poses a challenge to compile BaseCryptLibOnProtocol with a
> platform-
> > agnostic version of TimerLib that works universally.
> >
> > The solution here is to allow platform to specify their source of
> entropy in
> > addition to providing two new RngLibs: one that uses the TimerLib as
> well as
> > one that uses RngProtocol to provide randomness. Then the decision to use
> > RDRAND or other entropy sources is up to the platform. Mixing various
> entropy
> > sources is the onus of the platform. It has been suggested on
> Devel#40590 and
> > BZ#1871 that there should be mixing of the PerformanceCounter and RDRAND
> > using
> > something similar to the yarrow alogirthm that FreeBSD uses for example.
> This
> > patch series doesn't offer an RngLib that offers that sort of mixing as
> the
> > ultimate source of random is defined by the platform.
> >
> > This patch series offers three benefits:
> > 1. Dependency reduction: Removes the need for a platform specific timer
> > library. We publish a single binary used on numerous platforms for
> > crypto and the introduced timer lib dependency caused issues because we
> > could not fulfill our platform needs with one library instance.
> >
> > 2. Code maintenance: Removing this additional code and leveraging an
> existing
> > library within Edk2 means less code to maintain.
> >
> > 3. Platform defined quality: A platform can choose which instance to use
> and
> > the implications of that instance.
> >
> > This patch series seeks to address five seperate issues.
> > 1) Use RngLib interface to generate random entropy in rand_pool
> > 2) Remove dependency on TimerLib in OpensslLib
> > 3) Add a new version of RngLib implemented by TimerLib
> > 4) Add a new version of RngLib implemented by EFI_RNG_PROTOCOL
> > 5) Add RngLib to platforms in EDK2 such as ArmVirtPkg and OvmfPkg
> >
> > Since this changes the dependencies of OpenSSL, this has the potential
> of being
> > a breaking change for platforms in edk2-platforms. The easiest solution
> is just
> > to use the RngLib that uses the TimerLib as this closely mimics the
> behavior of
> > OpenSSL prior to this patch series. There is also a null version of
> RngLib for
> > CI environments that need this change
> > (https://edk2.groups.io/g/devel/message/50432). Though it should be
> pointed
> > out
> > that in CI environments, the null version of BaseCryptLib or OpenSSL
> should be
> > used.
> >
> > In addition, it has been suggested that
> > 1) Add AsmRdSeed to BaseLib.
> > 2) Update BaseRngLib to use AsmRdSeed() for the random number,
> > if RdSeed is supported (CPUID BIT18)
> >
> > However, this is largely out of scope for this particular patch series
> and
> > will likely need to be in a follow-up series later.
> >
> > It is my understanding that the OpenSSL code uses the values provided as
> a
> > randomness pool rather than a seed or random numbers itself, so the
> > requirements for randomness are not quite as stringent as other
> applications.
> >
> > For the ArmVirtPkg and OvmfPkg platforms, the patch series here just
> adds in
> > the TimerLib based RngLib as that is similar to the functionality of
> before.
> > It is added as a common library so any custom RngLib defined in the DSC
> > should take precedence over the TimerLibRngLib.
> >
> > 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: Anthony Perard <anthony.perard@citrix.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Julien Grall <julien@xen.org>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Leif Lindholm <leif@nuviainc.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> > Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> > Cc: Sean Brogan <sean.brogan@microsoft.com>
> >
> > Signed-off-by: Matthew Carlson <matthewfcarlson@gmail.com>
> >
> >
> > Matthew Carlson (5):
> > MdePkg: TimerRngLib: Added RngLib that uses TimerLib
> > MdePkg: BaseRngLibDxe: Add RngLib that uses RngDxe
> > OvmfPkg: Add RngLib based on TimerLib for Crypto
> > ArmVirtPkg: Add RngLib based on TimerLib for CryptoPkg
> > CryptoPkg: OpensslLib: Use RngLib to generate entropy in rand_pool
> >
> > CryptoPkg/Library/OpensslLib/rand_pool.c | 203
> ++------------------
> > CryptoPkg/Library/OpensslLib/rand_pool_noise.c | 29 ---
> > CryptoPkg/Library/OpensslLib/rand_pool_noise_tsc.c | 43 -----
> > MdePkg/Library/BaseRngLibDxe/RngDxeLib.c | 200
> > +++++++++++++++++++
> > MdePkg/Library/BaseRngLibTimerLib/RngLibTimer.c | 187
> > ++++++++++++++++++
> > ArmVirtPkg/ArmVirt.dsc.inc | 1 +
> > CryptoPkg/CryptoPkg.dsc | 1 +
> > CryptoPkg/Library/OpensslLib/OpensslLib.inf | 15 +-
> > CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf | 15 +-
> > CryptoPkg/Library/OpensslLib/rand_pool_noise.h | 29 ---
> > MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf | 38 ++++
> > MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf | 40 ++++
> > MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.uni | 17 ++
> > MdePkg/MdePkg.dsc | 5 +-
> > OvmfPkg/Bhyve/BhyvePkgX64.dsc | 1 +
> > OvmfPkg/OvmfPkgIa32.dsc | 1 +
> > OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
> > OvmfPkg/OvmfPkgX64.dsc | 1 +
> > OvmfPkg/OvmfXen.dsc | 1 +
> > 19 files changed, 514 insertions(+), 314 deletions(-)
> > delete mode 100644 CryptoPkg/Library/OpensslLib/rand_pool_noise.c
> > delete mode 100644 CryptoPkg/Library/OpensslLib/rand_pool_noise_tsc.c
> > create mode 100644 MdePkg/Library/BaseRngLibDxe/RngDxeLib.c
> > create mode 100644 MdePkg/Library/BaseRngLibTimerLib/RngLibTimer.c
> > delete mode 100644 CryptoPkg/Library/OpensslLib/rand_pool_noise.h
> > create mode 100644 MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf
> > create mode 100644
> > MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
> > create mode 100644
> > MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.uni
> >
> > --
> > 2.27.0.windows.1
>
>
[-- Attachment #2: Type: text/html, Size: 12701 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread