Re: [PATCH v6 2/5] MdePkg: BaseRngLibDxe: Add RngLib that uses RngDxe

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

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
>