[edk2-devel] [PATCH v6 2/5] MdePkg: BaseRngLibDxe: Add RngLib that uses RngDxe
Matthew Carlson
matthewfcarlson at gmail.com
Thu Aug 13 19:18:07 UTC 2020
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 at arm.com>
wrote:
> On 8/13/20 12:43 AM, matthewfcarlson at gmail.com wrote:
> > From: Matthew Carlson <macarl at 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 at arm.com>
> > Cc: Michael D Kinney <michael.d.kinney at intel.com>
> > Cc: Liming Gao <liming.gao at intel.com>
> > Cc: Zhiguang Liu <zhiguang.liu at intel.com>
> > Signed-off-by: Matthew Carlson <matthewfcarlson at gmail.com>
> > ---
> > MdePkg/Library/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
> >
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#64246): https://edk2.groups.io/g/devel/message/64246
Mute This Topic: https://groups.io/mt/76157314/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20200813/6c4b7f51/attachment.htm>
More information about the edk2-devel-archive
mailing list