From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-io1-f67.google.com (mail-io1-f67.google.com [209.85.166.67]) by mx.groups.io with SMTP id smtpd.web11.2649.1597346298165835365 for ; Thu, 13 Aug 2020 12:18:18 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20161025 header.b=INtdA0RC; spf=pass (domain: gmail.com, ip: 209.85.166.67, mailfrom: matthewfcarlson@gmail.com) Received: by mail-io1-f67.google.com with SMTP id b16so8510191ioj.4 for ; Thu, 13 Aug 2020 12:18:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=MtIVtO6hkJNPSum6zVMfBXu/9mA31Og+cv1IhsP+sZ8=; b=INtdA0RCuylShIE1Y2WppeR1SXR/IJbPfJ75ITWTRgBuZnEqrIz76SbJZDBjq0fsQx YgvX2duBcw+7lONxtzymUtaVphlJc1ZPOUxKuiZgCO9QQ7Cf2TdGlTkzS7MTWjYbWzpE D/NObbjVBvRRBzJhzwuZV7iBpgad7SR6eTLSBRuQR9DFX6UlF/rKhUY5wcYLmvLD5mWL tNI2IGcO1AW8p1iWmAvUMd/EyeiSFWNx9xn2wu0egXau7pdaMq5bNFN7B6fkZq/pv/xl LWMgaDIlJCOUNxsOEywK/UGlEr6FRbMAvBTb4f1WSdxKVnhqLw9GPG9FCbUEeKzVXl2N Fhrg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=MtIVtO6hkJNPSum6zVMfBXu/9mA31Og+cv1IhsP+sZ8=; b=PUcTlg/oIoz0OtCeP3pBS8RuwhE2Tor1/ooJUVKwf0fS/uEZXuHDWIc+029aTcO6T/ y0sRttVtGJwC3ubvtoLBvtP+s2EKLl9M8kkTmUaEHzQL9+n9Zoy4NPQ3cQzPgI7NdglG RqXBKbB6cNwyNKIl3by3G4AsxCeWwGEDP88C/IDug4ptTvVt56HTFz7Mzro2YeZNMKhW YMIRHKkafe4JO8D3ryZB+w4hiR3gHAIEswurgCyOXC5onIHouxpT2bECpmtcpqC1dP3u YpGBt/B00nJp4vhiubthM7kyGS+clGViDIRybIo2qaQUgKUTDLC+aYpKJX0EEEmvFuoi D/jw== X-Gm-Message-State: AOAM531u3G+wOP6hpZOGD0FAnpgZDzMx9D3ziPBpeA5CzULia6xr20s5 br7dCFpxDkCNhzehlrvO8+hX7vowr9DkpKzTC50= X-Google-Smtp-Source: ABdhPJyjzTvn4XYR8d34Km8/qoJVNBDPDpSDdcnesPh5tPBN0q6xQ3JQ0zuR7ByaKhf0xQDLNgEyfumG2DDjPvBlzIk= X-Received: by 2002:a6b:5c17:: with SMTP id z23mr6210945ioh.67.1597346297580; Thu, 13 Aug 2020 12:18:17 -0700 (PDT) MIME-Version: 1.0 References: <20200812224338.287-1-matthewfcarlson@gmail.com> <20200812224338.287-3-matthewfcarlson@gmail.com> <1b47936f-ed13-778e-79ed-707e0ca3bdbc@arm.com> In-Reply-To: <1b47936f-ed13-778e-79ed-707e0ca3bdbc@arm.com> From: "Matthew Carlson" Date: Thu, 13 Aug 2020 12:18:07 -0700 Message-ID: Subject: Re: [PATCH v6 2/5] MdePkg: BaseRngLibDxe: Add RngLib that uses RngDxe To: Ard Biesheuvel Cc: devel@edk2.groups.io, Michael D Kinney , Liming Gao , Zhiguang Liu Content-Type: multipart/alternative; boundary="000000000000a6918b05acc72aa6" --000000000000a6918b05acc72aa6 Content-Type: text/plain; charset="UTF-8" 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 wrote: > On 8/13/20 12:43 AM, matthewfcarlson@gmail.com wrote: > > From: Matthew Carlson > > > > 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 > > Cc: Michael D Kinney > > Cc: Liming Gao > > Cc: Zhiguang Liu > > Signed-off-by: Matthew Carlson > > --- > > 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 > > +#include > > +#include > > +#include > > +#include > > + > > +/** > > +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 > > > > --000000000000a6918b05acc72aa6 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Thanks for the feedback.

I've addre= ssed 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 ca= n make some promising regarding=C2=A0the validity of its random number gene= ration. That said, this is code that another coworker at Microsoft wrote, s= o I'm not 100% sure why it does that this particular way.=C2=A0

Do you have a suggestion about what sort of algorithm sho= uld be selected? Perhaps just using the default every time? Keep the patter= n 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=C2=A0regardless of where they're coming fro= m. I'm open to adding an assert there as well to help debugability.=C2= =A0

-Matthew Carlson


On Thu, Aug 13, 2020 at 5:19 AM Ard Biesheuvel &l= t;ard.biesheuvel@arm.com> = wrote:
On 8/13/2= 0 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_b= ug.cgi?id=3D1871
>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Signed-off-by: Matthew Carlson <matthewfcarlson@gmail.com>
> ---
>=C2=A0 =C2=A0MdePkg/Library/BaseRngLibDxe/RngDxeLib.c=C2=A0 =C2=A0 =C2= =A0 =C2=A0| 200 ++++++++++++++++++++
>=C2=A0 =C2=A0MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf |=C2=A0 38 = ++++
>=C2=A0 =C2=A0MdePkg/MdePkg.dsc=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2=A0 =C2= =A04 +-
>=C2=A0 =C2=A03 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:
> +
> +=C2=A0 =C2=A0 Generates a random number via the NIST
> +=C2=A0 =C2=A0 800-9A algorithm.=C2=A0 Refer to
> +=C2=A0 =C2=A0 http://csrc.nist.g= ov/groups/STM/cavp/documents/drbg/DRBGVS.pdf
> +=C2=A0 =C2=A0 for more information.
> +
> +=C2=A0 =C2=A0 Arguments:
> +
> +=C2=A0 =C2=A0 Buffer=C2=A0 =C2=A0 =C2=A0 -- Buffer to receive the ran= dom number.
> +=C2=A0 =C2=A0 BufferSize=C2=A0 -- Number of bytes in Buffer.
> +
> +Return Value:
> +
> +=C2=A0 =C2=A0 EFI_SUCCESS or underlying failure code.
> +
> +**/

STATIC ?

> +EFI_STATUS
> +EFIAPI
> +GenerateRandomNumberViaNist800Algorithm(

space before (

> +=C2=A0 OUT UINT8* Buffer,

put * on the rhs

> +=C2=A0 IN=C2=A0 UINTN=C2=A0 BufferSize
> +=C2=A0 )
> +{
> +=C2=A0 EFI_STATUS=C2=A0 =C2=A0 =C2=A0 =C2=A0 Status;
> +=C2=A0 EFI_RNG_PROTOCOL* RngProtocol;

likewise

> +
> +=C2=A0 RngProtocol =3D NULL;
> +
> +=C2=A0 if (Buffer =3D=3D NULL) {
> +=C2=A0 =C2=A0 =C2=A0 DEBUG((DEBUG_ERROR, "[%a] Buffer =3D=3D NUL= L.\n", __FUNCTION__));

Could you drop the [] around the function name? This is rather
unidiomatic for EDK2

> +=C2=A0 =C2=A0 =C2=A0 return EFI_INVALID_PARAMETER;
> +=C2=A0 }
> +
> +=C2=A0 Status =3D gBS->LocateProtocol(&gEfiRngProtocolGuid, NU= LL, (VOID **)&RngProtocol);

Space before (

> +=C2=A0 if (EFI_ERROR(Status) || RngProtocol =3D=3D NULL) {

Space before (. Also, I think the second condition could be an ASSERT()

> +=C2=A0 =C2=A0 =C2=A0 DEBUG((DEBUG_ERROR, "[%a] Could not locate = RNG prototocol, Status =3D %r\n", __FUNCTION__, Status));
> +=C2=A0 =C2=A0 =C2=A0 return Status;
> +=C2=A0 }
> +
> +=C2=A0 Status =3D RngProtocol->GetRNG(RngProtocol, &gEfiRngAlg= orithmSp80090Ctr256Guid, BufferSize, Buffer);
> +=C2=A0 DEBUG((DEBUG_INFO, "[%a] GetRNG algorithm CTR-256 - Statu= s =3D %r\n", __FUNCTION__, Status)); > +=C2=A0 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.


> +=C2=A0 =C2=A0 return Status;
> +=C2=A0 }
> +
> +=C2=A0 Status =3D RngProtocol->GetRNG(RngProtocol, &gEfiRngAlg= orithmSp80090Hmac256Guid, BufferSize, Buffer);
> +=C2=A0 DEBUG((DEBUG_INFO, "[%a] GetRNG algorithm HMAC-256 - Stat= us =3D %r\n", __FUNCTION__, Status));
> +=C2=A0 if(!EFI_ERROR(Status)) {
> +=C2=A0 =C2=A0 return Status;
> +=C2=A0 }
> +
> +=C2=A0 Status =3D RngProtocol->GetRNG(RngProtocol, &gEfiRngAlg= orithmSp80090Hash256Guid, BufferSize, Buffer);
> +=C2=A0 DEBUG((DEBUG_INFO, "[%a] GetRNG algorithm Hash-256 - Stat= us =3D %r\n", __FUNCTION__, Status));
> +=C2=A0 if(!EFI_ERROR(Status)) {
> +=C2=A0 =C2=A0 return Status;
> +=C2=A0 }

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,

> +=C2=A0 // If we get to this point, we have failed
> +=C2=A0 DEBUG((DEBUG_ERROR, "[%a] GetRNG() failed, staus =3D %r\n= ", __FUNCTION__, Status));
> +
> +=C2=A0 return Status;
> +}// GenerateRandomNumberViaNist800Algorithm()
> +
> +
> +/**
> +=C2=A0 Generates a 16-bit random number.
> +
> +=C2=A0 if Rand is NULL, return FALSE.
> +

Can't we simply stipulate that Rand !=3D NULL? Is there ever a point to=
calling this function with a NULL buffer?

> +=C2=A0 @param[out] Rand=C2=A0 =C2=A0 =C2=A0Buffer pointer to store th= e 16-bit random value.
> +
> +=C2=A0 @retval TRUE=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Random number ge= nerated successfully.
> +=C2=A0 @retval FALSE=C2=A0 =C2=A0 =C2=A0 =C2=A0 Failed to generate th= e random number.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +GetRandomNumber16 (
> +=C2=A0 OUT=C2=A0 =C2=A0 =C2=A0UINT16=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 *Rand
> +=C2=A0 )
> +{
> +=C2=A0 EFI_STATUS Status;
> +
> +=C2=A0 if (Rand =3D=3D NULL)
> +=C2=A0 {
> +=C2=A0 =C2=A0 return FALSE;
> +=C2=A0 }
> +
> +=C2=A0 Status =3D GenerateRandomNumberViaNist800Algorithm ((UINT8 *)R= and, 2);
> +=C2=A0 if (EFI_ERROR(Status))
> +=C2=A0 {
> +=C2=A0 =C2=A0 return FALSE;
> +=C2=A0 }
> +=C2=A0 return TRUE;
> +}
> +
> +/**
> +=C2=A0 Generates a 32-bit random number.
> +
> +=C2=A0 if Rand is NULL, return FALSE.
> +
> +=C2=A0 @param[out] Rand=C2=A0 =C2=A0 =C2=A0Buffer pointer to store th= e 32-bit random value.
> +
> +=C2=A0 @retval TRUE=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Random number ge= nerated successfully.
> +=C2=A0 @retval FALSE=C2=A0 =C2=A0 =C2=A0 =C2=A0 Failed to generate th= e random number.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +GetRandomNumber32 (
> +=C2=A0 OUT=C2=A0 =C2=A0 =C2=A0UINT32=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 *Rand
> +=C2=A0 )
> +{
> +=C2=A0 EFI_STATUS Status;
> +
> +=C2=A0 if (Rand =3D=3D NULL) {
> +=C2=A0 =C2=A0 return FALSE;
> +=C2=A0 }
> +
> +=C2=A0 Status =3D GenerateRandomNumberViaNist800Algorithm ((UINT8 *)R= and, 4);
> +=C2=A0 if (EFI_ERROR(Status)) {
> +=C2=A0 =C2=A0 return FALSE;
> +=C2=A0 }
> +=C2=A0 return TRUE;
> +}
> +
> +/**
> +=C2=A0 Generates a 64-bit random number.
> +
> +=C2=A0 if Rand is NULL, return FALSE.
> +
> +=C2=A0 @param[out] Rand=C2=A0 =C2=A0 =C2=A0Buffer pointer to store th= e 64-bit random value.
> +
> +=C2=A0 @retval TRUE=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Random number ge= nerated successfully.
> +=C2=A0 @retval FALSE=C2=A0 =C2=A0 =C2=A0 =C2=A0 Failed to generate th= e random number.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +GetRandomNumber64 (
> +=C2=A0 OUT=C2=A0 =C2=A0 =C2=A0UINT64=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 *Rand
> +=C2=A0 )
> +{
> +=C2=A0 EFI_STATUS Status;
> +
> +=C2=A0 if (Rand =3D=3D NULL)
> +=C2=A0 {
> +=C2=A0 =C2=A0 return FALSE;
> +=C2=A0 }
> +
> +=C2=A0 Status =3D GenerateRandomNumberViaNist800Algorithm ((UINT8 *)R= and, 8);
> +=C2=A0 if (EFI_ERROR(Status)) {
> +=C2=A0 =C2=A0 return FALSE;
> +=C2=A0 }
> +=C2=A0 return TRUE;
> +}
> +
> +/**
> +=C2=A0 Generates a 128-bit random number.
> +
> +=C2=A0 if Rand is NULL, return FALSE.
> +
> +=C2=A0 @param[out] Rand=C2=A0 =C2=A0 =C2=A0Buffer pointer to store th= e 128-bit random value.
> +
> +=C2=A0 @retval TRUE=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Random number ge= nerated successfully.
> +=C2=A0 @retval FALSE=C2=A0 =C2=A0 =C2=A0 =C2=A0 Failed to generate th= e random number.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +GetRandomNumber128 (
> +=C2=A0 OUT=C2=A0 =C2=A0 =C2=A0UINT64=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 *Rand
> +=C2=A0 )
> +{
> +=C2=A0 EFI_STATUS Status;
> +
> +=C2=A0 if (Rand =3D=3D NULL) {
> +=C2=A0 =C2=A0 return FALSE;
> +=C2=A0 }
> +
> +=C2=A0 Status =3D GenerateRandomNumberViaNist800Algorithm ((UINT8 *)R= and, 16);
> +=C2=A0 if (EFI_ERROR(Status)) {
> +=C2=A0 =C2=A0 return FALSE;
> +=C2=A0 }
> +=C2=A0 return TRUE;
> +}
> diff --git a/MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf b/MdePkg/L= ibrary/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 R= ngProtocol
> +#
> +# @copyright
> +# Copyright (c) Microsoft Corporation. All rights reserved.
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +# MU_CHANGE: New file

Drop this please

> +##
> +
> +[Defines]
> +=C2=A0 INF_VERSION=C2=A0 =C2=A0 =C2=A0=3D 0x00010017
> +=C2=A0 BASE_NAME=C2=A0 =C2=A0 =C2=A0 =C2=A0=3D BaseRngLibDxe
> +=C2=A0 FILE_GUID=C2=A0 =C2=A0 =C2=A0 =C2=A0=3D FF9F84C5-A33E-44E3-9BB= 5-0D654B2D4149
> +=C2=A0 MODULE_TYPE=C2=A0 =C2=A0 =C2=A0=3D DXE_DRIVER
> +=C2=A0 VERSION_STRING=C2=A0 =3D 1.0
> +=C2=A0 LIBRARY_CLASS=C2=A0 =C2=A0=3D RngLib|DXE_DRIVER UEFI_APPLICATI= ON UEFI_DRIVER
> +
> +[Packages]
> +=C2=A0 MdePkg/MdePkg.dec
> +
> +[Sources]
> +=C2=A0 RngDxeLib.c
> +
> +[LibraryClasses]
> +=C2=A0 DebugLib
> +=C2=A0 UefiBootServicesTableLib
> +
> +[Protocols]
> +=C2=A0 gEfiRngProtocolGuid=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0## CONSUMES
> +
> +[Depex]
> +=C2=A0 gEfiRngProtocolGuid
> +
> +[Guids]
> +=C2=A0 gEfiRngAlgorithmSp80090Ctr256Guid=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 ## CONSUMES
> +=C2=A0 gEfiRngAlgorithmSp80090Hash256Guid=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0## CONSUMES
> +=C2=A0 gEfiRngAlgorithmSp80090Hmac256Guid=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0## CONSUMES

I'm not sure these annotations are appropriate for the [Guids] section<= br>
> 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 @@
>=C2=A0 =C2=A0 =C2=A0MdePkg/Library/BasePostCodeLibPort80/BasePostCodeLi= bPort80.inf
>=C2=A0 =C2=A0 =C2=A0MdePkg/Library/BasePrintLib/BasePrintLib.inf
>=C2=A0 =C2=A0 =C2=A0MdePkg/Library/BaseReportStatusCodeLibNull/BaseRepo= rtStatusCodeLibNull.inf
> -=C2=A0 MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
> +=C2=A0 MdePkg/Library/BaseRngLibDxe/BaseRngLibDxe.inf
>=C2=A0 =C2=A0 =C2=A0MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf > +=C2=A0 MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
> +
>=C2=A0 =C2=A0 =C2=A0MdePkg/Library/BaseSerialPortLibNull/BaseSerialPort= LibNull.inf
>=C2=A0 =C2=A0 =C2=A0MdePkg/Library/BaseSynchronizationLib/BaseSynchroni= zationLib.inf
>=C2=A0 =C2=A0 =C2=A0MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLi= bNullTemplate.inf
>

--000000000000a6918b05acc72aa6--