Re: [PATCH v1 5/8] MdePkg/Rng: Add GetRngGuid() to RngLib

["Sami Mujawar" <sami.mujawar@arm.com>]

Hi Pierre,

Thank you for this patch.

Please find my response inline marked [SAMI].

With that addressed,

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

On 09/05/2023 08:40 am, pierre.gondois@arm.com wrote:
> From: Pierre Gondois <pierre.gondois@arm.com>
>
> The EFI_RNG_PROTOCOL can use the RngLib. The RngLib has multiple
> implementations, some of them are unsafe (e.g. BaseRngLibTimerLib).
> To allow the RngDxe to detect when such implementation is used,
> add a GetRngGuid() function to the RngLib.
>
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>   MdePkg/Include/Library/RngLib.h               | 17 ++++++++
>   MdePkg/Library/BaseRngLib/AArch64/Rndr.c      | 42 +++++++++++++++++++
>   MdePkg/Library/BaseRngLib/BaseRngLib.inf      |  9 ++++
>   MdePkg/Library/BaseRngLib/Rand/RdRand.c       | 26 ++++++++++++
>   .../Library/BaseRngLibNull/BaseRngLibNull.c   | 22 ++++++++++
>   .../BaseRngLibTimerLib/BaseRngLibTimerLib.inf |  3 ++
>   .../Library/BaseRngLibTimerLib/RngLibTimer.c  | 28 +++++++++++++
>   MdePkg/Library/DxeRngLib/DxeRngLib.c          | 28 +++++++++++++
>   8 files changed, 175 insertions(+)
>
> diff --git a/MdePkg/Include/Library/RngLib.h b/MdePkg/Include/Library/RngLib.h
> index 429ed19e287e..945482cd5e56 100644
> --- a/MdePkg/Include/Library/RngLib.h
> +++ b/MdePkg/Include/Library/RngLib.h
> @@ -1,6 +1,7 @@
>   /** @file
>     Provides random number generator services.
>   
> +Copyright (c) 2023, Arm Limited. All rights reserved.<BR>
>   Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
>   SPDX-License-Identifier: BSD-2-Clause-Patent
>   
> @@ -77,4 +78,20 @@ GetRandomNumber128 (
>     OUT     UINT64  *Rand
>     );
>   
> +/**
> +  Get a GUID identifying the RNG algorithm implementation.
> +
> +  @param [out] RngGuid  If success, contains the GUID identifying
> +                        the RNG algorithm implementation.
> +
> +  @retval EFI_SUCCESS             Success.
> +  @retval EFI_UNSUPPORTED         Not supported.
> +  @retval EFI_INVALID_PARAMETER   Invalid parameter.
> +**/
> +EFI_STATUS
> +EFIAPI
> +GetRngGuid (
> +  GUID  *RngGuid
> +  );
> +
>   #endif // __RNG_LIB_H__
> diff --git a/MdePkg/Library/BaseRngLib/AArch64/Rndr.c b/MdePkg/Library/BaseRngLib/AArch64/Rndr.c
> index 20811bf3ebf3..d39db62153ee 100644
> --- a/MdePkg/Library/BaseRngLib/AArch64/Rndr.c
> +++ b/MdePkg/Library/BaseRngLib/AArch64/Rndr.c
> @@ -2,6 +2,7 @@
>     Random number generator service that uses the RNDR instruction
>     to provide pseudorandom numbers.
>   
> +  Copyright (c) 2023, Arm Limited. All rights reserved.<BR>
>     Copyright (c) 2021, NUVIA Inc. All rights reserved.<BR>
>     Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
>   
> @@ -11,6 +12,7 @@
>   
>   #include <Uefi.h>
>   #include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
>   #include <Library/DebugLib.h>
>   #include <Library/RngLib.h>
>   
> @@ -138,3 +140,43 @@ ArchIsRngSupported (
>   {
>     return mRndrSupported;
>   }
> +
> +/**
> +  Get a GUID identifying the RNG algorithm implementation.
> +
> +  @param [out] RngGuid  If success, contains the GUID identifying
> +                        the RNG algorithm implementation.
> +
> +  @retval EFI_SUCCESS             Success.
> +  @retval EFI_UNSUPPORTED         Not supported.
> +  @retval EFI_INVALID_PARAMETER   Invalid parameter.
> +**/
> +EFI_STATUS
> +EFIAPI
> +GetRngGuid (
> +  GUID  *RngGuid
> +  )
> +{
> +  GUID  *RngLibGuid;
> +
> +  if (RngGuid == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (!mRndrSupported) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  //
> +  // If the platform advertises the algorithm behind RNDR instruction,
> +  // use it. Otherwise use gEfiRngAlgorithmArmRndr.
> +  //
> +  RngLibGuid = PcdGetPtr (PcdCpuRngSupportedAlgorithm);
> +  if (!IsZeroGuid (RngLibGuid)) {
> +    CopyMem (RngGuid, RngLibGuid, sizeof (*RngGuid));
> +  } else {
> +    CopyMem (RngGuid, &gEfiRngAlgorithmArmRndr, sizeof (*RngGuid));
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> diff --git a/MdePkg/Library/BaseRngLib/BaseRngLib.inf b/MdePkg/Library/BaseRngLib/BaseRngLib.inf
> index 1fcceb941495..a79fbf03d74c 100644
> --- a/MdePkg/Library/BaseRngLib/BaseRngLib.inf
> +++ b/MdePkg/Library/BaseRngLib/BaseRngLib.inf
> @@ -4,6 +4,7 @@
>   #  BaseRng Library that uses CPU RNG instructions (e.g. RdRand) to
>   #  provide random numbers.
>   #
> +#  Copyright (c) 2023, Arm Limited. All rights reserved.<BR>
>   #  Copyright (c) 2021, NUVIA Inc. All rights reserved.<BR>
>   #  Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
>   #
> @@ -43,9 +44,17 @@ [Sources.AARCH64]
>     AArch64/ArmReadIdIsar0.asm | MSFT
>     AArch64/ArmRng.asm         | MSFT
>   
> +[Guids]
> +  gEfiRngAlgorithmArmRndr
> +  gEfiRngAlgorithmSp80090Ctr256Guid
> +  gEfiRngAlgorithmUnSafe

[SAMI] Maybe we can split the [Guids] section into [Guids.AARCH64] and 
[Guids.Ia32, Guids.X64].

Also gEfiRngAlgorithmUnSafe does not appear to be used and can be removed.

[/SAMI]

> +
>   [Packages]
>     MdePkg/MdePkg.dec
>   
> +[Pcd.AARCH64]
> +  gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm
> +
>   [LibraryClasses]
>     BaseLib
>     DebugLib
> diff --git a/MdePkg/Library/BaseRngLib/Rand/RdRand.c b/MdePkg/Library/BaseRngLib/Rand/RdRand.c
> index 070d41e2555f..9bd68352f9f7 100644
> --- a/MdePkg/Library/BaseRngLib/Rand/RdRand.c
> +++ b/MdePkg/Library/BaseRngLib/Rand/RdRand.c
> @@ -2,6 +2,7 @@
>     Random number generator services that uses RdRand instruction access
>     to provide high-quality random numbers.
>   
> +Copyright (c) 2023, Arm Limited. All rights reserved.<BR>
>   Copyright (c) 2021, NUVIA Inc. All rights reserved.<BR>
>   Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
>   
> @@ -11,6 +12,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>   
>   #include <Uefi.h>
>   #include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
>   #include <Library/DebugLib.h>
>   
>   #include "BaseRngLibInternals.h"
> @@ -128,3 +130,27 @@ ArchIsRngSupported (
>     */
>     return TRUE;
>   }
> +
> +/**
> +  Get a GUID identifying the RNG algorithm implementation.
> +
> +  @param [out] RngGuid  If success, contains the GUID identifying
> +                        the RNG algorithm implementation.
> +
> +  @retval EFI_SUCCESS             Success.
> +  @retval EFI_UNSUPPORTED         Not supported.
> +  @retval EFI_INVALID_PARAMETER   Invalid parameter.
> +**/
> +EFI_STATUS
> +EFIAPI
> +GetRngGuid (
> +  GUID  *RngGuid
> +  )
> +{
> +  if (RngGuid == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  CopyMem (RngGuid, &gEfiRngAlgorithmSp80090Ctr256Guid, sizeof (*RngGuid));
> +  return EFI_SUCCESS;
> +}
> diff --git a/MdePkg/Library/BaseRngLibNull/BaseRngLibNull.c b/MdePkg/Library/BaseRngLibNull/BaseRngLibNull.c
> index efba5c851ead..af5e8eb8f72a 100644
> --- a/MdePkg/Library/BaseRngLibNull/BaseRngLibNull.c
> +++ b/MdePkg/Library/BaseRngLibNull/BaseRngLibNull.c
> @@ -1,13 +1,16 @@
>   /** @file
>     Null version of Random number generator services.
>   
> +Copyright (c) 2023, Arm Limited. All rights reserved.<BR>
>   Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
>   SPDX-License-Identifier: BSD-2-Clause-Patent
>   
>   **/
>   
> +#include <Uefi.h>
>   #include <Library/DebugLib.h>
>   #include <Library/RngLib.h>
> +#include <Protocol/Rng.h>
>   
>   /**
>     Generates a 16-bit random number.
> @@ -92,3 +95,22 @@ GetRandomNumber128 (
>     ASSERT (FALSE);
>     return FALSE;
>   }
> +
> +/**
> +  Get a GUID identifying the RNG algorithm implementation.
> +
> +  @param [out] RngGuid  If success, contains the GUID identifying
> +                        the RNG algorithm implementation.
> +
> +  @retval EFI_SUCCESS             Success.
> +  @retval EFI_UNSUPPORTED         Not supported.
> +  @retval EFI_INVALID_PARAMETER   Invalid parameter.
> +**/
> +EFI_STATUS
> +EFIAPI
> +GetRngGuid (
> +  GUID  *RngGuid
> +  )
> +{
> +  return EFI_UNSUPPORTED;
> +}
> diff --git a/MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf b/MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
> index f857290e823b..13e10141fad0 100644
> --- a/MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
> +++ b/MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
> @@ -30,6 +30,9 @@ [Sources]
>   [Packages]
>     MdePkg/MdePkg.dec
>   
> +[Guids]
> +  gEfiRngAlgorithmUnSafe
> +
>   [LibraryClasses]
>     BaseLib
>     DebugLib
> diff --git a/MdePkg/Library/BaseRngLibTimerLib/RngLibTimer.c b/MdePkg/Library/BaseRngLibTimerLib/RngLibTimer.c
> index 980854d67b72..fc9f7e31a333 100644
> --- a/MdePkg/Library/BaseRngLibTimerLib/RngLibTimer.c
> +++ b/MdePkg/Library/BaseRngLibTimerLib/RngLibTimer.c
> @@ -2,14 +2,18 @@
>     BaseRng Library that uses the TimerLib to provide reasonably random numbers.
>     Do not use this on a production system.
>   
> +  Copyright (c) 2023, Arm Limited. All rights reserved.
>     Copyright (c) Microsoft Corporation.
>     SPDX-License-Identifier: BSD-2-Clause-Patent
>   **/
>   
>   #include <Base.h>
> +#include <Uefi.h>
>   #include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
>   #include <Library/DebugLib.h>
>   #include <Library/TimerLib.h>
> +#include <Protocol/Rng.h>
>   
>   #define DEFAULT_DELAY_TIME_IN_MICROSECONDS  10
>   
> @@ -190,3 +194,27 @@ GetRandomNumber128 (
>     // Read second 64 bits
>     return GetRandomNumber64 (++Rand);
>   }
> +
> +/**
> +  Get a GUID identifying the RNG algorithm implementation.
> +
> +  @param [out] RngGuid  If success, contains the GUID identifying
> +                        the RNG algorithm implementation.
> +
> +  @retval EFI_SUCCESS             Success.
> +  @retval EFI_UNSUPPORTED         Not supported.
> +  @retval EFI_INVALID_PARAMETER   Invalid parameter.
> +**/
> +EFI_STATUS
> +EFIAPI
> +GetRngGuid (
> +  GUID  *RngGuid
> +  )
> +{
> +  if (RngGuid == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  CopyMem (RngGuid, &gEfiRngAlgorithmUnSafe, sizeof (*RngGuid));
> +  return EFI_SUCCESS;
> +}
> diff --git a/MdePkg/Library/DxeRngLib/DxeRngLib.c b/MdePkg/Library/DxeRngLib/DxeRngLib.c
> index a01b66ad7d20..cd23859e3112 100644
> --- a/MdePkg/Library/DxeRngLib/DxeRngLib.c
> +++ b/MdePkg/Library/DxeRngLib/DxeRngLib.c
> @@ -1,6 +1,7 @@
>   /** @file
>    Provides an implementation of the library class RngLib that uses the Rng protocol.
>   
> + Copyright (c) 2023, Arm Limited. All rights reserved.
>    Copyright (c) Microsoft Corporation. All rights reserved.
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>   
> @@ -207,3 +208,30 @@ GetRandomNumber128 (
>   
>     return TRUE;
>   }
> +
> +/**
> +  Get a GUID identifying the RNG algorithm implementation.
> +
> +  @param [out] RngGuid  If success, contains the GUID identifying
> +                        the RNG algorithm implementation.
> +
> +  @retval EFI_SUCCESS             Success.
> +  @retval EFI_UNSUPPORTED         Not supported.
> +  @retval EFI_INVALID_PARAMETER   Invalid parameter.
> +**/
> +EFI_STATUS
> +EFIAPI
> +GetRngGuid (
> +  GUID  *RngGuid
> +  )
> +{
> +  /* It is not possible to know beforehand which Rng algorithm will
> +     be used by this library.
> +     This API is mainly used by RngDxe. RngDxe relies on the RngLib.
> +     The RngLib|DxeRngLib.inf implementation locates and uses an installed
> +     EFI_RNG_PROTOCOL.
> +     It is thus not possible to have both RngDxe and RngLib|DxeRngLib.inf.
> +     and it is ok not to support this API.
> +  */
> +  return EFI_UNSUPPORTED;
> +}