[PATCH v1 0/8] SecurityPkg/MdePkg: RngLib GUID

[PATCH v1 0/8] SecurityPkg/MdePkg: RngLib GUID

[PierreGondois]

From: Pierre Gondois <pierre.gondois@arm.com>

This patchset follows the 'code first' approach and relies on [1].
This patchset follows the thread at [3] that aims to solve [2].
[1] and [2] are bound and this patchset aims to solve both.

In this patchset:
a-
The RngDxe can rely on the RngLib. However the RngLib has no
interface allowing to describe which Rng algorithm is implemented.
The RngDxe must advertise the algorithm that are available through
the RngGetInfo() callback.
Add a GetRngGuid() for interface to the RngLib.

b-
The Arm Architecture states the RNDR that the DRBG algorithm should
be compliant with NIST SP800-90A, while not mandating a particular
algorithm, so as to be inclusive of different geographies.
The RngLib can rely on this Arm RNDR instruction. In order to
accurately describe the implementation using the RNDR instruction,
add a EFI_RNG_ALGORITHM_ARM_RNDR GUID [1].

c-
For the same reason as a/b, add a GUID describing unsafe RNG
algorithms, allowing to accurately describe the BaseRngLibTimerLib.

d-
Use a/b/c mechanisms/GUIDs to select a safe Rng algorithm in the
Arm implementation of the RngDxe.

[1] BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4441
[2] BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4151
[3] https://edk2.groups.io/g/devel/message/100806

Pierre Gondois (8):
  MdePkg/ArmTrngLib: Remove ASSERTs in Null implementation
  MdePkg/MdePkg.dec: Move PcdCpuRngSupportedAlgorithm to MdePkg
  MdePkg/DxeRngLib: Request raw algorithm instead of default
  MdePkg/Rng: Add GUIDs to describe Rng algorithms
  MdePkg/Rng: Add GetRngGuid() to RngLib
  SecurityPkg/RngDxe: Use GetRngGuid() when probing RngLib
  SecurityPkg/RngDxe: Select safe default Rng algorithm
  SecurityPkg/RngDxe: Simplify Rng algorithm selection for Arm

 MdePkg/Include/Library/RngLib.h               | 17 +++++
 MdePkg/Include/Protocol/Rng.h                 | 20 ++++++
 .../BaseArmTrngLibNull/BaseArmTrngLibNull.c   |  4 --
 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          | 36 +++++++++-
 MdePkg/MdePkg.dec                             |  7 ++
 .../RngDxe/AArch64/AArch64Algo.c              | 70 +++++++++++++++----
 .../RandomNumberGenerator/RngDxe/ArmRngDxe.c  | 23 +++---
 .../RandomNumberGenerator/RngDxe/RngDxe.inf   |  5 +-
 SecurityPkg/SecurityPkg.dec                   |  2 -
 15 files changed, 278 insertions(+), 36 deletions(-)

-- 
2.25.1

[PATCH v1 1/8] MdePkg/ArmTrngLib: Remove ASSERTs in Null implementation

[PierreGondois]

From: Pierre Gondois <pierre.gondois@arm.com>

Remove ASSERTs to allow RngDxe probing the Null implementation
of the TrngLib.

Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
---
 MdePkg/Library/BaseArmTrngLibNull/BaseArmTrngLibNull.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/MdePkg/Library/BaseArmTrngLibNull/BaseArmTrngLibNull.c b/MdePkg/Library/BaseArmTrngLibNull/BaseArmTrngLibNull.c
index 316d78bf5e83..0ea9aafa59f1 100644
--- a/MdePkg/Library/BaseArmTrngLibNull/BaseArmTrngLibNull.c
+++ b/MdePkg/Library/BaseArmTrngLibNull/BaseArmTrngLibNull.c
@@ -41,7 +41,6 @@ GetArmTrngVersion (
   OUT UINT16  *MinorRevision
   )
 {
-  ASSERT (FALSE);
   return RETURN_UNSUPPORTED;
 }
 
@@ -67,7 +66,6 @@ GetArmTrngUuid (
   OUT GUID  *Guid
   )
 {
-  ASSERT (FALSE);
   return RETURN_UNSUPPORTED;
 }
 
@@ -83,7 +81,6 @@ GetArmTrngMaxSupportedEntropyBits (
   VOID
   )
 {
-  ASSERT (FALSE);
   return 0;
 }
 
@@ -116,6 +113,5 @@ GetArmTrngEntropy (
   OUT UINT8  *Buffer
   )
 {
-  ASSERT (FALSE);
   return RETURN_UNSUPPORTED;
 }
-- 
2.25.1

Re: [PATCH v1 1/8] MdePkg/ArmTrngLib: Remove ASSERTs in Null implementation

[Sami Mujawar]

Hi Pierre,

Thank you for this patch.

This change looks good to me.

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>
>
> Remove ASSERTs to allow RngDxe probing the Null implementation
> of the TrngLib.
>
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>   MdePkg/Library/BaseArmTrngLibNull/BaseArmTrngLibNull.c | 4 ----
>   1 file changed, 4 deletions(-)
>
> diff --git a/MdePkg/Library/BaseArmTrngLibNull/BaseArmTrngLibNull.c b/MdePkg/Library/BaseArmTrngLibNull/BaseArmTrngLibNull.c
> index 316d78bf5e83..0ea9aafa59f1 100644
> --- a/MdePkg/Library/BaseArmTrngLibNull/BaseArmTrngLibNull.c
> +++ b/MdePkg/Library/BaseArmTrngLibNull/BaseArmTrngLibNull.c
> @@ -41,7 +41,6 @@ GetArmTrngVersion (
>     OUT UINT16  *MinorRevision
>     )
>   {
> -  ASSERT (FALSE);
>     return RETURN_UNSUPPORTED;
>   }
>   
> @@ -67,7 +66,6 @@ GetArmTrngUuid (
>     OUT GUID  *Guid
>     )
>   {
> -  ASSERT (FALSE);
>     return RETURN_UNSUPPORTED;
>   }
>   
> @@ -83,7 +81,6 @@ GetArmTrngMaxSupportedEntropyBits (
>     VOID
>     )
>   {
> -  ASSERT (FALSE);
>     return 0;
>   }
>   
> @@ -116,6 +113,5 @@ GetArmTrngEntropy (
>     OUT UINT8  *Buffer
>     )
>   {
> -  ASSERT (FALSE);
>     return RETURN_UNSUPPORTED;
>   }

Re: [edk2-devel] [PATCH v1 1/8] MdePkg/ArmTrngLib: Remove ASSERTs in Null implementation

[Kun Qin]

Hi Pierre,

Do we really need this removal of ASSERT? I tried to use the real 
ArmTrngLib with this patch
and it seems to work fine with a TFA that does not support TRNG interfaces.

I think it would be valuable to keep the ASSERT to indicate there might 
be an integration error?

Please let me know if I missed anything.

Regards,
Kun

On 5/9/2023 12:40 AM, PierreGondois wrote:
> From: Pierre Gondois <pierre.gondois@arm.com>
>
> Remove ASSERTs to allow RngDxe probing the Null implementation
> of the TrngLib.
>
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>   MdePkg/Library/BaseArmTrngLibNull/BaseArmTrngLibNull.c | 4 ----
>   1 file changed, 4 deletions(-)
>
> diff --git a/MdePkg/Library/BaseArmTrngLibNull/BaseArmTrngLibNull.c b/MdePkg/Library/BaseArmTrngLibNull/BaseArmTrngLibNull.c
> index 316d78bf5e83..0ea9aafa59f1 100644
> --- a/MdePkg/Library/BaseArmTrngLibNull/BaseArmTrngLibNull.c
> +++ b/MdePkg/Library/BaseArmTrngLibNull/BaseArmTrngLibNull.c
> @@ -41,7 +41,6 @@ GetArmTrngVersion (
>     OUT UINT16  *MinorRevision
>     )
>   {
> -  ASSERT (FALSE);
>     return RETURN_UNSUPPORTED;
>   }
>   
> @@ -67,7 +66,6 @@ GetArmTrngUuid (
>     OUT GUID  *Guid
>     )
>   {
> -  ASSERT (FALSE);
>     return RETURN_UNSUPPORTED;
>   }
>   
> @@ -83,7 +81,6 @@ GetArmTrngMaxSupportedEntropyBits (
>     VOID
>     )
>   {
> -  ASSERT (FALSE);
>     return 0;
>   }
>   
> @@ -116,6 +113,5 @@ GetArmTrngEntropy (
>     OUT UINT8  *Buffer
>     )
>   {
> -  ASSERT (FALSE);
>     return RETURN_UNSUPPORTED;
>   }

Re: [edk2-devel] [PATCH v1 1/8] MdePkg/ArmTrngLib: Remove ASSERTs in Null implementation

[PierreGondois]

Hello Kun,
The reason was that:
- KvmTool uses the SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf module
- the RngDxe.inf modules requires an ArmTrng implementation
- GetArmTrngVersion() is called to check whether there is a Trng backend
- GetArmTrngVersion() asserts as it is a NULL implementation

but it seems the actual implementation of the ArmTrngLib could be used instead.
The returned error code is actually the same (UNSUPPORTED). I will check with
Sami if this is ok to do this instead.

Regards,
Pierre

On 6/29/23 22:34, Kun Qin wrote:
> Hi Pierre,
> 
> Do we really need this removal of ASSERT? I tried to use the real
> ArmTrngLib with this patch
> and it seems to work fine with a TFA that does not support TRNG interfaces.
> 
> I think it would be valuable to keep the ASSERT to indicate there might
> be an integration error?
> 
> Please let me know if I missed anything.
> 
> Regards,
> Kun
> 
> On 5/9/2023 12:40 AM, PierreGondois wrote:
>> From: Pierre Gondois <pierre.gondois@arm.com>
>>
>> Remove ASSERTs to allow RngDxe probing the Null implementation
>> of the TrngLib.
>>
>> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
>> ---
>>    MdePkg/Library/BaseArmTrngLibNull/BaseArmTrngLibNull.c | 4 ----
>>    1 file changed, 4 deletions(-)
>>
>> diff --git a/MdePkg/Library/BaseArmTrngLibNull/BaseArmTrngLibNull.c b/MdePkg/Library/BaseArmTrngLibNull/BaseArmTrngLibNull.c
>> index 316d78bf5e83..0ea9aafa59f1 100644
>> --- a/MdePkg/Library/BaseArmTrngLibNull/BaseArmTrngLibNull.c
>> +++ b/MdePkg/Library/BaseArmTrngLibNull/BaseArmTrngLibNull.c
>> @@ -41,7 +41,6 @@ GetArmTrngVersion (
>>      OUT UINT16  *MinorRevision
>>      )
>>    {
>> -  ASSERT (FALSE);
>>      return RETURN_UNSUPPORTED;
>>    }
>>    
>> @@ -67,7 +66,6 @@ GetArmTrngUuid (
>>      OUT GUID  *Guid
>>      )
>>    {
>> -  ASSERT (FALSE);
>>      return RETURN_UNSUPPORTED;
>>    }
>>    
>> @@ -83,7 +81,6 @@ GetArmTrngMaxSupportedEntropyBits (
>>      VOID
>>      )
>>    {
>> -  ASSERT (FALSE);
>>      return 0;
>>    }
>>    
>> @@ -116,6 +113,5 @@ GetArmTrngEntropy (
>>      OUT UINT8  *Buffer
>>      )
>>    {
>> -  ASSERT (FALSE);
>>      return RETURN_UNSUPPORTED;
>>    }

[PATCH v1 2/8] MdePkg/MdePkg.dec: Move PcdCpuRngSupportedAlgorithm to MdePkg

[PierreGondois]

From: Pierre Gondois <pierre.gondois@arm.com>

In order to use PcdCpuRngSupportedAlgorithm in the MdePkg in a
following patch and to avoid making the MdePkg dependent on another
package, move PcdCpuRngSupportedAlgorithm to the MdePkg.

As the Pcf is only used for AARCH64, place it in an AARCH64
specific sections.

Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
---
 MdePkg/MdePkg.dec                                   | 5 +++++
 SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf | 4 ++--
 SecurityPkg/SecurityPkg.dec                         | 2 --
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index d6c4179b2a48..0ecfad5795e4 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -2357,6 +2357,11 @@ [PcdsFixedAtBuild,PcdsPatchableInModule]
   # @Prompt IPMI KCS Interface I/O Base Address
   gEfiMdePkgTokenSpaceGuid.PcdIpmiKcsIoBaseAddress|0xca2|UINT16|0x00000031
 
+[PcdsFixedAtBuild.AARCH64, PcdsPatchableInModule.AARCH64]
+  ## GUID identifying the Rng algorithm implemented by CPU instruction.
+  # @Prompt CPU Rng algorithm's GUID.
+  gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm|{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}|VOID*|0x00000032
+
 [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
   ## This value is used to set the base address of PCI express hierarchy.
   # @Prompt PCI Express Base Address.
diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf b/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
index c8e0ee4ae5d9..d6c2d30195bf 100644
--- a/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
+++ b/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
@@ -79,8 +79,8 @@ [Guids]
 [Protocols]
   gEfiRngProtocolGuid                ## PRODUCES
 
-[Pcd]
-  gEfiSecurityPkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm      ## CONSUMES
+[Pcd.AARCH64]
+  gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm      ## CONSUMES
 
 [Depex]
   TRUE
diff --git a/SecurityPkg/SecurityPkg.dec b/SecurityPkg/SecurityPkg.dec
index 0a8042d63fe1..6bb02d58bdf0 100644
--- a/SecurityPkg/SecurityPkg.dec
+++ b/SecurityPkg/SecurityPkg.dec
@@ -325,8 +325,6 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
   gEfiSecurityPkgTokenSpaceGuid.PcdStatusCodeFvVerificationPass|0x0303100A|UINT32|0x00010030
   gEfiSecurityPkgTokenSpaceGuid.PcdStatusCodeFvVerificationFail|0x0303100B|UINT32|0x00010031
 
-  gEfiSecurityPkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm|{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}|VOID*|0x00010032
-
 [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
   ## Image verification policy for OptionRom. Only following values are valid:<BR><BR>
   #  NOTE: Do NOT use 0x5 and 0x2 since it violates the UEFI specification and has been removed.<BR>
-- 
2.25.1

Re: [PATCH v1 2/8] MdePkg/MdePkg.dec: Move PcdCpuRngSupportedAlgorithm to MdePkg

[Sami Mujawar]

Hi Pierre,

Please see my response inline marked [SAMI].

With that fixed,

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>
>
> In order to use PcdCpuRngSupportedAlgorithm in the MdePkg in a
> following patch and to avoid making the MdePkg dependent on another
> package, move PcdCpuRngSupportedAlgorithm to the MdePkg.
>
> As the Pcf is only used for AARCH64, place it in an AARCH64
> specific sections.
>
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>   MdePkg/MdePkg.dec                                   | 5 +++++
>   SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf | 4 ++--
>   SecurityPkg/SecurityPkg.dec                         | 2 --
>   3 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> index d6c4179b2a48..0ecfad5795e4 100644
> --- a/MdePkg/MdePkg.dec
> +++ b/MdePkg/MdePkg.dec
> @@ -2357,6 +2357,11 @@ [PcdsFixedAtBuild,PcdsPatchableInModule]
>     # @Prompt IPMI KCS Interface I/O Base Address
>     gEfiMdePkgTokenSpaceGuid.PcdIpmiKcsIoBaseAddress|0xca2|UINT16|0x00000031
>   
> +[PcdsFixedAtBuild.AARCH64, PcdsPatchableInModule.AARCH64]
> +  ## GUID identifying the Rng algorithm implemented by CPU instruction.
> +  # @Prompt CPU Rng algorithm's GUID.
> +  gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm|{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}|VOID*|0x00000032

[SAMI] Apparently the token value 0x00000032 is already used when 
rebased with latest edk2 code and results in the following build error:

The TokenValue [0x00000032] of PCD 
[gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm] is conflict with: 
[gEfiMdePkgTokenSpaceGuid.PcdIpmiSsifSmbusSlaveAddr]

I believe changing this to 0x00000037 should fix the issue.

[/SAMI]

> +
>   [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>     ## This value is used to set the base address of PCI express hierarchy.
>     # @Prompt PCI Express Base Address.
> diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf b/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
> index c8e0ee4ae5d9..d6c2d30195bf 100644
> --- a/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
> +++ b/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
> @@ -79,8 +79,8 @@ [Guids]
>   [Protocols]
>     gEfiRngProtocolGuid                ## PRODUCES
>   
> -[Pcd]
> -  gEfiSecurityPkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm      ## CONSUMES
> +[Pcd.AARCH64]
> +  gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm      ## CONSUMES
>   
>   [Depex]
>     TRUE
> diff --git a/SecurityPkg/SecurityPkg.dec b/SecurityPkg/SecurityPkg.dec
> index 0a8042d63fe1..6bb02d58bdf0 100644
> --- a/SecurityPkg/SecurityPkg.dec
> +++ b/SecurityPkg/SecurityPkg.dec
> @@ -325,8 +325,6 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>     gEfiSecurityPkgTokenSpaceGuid.PcdStatusCodeFvVerificationPass|0x0303100A|UINT32|0x00010030
>     gEfiSecurityPkgTokenSpaceGuid.PcdStatusCodeFvVerificationFail|0x0303100B|UINT32|0x00010031
>   
> -  gEfiSecurityPkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm|{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}|VOID*|0x00010032
> -
>   [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>     ## Image verification policy for OptionRom. Only following values are valid:<BR><BR>
>     #  NOTE: Do NOT use 0x5 and 0x2 since it violates the UEFI specification and has been removed.<BR>

Re: [edk2-devel] [PATCH v1 2/8] MdePkg/MdePkg.dec: Move PcdCpuRngSupportedAlgorithm to MdePkg

[Kun Qin]

This patch seems to have some discrepancy between the title and content :)

Can you please break this patch into 2, so that MdePkg change and 
SecurityPkg can be their
own commit?

Thanks,
Kun

On 5/9/2023 12:40 AM, PierreGondois wrote:
> From: Pierre Gondois <pierre.gondois@arm.com>
>
> In order to use PcdCpuRngSupportedAlgorithm in the MdePkg in a
> following patch and to avoid making the MdePkg dependent on another
> package, move PcdCpuRngSupportedAlgorithm to the MdePkg.
>
> As the Pcf is only used for AARCH64, place it in an AARCH64
> specific sections.
>
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>   MdePkg/MdePkg.dec                                   | 5 +++++
>   SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf | 4 ++--
>   SecurityPkg/SecurityPkg.dec                         | 2 --
>   3 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> index d6c4179b2a48..0ecfad5795e4 100644
> --- a/MdePkg/MdePkg.dec
> +++ b/MdePkg/MdePkg.dec
> @@ -2357,6 +2357,11 @@ [PcdsFixedAtBuild,PcdsPatchableInModule]
>     # @Prompt IPMI KCS Interface I/O Base Address
>     gEfiMdePkgTokenSpaceGuid.PcdIpmiKcsIoBaseAddress|0xca2|UINT16|0x00000031
>   
> +[PcdsFixedAtBuild.AARCH64, PcdsPatchableInModule.AARCH64]
> +  ## GUID identifying the Rng algorithm implemented by CPU instruction.
> +  # @Prompt CPU Rng algorithm's GUID.
> +  gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm|{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}|VOID*|0x00000032
> +
>   [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>     ## This value is used to set the base address of PCI express hierarchy.
>     # @Prompt PCI Express Base Address.
> diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf b/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
> index c8e0ee4ae5d9..d6c2d30195bf 100644
> --- a/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
> +++ b/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
> @@ -79,8 +79,8 @@ [Guids]
>   [Protocols]
>     gEfiRngProtocolGuid                ## PRODUCES
>   
> -[Pcd]
> -  gEfiSecurityPkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm      ## CONSUMES
> +[Pcd.AARCH64]
> +  gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm      ## CONSUMES
>   
>   [Depex]
>     TRUE
> diff --git a/SecurityPkg/SecurityPkg.dec b/SecurityPkg/SecurityPkg.dec
> index 0a8042d63fe1..6bb02d58bdf0 100644
> --- a/SecurityPkg/SecurityPkg.dec
> +++ b/SecurityPkg/SecurityPkg.dec
> @@ -325,8 +325,6 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>     gEfiSecurityPkgTokenSpaceGuid.PcdStatusCodeFvVerificationPass|0x0303100A|UINT32|0x00010030
>     gEfiSecurityPkgTokenSpaceGuid.PcdStatusCodeFvVerificationFail|0x0303100B|UINT32|0x00010031
>   
> -  gEfiSecurityPkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm|{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}|VOID*|0x00010032
> -
>   [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>     ## Image verification policy for OptionRom. Only following values are valid:<BR><BR>
>     #  NOTE: Do NOT use 0x5 and 0x2 since it violates the UEFI specification and has been removed.<BR>

Re: [edk2-devel] [PATCH v1 2/8] MdePkg/MdePkg.dec: Move PcdCpuRngSupportedAlgorithm to MdePkg

[PierreGondois]

Hello Kun,

On 6/29/23 22:36, Kun Qin wrote:
> This patch seems to have some discrepancy between the title and content :)

I'm not sure I see the discrepancy between the title/content,
should I have mentioned the SecurityPkg ?

> 
> Can you please break this patch into 2, so that MdePkg change and
> SecurityPkg can be their
> own commit?

I think the change is small enough to be in one patch,
I think I already saw patches touching 2 different packages for this
kind of cases, like:
commit 9a24c3546ebe ("MdeModulePkg: Move CPU_EXCEPTION_INIT_DATA to UefiCpuPkg")


Regards,
Pierre

> 
> Thanks,
> Kun
> 
> On 5/9/2023 12:40 AM, PierreGondois wrote:
>> From: Pierre Gondois <pierre.gondois@arm.com>
>>
>> In order to use PcdCpuRngSupportedAlgorithm in the MdePkg in a
>> following patch and to avoid making the MdePkg dependent on another
>> package, move PcdCpuRngSupportedAlgorithm to the MdePkg.
>>
>> As the Pcf is only used for AARCH64, place it in an AARCH64
>> specific sections.
>>
>> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
>> ---
>>    MdePkg/MdePkg.dec                                   | 5 +++++
>>    SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf | 4 ++--
>>    SecurityPkg/SecurityPkg.dec                         | 2 --
>>    3 files changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
>> index d6c4179b2a48..0ecfad5795e4 100644
>> --- a/MdePkg/MdePkg.dec
>> +++ b/MdePkg/MdePkg.dec
>> @@ -2357,6 +2357,11 @@ [PcdsFixedAtBuild,PcdsPatchableInModule]
>>      # @Prompt IPMI KCS Interface I/O Base Address
>>      gEfiMdePkgTokenSpaceGuid.PcdIpmiKcsIoBaseAddress|0xca2|UINT16|0x00000031
>>    
>> +[PcdsFixedAtBuild.AARCH64, PcdsPatchableInModule.AARCH64]
>> +  ## GUID identifying the Rng algorithm implemented by CPU instruction.
>> +  # @Prompt CPU Rng algorithm's GUID.
>> +  gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm|{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}|VOID*|0x00000032
>> +
>>    [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>>      ## This value is used to set the base address of PCI express hierarchy.
>>      # @Prompt PCI Express Base Address.
>> diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf b/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
>> index c8e0ee4ae5d9..d6c2d30195bf 100644
>> --- a/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
>> +++ b/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
>> @@ -79,8 +79,8 @@ [Guids]
>>    [Protocols]
>>      gEfiRngProtocolGuid                ## PRODUCES
>>    
>> -[Pcd]
>> -  gEfiSecurityPkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm      ## CONSUMES
>> +[Pcd.AARCH64]
>> +  gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm      ## CONSUMES
>>    
>>    [Depex]
>>      TRUE
>> diff --git a/SecurityPkg/SecurityPkg.dec b/SecurityPkg/SecurityPkg.dec
>> index 0a8042d63fe1..6bb02d58bdf0 100644
>> --- a/SecurityPkg/SecurityPkg.dec
>> +++ b/SecurityPkg/SecurityPkg.dec
>> @@ -325,8 +325,6 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>>      gEfiSecurityPkgTokenSpaceGuid.PcdStatusCodeFvVerificationPass|0x0303100A|UINT32|0x00010030
>>      gEfiSecurityPkgTokenSpaceGuid.PcdStatusCodeFvVerificationFail|0x0303100B|UINT32|0x00010031
>>    
>> -  gEfiSecurityPkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm|{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}|VOID*|0x00010032
>> -
>>    [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>>      ## Image verification policy for OptionRom. Only following values are valid:<BR><BR>
>>      #  NOTE: Do NOT use 0x5 and 0x2 since it violates the UEFI specification and has been removed.<BR>

Re: [edk2-devel] [PATCH v1 2/8] MdePkg/MdePkg.dec: Move PcdCpuRngSupportedAlgorithm to MdePkg

[Kun Qin]

Hi Pierre,

Thanks for the example. If we are to keep this in one commit, I think we 
should
mention SecurityPkg in the title?

To follow the example, can we rename it to
`SecurityPkg/SecurityPkg.dec: Move PcdCpuRngSupportedAlgorithm to MdePkg`?

At least this makes it more obvious that it is touching 2 packages in 
one commit?

Thanks,
Kun

On 6/30/2023 7:30 AM, Pierre Gondois wrote:
> Hello Kun,
>
> On 6/29/23 22:36, Kun Qin wrote:
>> This patch seems to have some discrepancy between the title and 
>> content :)
>
> I'm not sure I see the discrepancy between the title/content,
> should I have mentioned the SecurityPkg ?
>
>>
>> Can you please break this patch into 2, so that MdePkg change and
>> SecurityPkg can be their
>> own commit?
>
> I think the change is small enough to be in one patch,
> I think I already saw patches touching 2 different packages for this
> kind of cases, like:
> commit 9a24c3546ebe ("MdeModulePkg: Move CPU_EXCEPTION_INIT_DATA to 
> UefiCpuPkg")
>
>
> Regards,
> Pierre
>
>>
>> Thanks,
>> Kun
>>
>> On 5/9/2023 12:40 AM, PierreGondois wrote:
>>> From: Pierre Gondois <pierre.gondois@arm.com>
>>>
>>> In order to use PcdCpuRngSupportedAlgorithm in the MdePkg in a
>>> following patch and to avoid making the MdePkg dependent on another
>>> package, move PcdCpuRngSupportedAlgorithm to the MdePkg.
>>>
>>> As the Pcf is only used for AARCH64, place it in an AARCH64
>>> specific sections.
>>>
>>> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
>>> ---
>>>    MdePkg/MdePkg.dec                                   | 5 +++++
>>>    SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf | 4 ++--
>>>    SecurityPkg/SecurityPkg.dec                         | 2 --
>>>    3 files changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
>>> index d6c4179b2a48..0ecfad5795e4 100644
>>> --- a/MdePkg/MdePkg.dec
>>> +++ b/MdePkg/MdePkg.dec
>>> @@ -2357,6 +2357,11 @@ [PcdsFixedAtBuild,PcdsPatchableInModule]
>>>      # @Prompt IPMI KCS Interface I/O Base Address
>>> gEfiMdePkgTokenSpaceGuid.PcdIpmiKcsIoBaseAddress|0xca2|UINT16|0x00000031 
>>>
>>>    +[PcdsFixedAtBuild.AARCH64, PcdsPatchableInModule.AARCH64]
>>> +  ## GUID identifying the Rng algorithm implemented by CPU 
>>> instruction.
>>> +  # @Prompt CPU Rng algorithm's GUID.
>>> + 
>>> gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm|{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}|VOID*|0x00000032
>>> +
>>>    [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, 
>>> PcdsDynamicEx]
>>>      ## This value is used to set the base address of PCI express 
>>> hierarchy.
>>>      # @Prompt PCI Express Base Address.
>>> diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf 
>>> b/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
>>> index c8e0ee4ae5d9..d6c2d30195bf 100644
>>> --- a/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
>>> +++ b/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
>>> @@ -79,8 +79,8 @@ [Guids]
>>>    [Protocols]
>>>      gEfiRngProtocolGuid                ## PRODUCES
>>>    -[Pcd]
>>> - gEfiSecurityPkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm ## CONSUMES
>>> +[Pcd.AARCH64]
>>> +  gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm ## CONSUMES
>>>       [Depex]
>>>      TRUE
>>> diff --git a/SecurityPkg/SecurityPkg.dec b/SecurityPkg/SecurityPkg.dec
>>> index 0a8042d63fe1..6bb02d58bdf0 100644
>>> --- a/SecurityPkg/SecurityPkg.dec
>>> +++ b/SecurityPkg/SecurityPkg.dec
>>> @@ -325,8 +325,6 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>>> gEfiSecurityPkgTokenSpaceGuid.PcdStatusCodeFvVerificationPass|0x0303100A|UINT32|0x00010030
>>> gEfiSecurityPkgTokenSpaceGuid.PcdStatusCodeFvVerificationFail|0x0303100B|UINT32|0x00010031
>>>    - 
>>> gEfiSecurityPkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm|{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}|VOID*|0x00010032
>>> -
>>>    [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, 
>>> PcdsDynamicEx]
>>>      ## Image verification policy for OptionRom. Only following 
>>> values are valid:<BR><BR>
>>>      #  NOTE: Do NOT use 0x5 and 0x2 since it violates the UEFI 
>>> specification and has been removed.<BR>

[PATCH v1 3/8] MdePkg/DxeRngLib: Request raw algorithm instead of default

[PierreGondois]

From: Pierre Gondois <pierre.gondois@arm.com>

The DxeRngLib tries to generate a random number using the 3 NIST
SP 800-90 compliant DRBG algorithms, i.e. 256-bits CTR, HASH and HMAC.
If none of the call is successful, the fallback option is the default
RNG algorithm of the EFI_RNG_PROTOCOL. This default algorithm might
be an unsafe implementation.

Try requesting the Raw algorithm before requesting the default one.

Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
---
 MdePkg/Library/DxeRngLib/DxeRngLib.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Library/DxeRngLib/DxeRngLib.c b/MdePkg/Library/DxeRngLib/DxeRngLib.c
index 46aea515924f..a01b66ad7d20 100644
--- a/MdePkg/Library/DxeRngLib/DxeRngLib.c
+++ b/MdePkg/Library/DxeRngLib/DxeRngLib.c
@@ -65,9 +65,15 @@ GenerateRandomNumberViaNist800Algorithm (
     return Status;
   }
 
+  Status = RngProtocol->GetRNG (RngProtocol, &gEfiRngAlgorithmRaw, BufferSize, Buffer);
+  DEBUG ((DEBUG_INFO, "%a: GetRNG algorithm Raw - Status = %r\n", __func__, Status));
+  if (!EFI_ERROR (Status)) {
+    return Status;
+  }
+
   // If all the other methods have failed, use the default method from the RngProtocol
   Status = RngProtocol->GetRNG (RngProtocol, NULL, BufferSize, Buffer);
-  DEBUG ((DEBUG_INFO, "%a: GetRNG algorithm Hash-256 - Status = %r\n", __func__, Status));
+  DEBUG ((DEBUG_INFO, "%a: GetRNG algorithm default - Status = %r\n", __func__, Status));
   if (!EFI_ERROR (Status)) {
     return Status;
   }
-- 
2.25.1

Re: [PATCH v1 3/8] MdePkg/DxeRngLib: Request raw algorithm instead of default

[Sami Mujawar]

Hi Pierre,

Thank you for this patch.

These changes look good to me.

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 DxeRngLib tries to generate a random number using the 3 NIST
> SP 800-90 compliant DRBG algorithms, i.e. 256-bits CTR, HASH and HMAC.
> If none of the call is successful, the fallback option is the default
> RNG algorithm of the EFI_RNG_PROTOCOL. This default algorithm might
> be an unsafe implementation.
>
> Try requesting the Raw algorithm before requesting the default one.
>
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>   MdePkg/Library/DxeRngLib/DxeRngLib.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/MdePkg/Library/DxeRngLib/DxeRngLib.c b/MdePkg/Library/DxeRngLib/DxeRngLib.c
> index 46aea515924f..a01b66ad7d20 100644
> --- a/MdePkg/Library/DxeRngLib/DxeRngLib.c
> +++ b/MdePkg/Library/DxeRngLib/DxeRngLib.c
> @@ -65,9 +65,15 @@ GenerateRandomNumberViaNist800Algorithm (
>       return Status;
>     }
>   
> +  Status = RngProtocol->GetRNG (RngProtocol, &gEfiRngAlgorithmRaw, BufferSize, Buffer);
> +  DEBUG ((DEBUG_INFO, "%a: GetRNG algorithm Raw - Status = %r\n", __func__, Status));
> +  if (!EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
>     // If all the other methods have failed, use the default method from the RngProtocol
>     Status = RngProtocol->GetRNG (RngProtocol, NULL, BufferSize, Buffer);
> -  DEBUG ((DEBUG_INFO, "%a: GetRNG algorithm Hash-256 - Status = %r\n", __func__, Status));
> +  DEBUG ((DEBUG_INFO, "%a: GetRNG algorithm default - Status = %r\n", __func__, Status));
>     if (!EFI_ERROR (Status)) {
>       return Status;
>     }

[PATCH v1 4/8] MdePkg/Rng: Add GUIDs to describe Rng algorithms

[PierreGondois]

From: Pierre Gondois <pierre.gondois@arm.com>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4441

The EFI_RNG_PROTOCOL can rely on 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,
a GetRngGuid() function is added in a following patch.

Prepare GetRngGuid() return values and add GUIDs describing
Rng algorithms:
- gEfiRngAlgorithmArmRndr
to describe a Rng algorithm accessed through Arm's RNDR instruction.
[1] states that the implementation of this algorithm should be
compliant to NIST SP900-80. The compliance is not guaranteed.
- gEfiRngAlgorithmUnSafe
to describe an unsafe implementation, cf. the BaseRngLibTimerLib.

[1] Arm Architecture Reference Manual Armv8, for A-profile architecture
sK12.1 'Properties of the generated random number'

Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
---
 MdePkg/Include/Protocol/Rng.h | 20 ++++++++++++++++++++
 MdePkg/MdePkg.dec             |  2 ++
 2 files changed, 22 insertions(+)

diff --git a/MdePkg/Include/Protocol/Rng.h b/MdePkg/Include/Protocol/Rng.h
index baf425587b3c..dfdaf36e41dc 100644
--- a/MdePkg/Include/Protocol/Rng.h
+++ b/MdePkg/Include/Protocol/Rng.h
@@ -67,6 +67,24 @@ typedef EFI_GUID EFI_RNG_ALGORITHM;
   { \
     0xe43176d7, 0xb6e8, 0x4827, {0xb7, 0x84, 0x7f, 0xfd, 0xc4, 0xb6, 0x85, 0x61 } \
   }
+///
+/// The Arm Architecture states the RNDR that the DRBG algorithm should be compliant
+/// with NIST SP800-90A, while not mandating a particular algorithm, so as to be
+/// inclusive of different geographies.
+///
+#define EFI_RNG_ALGORITHM_ARM_RNDR \
+  { \
+    0x43d2fde3, 0x9d4e, 0x4d79,  {0x02, 0x96, 0xa8, 0x9b, 0xca, 0x78, 0x08, 0x41} \
+  }
+///
+/// The implementation of a Random Number Generator might be unsafe, when using
+/// a dummy implementation for instance. Allow identifying such implementation
+/// with this GUID.
+///
+#define EFI_RNG_ALGORITHM_UNSAFE \
+  { \
+    0x869f728c, 0x409d, 0x4ab4, {0xac, 0x03, 0x71, 0xd3, 0x09, 0xc1, 0xb3, 0xf4 } \
+  }
 
 /**
   Returns information about the random number generation implementation.
@@ -146,5 +164,7 @@ extern EFI_GUID  gEfiRngAlgorithmSp80090Ctr256Guid;
 extern EFI_GUID  gEfiRngAlgorithmX9313DesGuid;
 extern EFI_GUID  gEfiRngAlgorithmX931AesGuid;
 extern EFI_GUID  gEfiRngAlgorithmRaw;
+extern EFI_GUID  gEfiRngAlgorithmArmRndr;
+extern EFI_GUID  gEfiRngAlgorithmUnSafe;
 
 #endif
diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index 0ecfad5795e4..754085eaa55b 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -633,6 +633,8 @@ [Guids]
   gEfiRngAlgorithmX9313DesGuid       = { 0x63c4785a, 0xca34, 0x4012, {0xa3, 0xc8, 0x0b, 0x6a, 0x32, 0x4f, 0x55, 0x46 }}
   gEfiRngAlgorithmX931AesGuid        = { 0xacd03321, 0x777e, 0x4d3d, {0xb1, 0xc8, 0x20, 0xcf, 0xd8, 0x88, 0x20, 0xc9 }}
   gEfiRngAlgorithmRaw                = { 0xe43176d7, 0xb6e8, 0x4827, {0xb7, 0x84, 0x7f, 0xfd, 0xc4, 0xb6, 0x85, 0x61 }}
+  gEfiRngAlgorithmArmRndr            = { 0x43d2fde3, 0x9d4e, 0x4d79, {0x02, 0x96, 0xa8, 0x9b, 0xca, 0x78, 0x08, 0x41 }}
+  gEfiRngAlgorithmUnSafe             = { 0x869f728c, 0x409d, 0x4ab4, {0xac, 0x03, 0x71, 0xd3, 0x09, 0xc1, 0xb3, 0xf4 }}
 
   ## Include/Protocol/AdapterInformation.h
   gEfiAdapterInfoMediaStateGuid       = { 0xD7C74207, 0xA831, 0x4A26, {0xB1, 0xF5, 0xD1, 0x93, 0x06, 0x5C, 0xE8, 0xB6 }}
-- 
2.25.1

Re: [PATCH v1 4/8] MdePkg/Rng: Add GUIDs to describe Rng algorithms

[Yao, Jiewen]

Is this defined in UEFI spec? or approved in future UEFI spec?

> -----Original Message-----
> From: pierre.gondois@arm.com <pierre.gondois@arm.com>
> Sent: Tuesday, May 9, 2023 3:41 PM
> To: devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ard
> Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar
> <sami.mujawar@arm.com>; Jose Marinho <Jose.Marinho@arm.com>;
> Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> Subject: [PATCH v1 4/8] MdePkg/Rng: Add GUIDs to describe Rng algorithms
> 
> From: Pierre Gondois <pierre.gondois@arm.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4441
> 
> The EFI_RNG_PROTOCOL can rely on 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,
> a GetRngGuid() function is added in a following patch.
> 
> Prepare GetRngGuid() return values and add GUIDs describing
> Rng algorithms:
> - gEfiRngAlgorithmArmRndr
> to describe a Rng algorithm accessed through Arm's RNDR instruction.
> [1] states that the implementation of this algorithm should be
> compliant to NIST SP900-80. The compliance is not guaranteed.
> - gEfiRngAlgorithmUnSafe
> to describe an unsafe implementation, cf. the BaseRngLibTimerLib.
> 
> [1] Arm Architecture Reference Manual Armv8, for A-profile architecture
> sK12.1 'Properties of the generated random number'
> 
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>  MdePkg/Include/Protocol/Rng.h | 20 ++++++++++++++++++++
>  MdePkg/MdePkg.dec             |  2 ++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/MdePkg/Include/Protocol/Rng.h
> b/MdePkg/Include/Protocol/Rng.h
> index baf425587b3c..dfdaf36e41dc 100644
> --- a/MdePkg/Include/Protocol/Rng.h
> +++ b/MdePkg/Include/Protocol/Rng.h
> @@ -67,6 +67,24 @@ typedef EFI_GUID EFI_RNG_ALGORITHM;
>    { \
>      0xe43176d7, 0xb6e8, 0x4827, {0xb7, 0x84, 0x7f, 0xfd, 0xc4, 0xb6, 0x85,
> 0x61 } \
>    }
> +///
> +/// The Arm Architecture states the RNDR that the DRBG algorithm should
> be compliant
> +/// with NIST SP800-90A, while not mandating a particular algorithm, so as
> to be
> +/// inclusive of different geographies.
> +///
> +#define EFI_RNG_ALGORITHM_ARM_RNDR \
> +  { \
> +    0x43d2fde3, 0x9d4e, 0x4d79,  {0x02, 0x96, 0xa8, 0x9b, 0xca, 0x78, 0x08,
> 0x41} \
> +  }
> +///
> +/// The implementation of a Random Number Generator might be unsafe,
> when using
> +/// a dummy implementation for instance. Allow identifying such
> implementation
> +/// with this GUID.
> +///
> +#define EFI_RNG_ALGORITHM_UNSAFE \
> +  { \
> +    0x869f728c, 0x409d, 0x4ab4, {0xac, 0x03, 0x71, 0xd3, 0x09, 0xc1, 0xb3,
> 0xf4 } \
> +  }
> 
>  /**
>    Returns information about the random number generation implementation.
> @@ -146,5 +164,7 @@ extern EFI_GUID
> gEfiRngAlgorithmSp80090Ctr256Guid;
>  extern EFI_GUID  gEfiRngAlgorithmX9313DesGuid;
>  extern EFI_GUID  gEfiRngAlgorithmX931AesGuid;
>  extern EFI_GUID  gEfiRngAlgorithmRaw;
> +extern EFI_GUID  gEfiRngAlgorithmArmRndr;
> +extern EFI_GUID  gEfiRngAlgorithmUnSafe;
> 
>  #endif
> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> index 0ecfad5795e4..754085eaa55b 100644
> --- a/MdePkg/MdePkg.dec
> +++ b/MdePkg/MdePkg.dec
> @@ -633,6 +633,8 @@ [Guids]
>    gEfiRngAlgorithmX9313DesGuid       = { 0x63c4785a, 0xca34, 0x4012, {0xa3,
> 0xc8, 0x0b, 0x6a, 0x32, 0x4f, 0x55, 0x46 }}
>    gEfiRngAlgorithmX931AesGuid        = { 0xacd03321, 0x777e, 0x4d3d, {0xb1,
> 0xc8, 0x20, 0xcf, 0xd8, 0x88, 0x20, 0xc9 }}
>    gEfiRngAlgorithmRaw                = { 0xe43176d7, 0xb6e8, 0x4827, {0xb7,
> 0x84, 0x7f, 0xfd, 0xc4, 0xb6, 0x85, 0x61 }}
> +  gEfiRngAlgorithmArmRndr            = { 0x43d2fde3, 0x9d4e, 0x4d79, {0x02,
> 0x96, 0xa8, 0x9b, 0xca, 0x78, 0x08, 0x41 }}
> +  gEfiRngAlgorithmUnSafe             = { 0x869f728c, 0x409d, 0x4ab4, {0xac,
> 0x03, 0x71, 0xd3, 0x09, 0xc1, 0xb3, 0xf4 }}
> 
>    ## Include/Protocol/AdapterInformation.h
>    gEfiAdapterInfoMediaStateGuid       = { 0xD7C74207, 0xA831, 0x4A26,
> {0xB1, 0xF5, 0xD1, 0x93, 0x06, 0x5C, 0xE8, 0xB6 }}
> --
> 2.25.1

Re: [PATCH v1 4/8] MdePkg/Rng: Add GUIDs to describe Rng algorithms

[Samer El-Haj-Mahmoud]

Hi Jiewen,

There is an open ECR for UEFI spec review: https://bugzilla.tianocore.org/show_bug.cgi?id=4441. These patches can wait on the list until the ECR is reviewed by UEFI Forum and the decision is documented in the BZ. If approved, then the code patches should be able to proceed.

Thanks,
--Samer



> -----Original Message-----
> From: Yao, Jiewen <jiewen.yao@intel.com>
> Sent: Tuesday, May 9, 2023 9:46 AM
> To: Pierre Gondois <Pierre.Gondois@arm.com>; devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; Wang,
> Jian J <jian.j.wang@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> Sami Mujawar <Sami.Mujawar@arm.com>; Jose Marinho
> <Jose.Marinho@arm.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> Mahmoud@arm.com>
> Subject: RE: [PATCH v1 4/8] MdePkg/Rng: Add GUIDs to describe Rng algorithms
>
> Is this defined in UEFI spec? or approved in future UEFI spec?
>
> > -----Original Message-----
> > From: pierre.gondois@arm.com <pierre.gondois@arm.com>
> > Sent: Tuesday, May 9, 2023 3:41 PM
> > To: devel@edk2.groups.io
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; Yao,
> > Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ard
> > Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar
> > <sami.mujawar@arm.com>; Jose Marinho <Jose.Marinho@arm.com>;
> > Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> > Subject: [PATCH v1 4/8] MdePkg/Rng: Add GUIDs to describe Rng algorithms
> >
> > From: Pierre Gondois <pierre.gondois@arm.com>
> >
> > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4441
> >
> > The EFI_RNG_PROTOCOL can rely on 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,
> > a GetRngGuid() function is added in a following patch.
> >
> > Prepare GetRngGuid() return values and add GUIDs describing
> > Rng algorithms:
> > - gEfiRngAlgorithmArmRndr
> > to describe a Rng algorithm accessed through Arm's RNDR instruction.
> > [1] states that the implementation of this algorithm should be
> > compliant to NIST SP900-80. The compliance is not guaranteed.
> > - gEfiRngAlgorithmUnSafe
> > to describe an unsafe implementation, cf. the BaseRngLibTimerLib.
> >
> > [1] Arm Architecture Reference Manual Armv8, for A-profile architecture
> > sK12.1 'Properties of the generated random number'
> >
> > Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> > ---
> >  MdePkg/Include/Protocol/Rng.h | 20 ++++++++++++++++++++
> >  MdePkg/MdePkg.dec             |  2 ++
> >  2 files changed, 22 insertions(+)
> >
> > diff --git a/MdePkg/Include/Protocol/Rng.h
> > b/MdePkg/Include/Protocol/Rng.h
> > index baf425587b3c..dfdaf36e41dc 100644
> > --- a/MdePkg/Include/Protocol/Rng.h
> > +++ b/MdePkg/Include/Protocol/Rng.h
> > @@ -67,6 +67,24 @@ typedef EFI_GUID EFI_RNG_ALGORITHM;
> >    { \
> >      0xe43176d7, 0xb6e8, 0x4827, {0xb7, 0x84, 0x7f, 0xfd, 0xc4, 0xb6, 0x85,
> > 0x61 } \
> >    }
> > +///
> > +/// The Arm Architecture states the RNDR that the DRBG algorithm should
> > be compliant
> > +/// with NIST SP800-90A, while not mandating a particular algorithm, so as
> > to be
> > +/// inclusive of different geographies.
> > +///
> > +#define EFI_RNG_ALGORITHM_ARM_RNDR \
> > +  { \
> > +    0x43d2fde3, 0x9d4e, 0x4d79,  {0x02, 0x96, 0xa8, 0x9b, 0xca, 0x78, 0x08,
> > 0x41} \
> > +  }
> > +///
> > +/// The implementation of a Random Number Generator might be unsafe,
> > when using
> > +/// a dummy implementation for instance. Allow identifying such
> > implementation
> > +/// with this GUID.
> > +///
> > +#define EFI_RNG_ALGORITHM_UNSAFE \
> > +  { \
> > +    0x869f728c, 0x409d, 0x4ab4, {0xac, 0x03, 0x71, 0xd3, 0x09, 0xc1, 0xb3,
> > 0xf4 } \
> > +  }
> >
> >  /**
> >    Returns information about the random number generation implementation.
> > @@ -146,5 +164,7 @@ extern EFI_GUID
> > gEfiRngAlgorithmSp80090Ctr256Guid;
> >  extern EFI_GUID  gEfiRngAlgorithmX9313DesGuid;
> >  extern EFI_GUID  gEfiRngAlgorithmX931AesGuid;
> >  extern EFI_GUID  gEfiRngAlgorithmRaw;
> > +extern EFI_GUID  gEfiRngAlgorithmArmRndr;
> > +extern EFI_GUID  gEfiRngAlgorithmUnSafe;
> >
> >  #endif
> > diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> > index 0ecfad5795e4..754085eaa55b 100644
> > --- a/MdePkg/MdePkg.dec
> > +++ b/MdePkg/MdePkg.dec
> > @@ -633,6 +633,8 @@ [Guids]
> >    gEfiRngAlgorithmX9313DesGuid       = { 0x63c4785a, 0xca34, 0x4012, {0xa3,
> > 0xc8, 0x0b, 0x6a, 0x32, 0x4f, 0x55, 0x46 }}
> >    gEfiRngAlgorithmX931AesGuid        = { 0xacd03321, 0x777e, 0x4d3d, {0xb1,
> > 0xc8, 0x20, 0xcf, 0xd8, 0x88, 0x20, 0xc9 }}
> >    gEfiRngAlgorithmRaw                = { 0xe43176d7, 0xb6e8, 0x4827, {0xb7,
> > 0x84, 0x7f, 0xfd, 0xc4, 0xb6, 0x85, 0x61 }}
> > +  gEfiRngAlgorithmArmRndr            = { 0x43d2fde3, 0x9d4e, 0x4d79, {0x02,
> > 0x96, 0xa8, 0x9b, 0xca, 0x78, 0x08, 0x41 }}
> > +  gEfiRngAlgorithmUnSafe             = { 0x869f728c, 0x409d, 0x4ab4, {0xac,
> > 0x03, 0x71, 0xd3, 0x09, 0xc1, 0xb3, 0xf4 }}
> >
> >    ## Include/Protocol/AdapterInformation.h
> >    gEfiAdapterInfoMediaStateGuid       = { 0xD7C74207, 0xA831, 0x4A26,
> > {0xB1, 0xF5, 0xD1, 0x93, 0x06, 0x5C, 0xE8, 0xB6 }}
> > --
> > 2.25.1

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH v1 4/8] MdePkg/Rng: Add GUIDs to describe Rng algorithms

[Yao, Jiewen]

Got it. Before that, I believe you can add the extension in MdeModulePkg or SecurityPkg.


> -----Original Message-----
> From: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> Sent: Tuesday, May 9, 2023 9:50 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; Pierre Gondois
> <Pierre.Gondois@arm.com>; devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>;
> Wang, Jian J <jian.j.wang@intel.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Sami Mujawar <Sami.Mujawar@arm.com>;
> Jose Marinho <Jose.Marinho@arm.com>; Samer El-Haj-Mahmoud
> <Samer.El-Haj-Mahmoud@arm.com>
> Subject: RE: [PATCH v1 4/8] MdePkg/Rng: Add GUIDs to describe Rng
> algorithms
> 
> Hi Jiewen,
> 
> There is an open ECR for UEFI spec review:
> https://bugzilla.tianocore.org/show_bug.cgi?id=4441. These patches can
> wait on the list until the ECR is reviewed by UEFI Forum and the decision is
> documented in the BZ. If approved, then the code patches should be able to
> proceed.
> 
> Thanks,
> --Samer
> 
> 
> 
> > -----Original Message-----
> > From: Yao, Jiewen <jiewen.yao@intel.com>
> > Sent: Tuesday, May 9, 2023 9:46 AM
> > To: Pierre Gondois <Pierre.Gondois@arm.com>; devel@edk2.groups.io
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>;
> Wang,
> > Jian J <jian.j.wang@intel.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>;
> > Sami Mujawar <Sami.Mujawar@arm.com>; Jose Marinho
> > <Jose.Marinho@arm.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> > Mahmoud@arm.com>
> > Subject: RE: [PATCH v1 4/8] MdePkg/Rng: Add GUIDs to describe Rng
> algorithms
> >
> > Is this defined in UEFI spec? or approved in future UEFI spec?
> >
> > > -----Original Message-----
> > > From: pierre.gondois@arm.com <pierre.gondois@arm.com>
> > > Sent: Tuesday, May 9, 2023 3:41 PM
> > > To: devel@edk2.groups.io
> > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > > <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>;
> Yao,
> > > Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> Ard
> > > Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar
> > > <sami.mujawar@arm.com>; Jose Marinho <Jose.Marinho@arm.com>;
> > > Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> > > Subject: [PATCH v1 4/8] MdePkg/Rng: Add GUIDs to describe Rng
> algorithms
> > >
> > > From: Pierre Gondois <pierre.gondois@arm.com>
> > >
> > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4441
> > >
> > > The EFI_RNG_PROTOCOL can rely on 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,
> > > a GetRngGuid() function is added in a following patch.
> > >
> > > Prepare GetRngGuid() return values and add GUIDs describing
> > > Rng algorithms:
> > > - gEfiRngAlgorithmArmRndr
> > > to describe a Rng algorithm accessed through Arm's RNDR instruction.
> > > [1] states that the implementation of this algorithm should be
> > > compliant to NIST SP900-80. The compliance is not guaranteed.
> > > - gEfiRngAlgorithmUnSafe
> > > to describe an unsafe implementation, cf. the BaseRngLibTimerLib.
> > >
> > > [1] Arm Architecture Reference Manual Armv8, for A-profile architecture
> > > sK12.1 'Properties of the generated random number'
> > >
> > > Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> > > ---
> > >  MdePkg/Include/Protocol/Rng.h | 20 ++++++++++++++++++++
> > >  MdePkg/MdePkg.dec             |  2 ++
> > >  2 files changed, 22 insertions(+)
> > >
> > > diff --git a/MdePkg/Include/Protocol/Rng.h
> > > b/MdePkg/Include/Protocol/Rng.h
> > > index baf425587b3c..dfdaf36e41dc 100644
> > > --- a/MdePkg/Include/Protocol/Rng.h
> > > +++ b/MdePkg/Include/Protocol/Rng.h
> > > @@ -67,6 +67,24 @@ typedef EFI_GUID EFI_RNG_ALGORITHM;
> > >    { \
> > >      0xe43176d7, 0xb6e8, 0x4827, {0xb7, 0x84, 0x7f, 0xfd, 0xc4, 0xb6,
> 0x85,
> > > 0x61 } \
> > >    }
> > > +///
> > > +/// The Arm Architecture states the RNDR that the DRBG algorithm
> should
> > > be compliant
> > > +/// with NIST SP800-90A, while not mandating a particular algorithm, so
> as
> > > to be
> > > +/// inclusive of different geographies.
> > > +///
> > > +#define EFI_RNG_ALGORITHM_ARM_RNDR \
> > > +  { \
> > > +    0x43d2fde3, 0x9d4e, 0x4d79,  {0x02, 0x96, 0xa8, 0x9b, 0xca, 0x78,
> 0x08,
> > > 0x41} \
> > > +  }
> > > +///
> > > +/// The implementation of a Random Number Generator might be
> unsafe,
> > > when using
> > > +/// a dummy implementation for instance. Allow identifying such
> > > implementation
> > > +/// with this GUID.
> > > +///
> > > +#define EFI_RNG_ALGORITHM_UNSAFE \
> > > +  { \
> > > +    0x869f728c, 0x409d, 0x4ab4, {0xac, 0x03, 0x71, 0xd3, 0x09, 0xc1,
> 0xb3,
> > > 0xf4 } \
> > > +  }
> > >
> > >  /**
> > >    Returns information about the random number generation
> implementation.
> > > @@ -146,5 +164,7 @@ extern EFI_GUID
> > > gEfiRngAlgorithmSp80090Ctr256Guid;
> > >  extern EFI_GUID  gEfiRngAlgorithmX9313DesGuid;
> > >  extern EFI_GUID  gEfiRngAlgorithmX931AesGuid;
> > >  extern EFI_GUID  gEfiRngAlgorithmRaw;
> > > +extern EFI_GUID  gEfiRngAlgorithmArmRndr;
> > > +extern EFI_GUID  gEfiRngAlgorithmUnSafe;
> > >
> > >  #endif
> > > diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> > > index 0ecfad5795e4..754085eaa55b 100644
> > > --- a/MdePkg/MdePkg.dec
> > > +++ b/MdePkg/MdePkg.dec
> > > @@ -633,6 +633,8 @@ [Guids]
> > >    gEfiRngAlgorithmX9313DesGuid       = { 0x63c4785a, 0xca34, 0x4012,
> {0xa3,
> > > 0xc8, 0x0b, 0x6a, 0x32, 0x4f, 0x55, 0x46 }}
> > >    gEfiRngAlgorithmX931AesGuid        = { 0xacd03321, 0x777e, 0x4d3d,
> {0xb1,
> > > 0xc8, 0x20, 0xcf, 0xd8, 0x88, 0x20, 0xc9 }}
> > >    gEfiRngAlgorithmRaw                = { 0xe43176d7, 0xb6e8, 0x4827, {0xb7,
> > > 0x84, 0x7f, 0xfd, 0xc4, 0xb6, 0x85, 0x61 }}
> > > +  gEfiRngAlgorithmArmRndr            = { 0x43d2fde3, 0x9d4e, 0x4d79,
> {0x02,
> > > 0x96, 0xa8, 0x9b, 0xca, 0x78, 0x08, 0x41 }}
> > > +  gEfiRngAlgorithmUnSafe             = { 0x869f728c, 0x409d, 0x4ab4, {0xac,
> > > 0x03, 0x71, 0xd3, 0x09, 0xc1, 0xb3, 0xf4 }}
> > >
> > >    ## Include/Protocol/AdapterInformation.h
> > >    gEfiAdapterInfoMediaStateGuid       = { 0xD7C74207, 0xA831, 0x4A26,
> > > {0xB1, 0xF5, 0xD1, 0x93, 0x06, 0x5C, 0xE8, 0xB6 }}
> > > --
> > > 2.25.1
> 
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended recipient,
> please notify the sender immediately and do not disclose the contents to any
> other person, use it for any purpose, or store or copy the information in any
> medium. Thank you.

Re: [PATCH v1 4/8] MdePkg/Rng: Add GUIDs to describe Rng algorithms

[PierreGondois]

Hello,
The mantis ticket (created by Jose Marinho) is available at:
https://mantis.uefi.org/mantis/view.php?id=2386

Also, are there any comments on the patch-set ?

Regards,
Pierre

On 5/9/23 15:50, Samer El-Haj-Mahmoud wrote:
> Hi Jiewen,
> 
> There is an open ECR for UEFI spec review: https://bugzilla.tianocore.org/show_bug.cgi?id=4441. These patches can wait on the list until the ECR is reviewed by UEFI Forum and the decision is documented in the BZ. If approved, then the code patches should be able to proceed.
> 
> Thanks,
> --Samer
> 
> 
> 
>> -----Original Message-----
>> From: Yao, Jiewen <jiewen.yao@intel.com>
>> Sent: Tuesday, May 9, 2023 9:46 AM
>> To: Pierre Gondois <Pierre.Gondois@arm.com>; devel@edk2.groups.io
>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
>> <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; Wang,
>> Jian J <jian.j.wang@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
>> Sami Mujawar <Sami.Mujawar@arm.com>; Jose Marinho
>> <Jose.Marinho@arm.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
>> Mahmoud@arm.com>
>> Subject: RE: [PATCH v1 4/8] MdePkg/Rng: Add GUIDs to describe Rng algorithms
>>
>> Is this defined in UEFI spec? or approved in future UEFI spec?
>>
>>> -----Original Message-----
>>> From: pierre.gondois@arm.com <pierre.gondois@arm.com>
>>> Sent: Tuesday, May 9, 2023 3:41 PM
>>> To: devel@edk2.groups.io
>>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
>>> <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; Yao,
>>> Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ard
>>> Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar
>>> <sami.mujawar@arm.com>; Jose Marinho <Jose.Marinho@arm.com>;
>>> Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
>>> Subject: [PATCH v1 4/8] MdePkg/Rng: Add GUIDs to describe Rng algorithms
>>>
>>> From: Pierre Gondois <pierre.gondois@arm.com>
>>>
>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4441
>>>
>>> The EFI_RNG_PROTOCOL can rely on 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,
>>> a GetRngGuid() function is added in a following patch.
>>>
>>> Prepare GetRngGuid() return values and add GUIDs describing
>>> Rng algorithms:
>>> - gEfiRngAlgorithmArmRndr
>>> to describe a Rng algorithm accessed through Arm's RNDR instruction.
>>> [1] states that the implementation of this algorithm should be
>>> compliant to NIST SP900-80. The compliance is not guaranteed.
>>> - gEfiRngAlgorithmUnSafe
>>> to describe an unsafe implementation, cf. the BaseRngLibTimerLib.
>>>
>>> [1] Arm Architecture Reference Manual Armv8, for A-profile architecture
>>> sK12.1 'Properties of the generated random number'
>>>
>>> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
>>> ---
>>>   MdePkg/Include/Protocol/Rng.h | 20 ++++++++++++++++++++
>>>   MdePkg/MdePkg.dec             |  2 ++
>>>   2 files changed, 22 insertions(+)
>>>
>>> diff --git a/MdePkg/Include/Protocol/Rng.h
>>> b/MdePkg/Include/Protocol/Rng.h
>>> index baf425587b3c..dfdaf36e41dc 100644
>>> --- a/MdePkg/Include/Protocol/Rng.h
>>> +++ b/MdePkg/Include/Protocol/Rng.h
>>> @@ -67,6 +67,24 @@ typedef EFI_GUID EFI_RNG_ALGORITHM;
>>>     { \
>>>       0xe43176d7, 0xb6e8, 0x4827, {0xb7, 0x84, 0x7f, 0xfd, 0xc4, 0xb6, 0x85,
>>> 0x61 } \
>>>     }
>>> +///
>>> +/// The Arm Architecture states the RNDR that the DRBG algorithm should
>>> be compliant
>>> +/// with NIST SP800-90A, while not mandating a particular algorithm, so as
>>> to be
>>> +/// inclusive of different geographies.
>>> +///
>>> +#define EFI_RNG_ALGORITHM_ARM_RNDR \
>>> +  { \
>>> +    0x43d2fde3, 0x9d4e, 0x4d79,  {0x02, 0x96, 0xa8, 0x9b, 0xca, 0x78, 0x08,
>>> 0x41} \
>>> +  }
>>> +///
>>> +/// The implementation of a Random Number Generator might be unsafe,
>>> when using
>>> +/// a dummy implementation for instance. Allow identifying such
>>> implementation
>>> +/// with this GUID.
>>> +///
>>> +#define EFI_RNG_ALGORITHM_UNSAFE \
>>> +  { \
>>> +    0x869f728c, 0x409d, 0x4ab4, {0xac, 0x03, 0x71, 0xd3, 0x09, 0xc1, 0xb3,
>>> 0xf4 } \
>>> +  }
>>>
>>>   /**
>>>     Returns information about the random number generation implementation.
>>> @@ -146,5 +164,7 @@ extern EFI_GUID
>>> gEfiRngAlgorithmSp80090Ctr256Guid;
>>>   extern EFI_GUID  gEfiRngAlgorithmX9313DesGuid;
>>>   extern EFI_GUID  gEfiRngAlgorithmX931AesGuid;
>>>   extern EFI_GUID  gEfiRngAlgorithmRaw;
>>> +extern EFI_GUID  gEfiRngAlgorithmArmRndr;
>>> +extern EFI_GUID  gEfiRngAlgorithmUnSafe;
>>>
>>>   #endif
>>> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
>>> index 0ecfad5795e4..754085eaa55b 100644
>>> --- a/MdePkg/MdePkg.dec
>>> +++ b/MdePkg/MdePkg.dec
>>> @@ -633,6 +633,8 @@ [Guids]
>>>     gEfiRngAlgorithmX9313DesGuid       = { 0x63c4785a, 0xca34, 0x4012, {0xa3,
>>> 0xc8, 0x0b, 0x6a, 0x32, 0x4f, 0x55, 0x46 }}
>>>     gEfiRngAlgorithmX931AesGuid        = { 0xacd03321, 0x777e, 0x4d3d, {0xb1,
>>> 0xc8, 0x20, 0xcf, 0xd8, 0x88, 0x20, 0xc9 }}
>>>     gEfiRngAlgorithmRaw                = { 0xe43176d7, 0xb6e8, 0x4827, {0xb7,
>>> 0x84, 0x7f, 0xfd, 0xc4, 0xb6, 0x85, 0x61 }}
>>> +  gEfiRngAlgorithmArmRndr            = { 0x43d2fde3, 0x9d4e, 0x4d79, {0x02,
>>> 0x96, 0xa8, 0x9b, 0xca, 0x78, 0x08, 0x41 }}
>>> +  gEfiRngAlgorithmUnSafe             = { 0x869f728c, 0x409d, 0x4ab4, {0xac,
>>> 0x03, 0x71, 0xd3, 0x09, 0xc1, 0xb3, 0xf4 }}
>>>
>>>     ## Include/Protocol/AdapterInformation.h
>>>     gEfiAdapterInfoMediaStateGuid       = { 0xD7C74207, 0xA831, 0x4A26,
>>> {0xB1, 0xF5, 0xD1, 0x93, 0x06, 0x5C, 0xE8, 0xB6 }}
>>> --
>>> 2.25.1
> 

Re: [PATCH v1 4/8] MdePkg/Rng: Add GUIDs to describe Rng algorithms

[Sami Mujawar]

Hi Pierre,

Thank you for this patch.

Please find my response inline marked [SAMI].

Other than the concern mentioned below, this patch looks good to me.

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>
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4441
>
> The EFI_RNG_PROTOCOL can rely on 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,
> a GetRngGuid() function is added in a following patch.
>
> Prepare GetRngGuid() return values and add GUIDs describing
> Rng algorithms:
> - gEfiRngAlgorithmArmRndr
> to describe a Rng algorithm accessed through Arm's RNDR instruction.
> [1] states that the implementation of this algorithm should be
> compliant to NIST SP900-80. The compliance is not guaranteed.
> - gEfiRngAlgorithmUnSafe
> to describe an unsafe implementation, cf. the BaseRngLibTimerLib.
>
> [1] Arm Architecture Reference Manual Armv8, for A-profile architecture
> sK12.1 'Properties of the generated random number'
>
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>   MdePkg/Include/Protocol/Rng.h | 20 ++++++++++++++++++++
>   MdePkg/MdePkg.dec             |  2 ++
>   2 files changed, 22 insertions(+)
>
> diff --git a/MdePkg/Include/Protocol/Rng.h b/MdePkg/Include/Protocol/Rng.h
> index baf425587b3c..dfdaf36e41dc 100644
> --- a/MdePkg/Include/Protocol/Rng.h
> +++ b/MdePkg/Include/Protocol/Rng.h
> @@ -67,6 +67,24 @@ typedef EFI_GUID EFI_RNG_ALGORITHM;
>     { \
>       0xe43176d7, 0xb6e8, 0x4827, {0xb7, 0x84, 0x7f, 0xfd, 0xc4, 0xb6, 0x85, 0x61 } \
>     }
> +///
> +/// The Arm Architecture states the RNDR that the DRBG algorithm should be compliant
> +/// with NIST SP800-90A, while not mandating a particular algorithm, so as to be
> +/// inclusive of different geographies.
> +///
> +#define EFI_RNG_ALGORITHM_ARM_RNDR \
> +  { \
> +    0x43d2fde3, 0x9d4e, 0x4d79,  {0x02, 0x96, 0xa8, 0x9b, 0xca, 0x78, 0x08, 0x41} \
> +  }
> +///
> +/// The implementation of a Random Number Generator might be unsafe, when using
> +/// a dummy implementation for instance. Allow identifying such implementation
> +/// with this GUID.
> +///
> +#define EFI_RNG_ALGORITHM_UNSAFE \
> +  { \
> +    0x869f728c, 0x409d, 0x4ab4, {0xac, 0x03, 0x71, 0xd3, 0x09, 0xc1, 0xb3, 0xf4 } \
> +  }

[SAMI] Unlike the EFI_RNG_ALGORITHM_ARM_RNDR which is backed by the code 
first spec update at https://mantis.uefi.org/mantis/view.php?id=2386; 
the EFI_RNG_ALGORITHM_UNSAFE is not backed by any specification.

Although I agree that a definition of the unsafe algorithm is required 
to support some platforms, I am not sure if this file and the macro 
prefix is right for this definition.

I would defer this decision, and any advice on how to proceed to the 
MdePkg maintainers.

[/SAMI]

>   
>   /**
>     Returns information about the random number generation implementation.
> @@ -146,5 +164,7 @@ extern EFI_GUID  gEfiRngAlgorithmSp80090Ctr256Guid;
>   extern EFI_GUID  gEfiRngAlgorithmX9313DesGuid;
>   extern EFI_GUID  gEfiRngAlgorithmX931AesGuid;
>   extern EFI_GUID  gEfiRngAlgorithmRaw;
> +extern EFI_GUID  gEfiRngAlgorithmArmRndr;
> +extern EFI_GUID  gEfiRngAlgorithmUnSafe;
>   
>   #endif
> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> index 0ecfad5795e4..754085eaa55b 100644
> --- a/MdePkg/MdePkg.dec
> +++ b/MdePkg/MdePkg.dec
> @@ -633,6 +633,8 @@ [Guids]
>     gEfiRngAlgorithmX9313DesGuid       = { 0x63c4785a, 0xca34, 0x4012, {0xa3, 0xc8, 0x0b, 0x6a, 0x32, 0x4f, 0x55, 0x46 }}
>     gEfiRngAlgorithmX931AesGuid        = { 0xacd03321, 0x777e, 0x4d3d, {0xb1, 0xc8, 0x20, 0xcf, 0xd8, 0x88, 0x20, 0xc9 }}
>     gEfiRngAlgorithmRaw                = { 0xe43176d7, 0xb6e8, 0x4827, {0xb7, 0x84, 0x7f, 0xfd, 0xc4, 0xb6, 0x85, 0x61 }}
> +  gEfiRngAlgorithmArmRndr            = { 0x43d2fde3, 0x9d4e, 0x4d79, {0x02, 0x96, 0xa8, 0x9b, 0xca, 0x78, 0x08, 0x41 }}
> +  gEfiRngAlgorithmUnSafe             = { 0x869f728c, 0x409d, 0x4ab4, {0xac, 0x03, 0x71, 0xd3, 0x09, 0xc1, 0xb3, 0xf4 }}
>   
>     ## Include/Protocol/AdapterInformation.h
>     gEfiAdapterInfoMediaStateGuid       = { 0xD7C74207, 0xA831, 0x4A26, {0xB1, 0xF5, 0xD1, 0x93, 0x06, 0x5C, 0xE8, 0xB6 }}

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

[PierreGondois]

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
+
 [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;
+}
-- 
2.25.1

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

[Sami Mujawar]

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;
> +}

[PATCH v1 6/8] SecurityPkg/RngDxe: Use GetRngGuid() when probing RngLib

[PierreGondois]

From: Pierre Gondois <pierre.gondois@arm.com>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4151

The EFI_RNG_PROTOCOL can rely on 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,
a GetRngGuid() function was added in a previous patch.

The EFI_RNG_PROTOCOL can advertise multiple algorithms through
Guids. The PcdCpuRngSupportedAlgorithm is currently used to
advertise the RngLib in the Arm implementation.

The issues of doing that are:
- the RngLib implementation might not use CPU instructions,
  cf. the BaseRngLibTimerLib
- most platforms don't set PcdCpuRngSupportedAlgorithm

A GetRngGuid() was added to the RngLib in a previous patch,
allowing to identify the algorithm implemented by the RngLib.
Make use of this function.

Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
---
 .../RngDxe/AArch64/AArch64Algo.c              | 24 +++++++++----------
 .../RandomNumberGenerator/RngDxe/ArmRngDxe.c  |  6 ++++-
 .../RandomNumberGenerator/RngDxe/RngDxe.inf   |  5 ++--
 3 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c b/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
index e8be217f8a8c..a1ff7bd58fda 100644
--- a/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
+++ b/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
@@ -10,6 +10,7 @@
 #include <Library/DebugLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/ArmTrngLib.h>
+#include <Library/RngLib.h>
 
 #include "RngDxeInternals.h"
 
@@ -28,9 +29,10 @@ GetAvailableAlgorithms (
   VOID
   )
 {
-  UINT64  DummyRand;
-  UINT16  MajorRevision;
-  UINT16  MinorRevision;
+  EFI_STATUS  Status;
+  UINT16      MajorRevision;
+  UINT16      MinorRevision;
+  GUID        RngGuid;
 
   // Rng algorithms 2 times, one for the allocation, one to populate.
   mAvailableAlgoArray = AllocateZeroPool (RNG_AVAILABLE_ALGO_MAX);
@@ -38,24 +40,22 @@ GetAvailableAlgorithms (
     return EFI_OUT_OF_RESOURCES;
   }
 
-  // Check RngGetBytes() before advertising PcdCpuRngSupportedAlgorithm.
-  if (!EFI_ERROR (RngGetBytes (sizeof (DummyRand), (UINT8 *)&DummyRand))) {
+  // Identify RngLib algorithm.
+  Status = GetRngGuid (&RngGuid);
+  if (!EFI_ERROR (Status)) {
     CopyMem (
       &mAvailableAlgoArray[mAvailableAlgoArrayCount],
-      PcdGetPtr (PcdCpuRngSupportedAlgorithm),
-      sizeof (EFI_RNG_ALGORITHM)
+      RngGuid,
+      sizeof (RngGuid)
       );
     mAvailableAlgoArrayCount++;
 
-    DEBUG_CODE_BEGIN ();
-    if (IsZeroGuid (PcdGetPtr (PcdCpuRngSupportedAlgorithm))) {
+    if (IsZeroGuid (&RngGuid)) {
       DEBUG ((
         DEBUG_WARN,
-        "PcdCpuRngSupportedAlgorithm should be a non-zero GUID\n"
+        "RngLib should have a non-zero GUID\n"
         ));
     }
-
-    DEBUG_CODE_END ();
   }
 
   // Raw algorithm (Trng)
diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c b/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
index ce49ff7ae661..78a18c5e1177 100644
--- a/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
+++ b/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
@@ -78,6 +78,7 @@ RngGetRNG (
 {
   EFI_STATUS  Status;
   UINTN       Index;
+  GUID        RngGuid;
 
   if ((This == NULL) || (RNGValueLength == 0) || (RNGValue == NULL)) {
     return EFI_INVALID_PARAMETER;
@@ -102,7 +103,10 @@ RngGetRNG (
   }
 
 FoundAlgo:
-  if (CompareGuid (RNGAlgorithm, PcdGetPtr (PcdCpuRngSupportedAlgorithm))) {
+  Status = GetRngGuid (&RngGuid);
+  if (!EFI_ERROR (Status) &&
+      CompareGuid (RNGAlgorithm, &RngGuid))
+  {
     Status = RngGetBytes (RNGValueLength, RNGValue);
     return Status;
   }
diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf b/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
index d6c2d30195bf..aa5743387ed9 100644
--- a/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
+++ b/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
@@ -75,13 +75,12 @@ [Guids]
   gEfiRngAlgorithmX9313DesGuid        ## SOMETIMES_PRODUCES    ## GUID        # Unique ID of the algorithm for RNG
   gEfiRngAlgorithmX931AesGuid         ## SOMETIMES_PRODUCES    ## GUID        # Unique ID of the algorithm for RNG
   gEfiRngAlgorithmRaw                 ## SOMETIMES_PRODUCES    ## GUID        # Unique ID of the algorithm for RNG
+  gEfiRngAlgorithmArmRndr             ## SOMETIMES_PRODUCES    ## GUID        # Unique ID of the algorithm for RNG
+  gEfiRngAlgorithmUnSafe              ## SOMETIMES_PRODUCES    ## GUID        # Unique ID of the algorithm for RNG
 
 [Protocols]
   gEfiRngProtocolGuid                ## PRODUCES
 
-[Pcd.AARCH64]
-  gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm      ## CONSUMES
-
 [Depex]
   TRUE
 
-- 
2.25.1

Re: [PATCH v1 6/8] SecurityPkg/RngDxe: Use GetRngGuid() when probing RngLib

[Sami Mujawar]

[-- Attachment #1: Type: text/plain, Size: 7116 bytes --]

Hi Pierre,

Thank you for this patch.

I think we should treat algorithms that have a zero GUID as unsafe. I 
have made some suggestions on those lines marked inline as [SAMI].

Regards,

Sami Mujawar

On 09/05/2023 08:40 am, pierre.gondois@arm.com wrote:
> From: Pierre Gondois<pierre.gondois@arm.com>
>
> BZ:https://bugzilla.tianocore.org/show_bug.cgi?id=4151
>
> The EFI_RNG_PROTOCOL can rely on 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,
> a GetRngGuid() function was added in a previous patch.
>
> The EFI_RNG_PROTOCOL can advertise multiple algorithms through
> Guids. The PcdCpuRngSupportedAlgorithm is currently used to
> advertise the RngLib in the Arm implementation.
>
> The issues of doing that are:
> - the RngLib implementation might not use CPU instructions,
>    cf. the BaseRngLibTimerLib
> - most platforms don't set PcdCpuRngSupportedAlgorithm
>
> A GetRngGuid() was added to the RngLib in a previous patch,
> allowing to identify the algorithm implemented by the RngLib.
> Make use of this function.
>
> Signed-off-by: Pierre Gondois<pierre.gondois@arm.com>
> ---
>   .../RngDxe/AArch64/AArch64Algo.c              | 24 +++++++++----------
>   .../RandomNumberGenerator/RngDxe/ArmRngDxe.c  |  6 ++++-
>   .../RandomNumberGenerator/RngDxe/RngDxe.inf   |  5 ++--
>   3 files changed, 19 insertions(+), 16 deletions(-)
>
> diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c b/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
> index e8be217f8a8c..a1ff7bd58fda 100644
> --- a/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
> +++ b/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
> @@ -10,6 +10,7 @@
>   #include <Library/DebugLib.h>
>   #include <Library/MemoryAllocationLib.h>
>   #include <Library/ArmTrngLib.h>
> +#include <Library/RngLib.h>
>   
>   #include "RngDxeInternals.h"
>   
> @@ -28,9 +29,10 @@ GetAvailableAlgorithms (
>     VOID
>     )
>   {
> -  UINT64  DummyRand;
> -  UINT16  MajorRevision;
> -  UINT16  MinorRevision;
> +  EFI_STATUS  Status;
> +  UINT16      MajorRevision;
> +  UINT16      MinorRevision;
> +  GUID        RngGuid;
>   
>     // Rng algorithms 2 times, one for the allocation, one to populate.
>     mAvailableAlgoArray = AllocateZeroPool (RNG_AVAILABLE_ALGO_MAX);
> @@ -38,24 +40,22 @@ GetAvailableAlgorithms (
>       return EFI_OUT_OF_RESOURCES;
>     }
>   
> -  // Check RngGetBytes() before advertising PcdCpuRngSupportedAlgorithm.
> -  if (!EFI_ERROR (RngGetBytes (sizeof (DummyRand), (UINT8 *)&DummyRand))) {
<SNIP>
> +  // Identify RngLib algorithm.
> +  Status = GetRngGuid (&RngGuid);
> +  if (!EFI_ERROR (Status)) {
>       CopyMem (
>         &mAvailableAlgoArray[mAvailableAlgoArrayCount],
> -      PcdGetPtr (PcdCpuRngSupportedAlgorithm),
> -      sizeof (EFI_RNG_ALGORITHM)
> +      RngGuid,
> +      sizeof (RngGuid)
>         );
>       mAvailableAlgoArrayCount++;
>   
> -    DEBUG_CODE_BEGIN ();
> -    if (IsZeroGuid (PcdGetPtr (PcdCpuRngSupportedAlgorithm))) {
> +    if (IsZeroGuid (&RngGuid)) {
>         DEBUG ((
>           DEBUG_WARN,
> -        "PcdCpuRngSupportedAlgorithm should be a non-zero GUID\n"
> +        "RngLib should have a non-zero GUID\n"
>           ));
>       }
> -
> -    DEBUG_CODE_END ();
>     }
>   
>     // Raw algorithm (Trng)

...

</SNIP>

[SAMI] I think this code should do the following:

---

   BOOLEAN UnSafeAlgo = FALSE;
   mAvailableAlgoArrayCount = 0;
// Identify RngLib algorithm.
   Status = GetRngGuid (&RngGuid);
if(!EFI_ERROR (Status)) {
if((IsZeroGuid (&RngGuid)) ||
         (CompareGuid (&RngGuid, &gEfiRngAlgorithmUnSafe))) {
// Treat zero GUID as an unsafe algorithm
       DEBUG ((
         DEBUG_WARN,
"RngLib uses an Unsafe algorithm and "
"must not be used for production builds.\n"
         ));
// Set the UnSafeAlgo flag to indicate an unsafe algorithm was found
// so that it can be added at the end of the algorithm list.
       UnSafeAlgo = TRUE;
     } else{
       CopyMem (
         &mAvailableAlgoArray[mAvailableAlgoArrayCount],
         &RngGuid,
sizeof(RngGuid)
         );
       mAvailableAlgoArrayCount++;
     }
   }
// Raw algorithm (Trng)
if(!EFI_ERROR (GetArmTrngVersion (&MajorRevision, &MinorRevision))) {
     CopyMem (
       &mAvailableAlgoArray[mAvailableAlgoArrayCount],
       &gEfiRngAlgorithmRaw,
sizeof(EFI_RNG_ALGORITHM)
       );
     mAvailableAlgoArrayCount++;
   }
// Add unsafe algorithms at the end of the list.
if(UnSafeAlgo) {
     CopyMem (
       &mAvailableAlgoArray[mAvailableAlgoArrayCount],
       &gEfiRngAlgorithmUnSafe,
sizeof(EFI_RNG_ALGORITHM)
       );
     mAvailableAlgoArrayCount++;
   }
---

Doing this will not need the call to RngFindDefaultAlgo () introduced in 
the next patch "[PATCH v1 7/8] SecurityPkg/RngDxe: Select safe default 
Rng algorithm", which can be dropped.

[SAMI]

> diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c b/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
> index ce49ff7ae661..78a18c5e1177 100644
> --- a/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
> +++ b/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
> @@ -78,6 +78,7 @@ RngGetRNG (
>   {
>     EFI_STATUS  Status;
>     UINTN       Index;
> +  GUID        RngGuid;
>   
>     if ((This == NULL) || (RNGValueLength == 0) || (RNGValue == NULL)) {
>       return EFI_INVALID_PARAMETER;
> @@ -102,7 +103,10 @@ RngGetRNG (
>     }
>   
>   FoundAlgo:
> -  if (CompareGuid (RNGAlgorithm, PcdGetPtr (PcdCpuRngSupportedAlgorithm))) {
> +  Status = GetRngGuid (&RngGuid);
> +  if (!EFI_ERROR (Status) &&
> +      CompareGuid (RNGAlgorithm, &RngGuid))
> +  {
>       Status = RngGetBytes (RNGValueLength, RNGValue);
>       return Status;
>     }
> diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf b/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
> index d6c2d30195bf..aa5743387ed9 100644
> --- a/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
> +++ b/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
> @@ -75,13 +75,12 @@ [Guids]
>     gEfiRngAlgorithmX9313DesGuid        ## SOMETIMES_PRODUCES    ## GUID        # Unique ID of the algorithm for RNG
>     gEfiRngAlgorithmX931AesGuid         ## SOMETIMES_PRODUCES    ## GUID        # Unique ID of the algorithm for RNG
>     gEfiRngAlgorithmRaw                 ## SOMETIMES_PRODUCES    ## GUID        # Unique ID of the algorithm for RNG
> +  gEfiRngAlgorithmArmRndr             ## SOMETIMES_PRODUCES    ## GUID        # Unique ID of the algorithm for RNG
[SAMI] Can the [Guids] section be split to relect the usage across 
architectures, please?
> +  gEfiRngAlgorithmUnSafe              ## SOMETIMES_PRODUCES    ## GUID        # Unique ID of the algorithm for RNG
>   
>   [Protocols]
>     gEfiRngProtocolGuid                ## PRODUCES
>   
> -[Pcd.AARCH64]
> -  gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm      ## CONSUMES
> -
>   [Depex]
>     TRUE
>   

[-- Attachment #2: Type: text/html, Size: 12693 bytes --]

[PATCH v1 7/8] SecurityPkg/RngDxe: Select safe default Rng algorithm

[PierreGondois]

From: Pierre Gondois <pierre.gondois@arm.com>

The first element of mAvailableAlgoArray should be the default
algorithm to avoid going through a selection process at each
RngGetRNG() call.
Once all the available Rng algorithms have been probed, place
a safe Rng algorithm at the first position of mAvailableAlgoArray.

Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
---
 .../RngDxe/AArch64/AArch64Algo.c              | 48 ++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c b/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
index a1ff7bd58fda..ed236b2e8141 100644
--- a/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
+++ b/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
@@ -17,6 +17,50 @@
 // Maximum number of Rng algorithms.
 #define RNG_AVAILABLE_ALGO_MAX  2
 
+/** mAvailableAlgoArray[0] should contain the default Rng algorithm.
+    The Rng algorithm at the first index might be unsafe.
+    If a safe algorithm is available, choose it as the default one.
+**/
+VOID
+EFIAPI
+RngFindDefaultAlgo (
+  VOID
+  )
+{
+  EFI_RNG_ALGORITHM  *CurAlgo;
+  EFI_RNG_ALGORITHM  TmpGuid;
+  UINTN              Index;
+
+  CurAlgo = &mAvailableAlgoArray[0];
+
+  if (IsZeroGuid (CurAlgo) ||
+      !CompareGuid (CurAlgo, &gEfiRngAlgorithmUnSafe))
+  {
+    // mAvailableAlgoArray[0] is a valid Rng algorithm.
+    return;
+  }
+
+  for (Index = 1; Index < mAvailableAlgoArrayCount; Index++) {
+    CurAlgo = &mAvailableAlgoArray[Index];
+    if (!IsZeroGuid (CurAlgo) ||
+        CompareGuid (CurAlgo, &gEfiRngAlgorithmUnSafe))
+    {
+      break;
+    }
+  }
+
+  if (Index == mAvailableAlgoArrayCount) {
+    // No valid Rng algorithm available.
+    return;
+  }
+
+  CopyMem (&TmpGuid, CurAlgo, sizeof (EFI_RNG_ALGORITHM));
+  CopyMem (CurAlgo, &mAvailableAlgoArray[0], sizeof (EFI_RNG_ALGORITHM));
+  CopyMem (&mAvailableAlgoArray[0], &TmpGuid, sizeof (EFI_RNG_ALGORITHM));
+
+  return;
+}
+
 /** Allocate and initialize mAvailableAlgoArray with the available
     Rng algorithms. Also update mAvailableAlgoArrayCount.
 
@@ -45,7 +89,7 @@ GetAvailableAlgorithms (
   if (!EFI_ERROR (Status)) {
     CopyMem (
       &mAvailableAlgoArray[mAvailableAlgoArrayCount],
-      RngGuid,
+      &RngGuid,
       sizeof (RngGuid)
       );
     mAvailableAlgoArrayCount++;
@@ -68,5 +112,7 @@ GetAvailableAlgorithms (
     mAvailableAlgoArrayCount++;
   }
 
+  RngFindDefaultAlgo ();
+
   return EFI_SUCCESS;
 }
-- 
2.25.1

Re: [PATCH v1 7/8] SecurityPkg/RngDxe: Select safe default Rng algorithm

[Sami Mujawar]

Hi Pierre,

I think this patch would not be required if my suggestions for patch 6/8 
are adopted.

Regards,

Sami Mujawar

On 09/05/2023 08:40 am, pierre.gondois@arm.com wrote:
> From: Pierre Gondois <pierre.gondois@arm.com>
>
> The first element of mAvailableAlgoArray should be the default
> algorithm to avoid going through a selection process at each
> RngGetRNG() call.
> Once all the available Rng algorithms have been probed, place
> a safe Rng algorithm at the first position of mAvailableAlgoArray.
>
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>   .../RngDxe/AArch64/AArch64Algo.c              | 48 ++++++++++++++++++-
>   1 file changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c b/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
> index a1ff7bd58fda..ed236b2e8141 100644
> --- a/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
> +++ b/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
> @@ -17,6 +17,50 @@
>   // Maximum number of Rng algorithms.
>   #define RNG_AVAILABLE_ALGO_MAX  2
>   
> +/** mAvailableAlgoArray[0] should contain the default Rng algorithm.
> +    The Rng algorithm at the first index might be unsafe.
> +    If a safe algorithm is available, choose it as the default one.
> +**/
> +VOID
> +EFIAPI
> +RngFindDefaultAlgo (
> +  VOID
> +  )
> +{
> +  EFI_RNG_ALGORITHM  *CurAlgo;
> +  EFI_RNG_ALGORITHM  TmpGuid;
> +  UINTN              Index;
> +
> +  CurAlgo = &mAvailableAlgoArray[0];
> +
> +  if (IsZeroGuid (CurAlgo) ||
> +      !CompareGuid (CurAlgo, &gEfiRngAlgorithmUnSafe))
> +  {
> +    // mAvailableAlgoArray[0] is a valid Rng algorithm.
> +    return;
> +  }
> +
> +  for (Index = 1; Index < mAvailableAlgoArrayCount; Index++) {
> +    CurAlgo = &mAvailableAlgoArray[Index];
> +    if (!IsZeroGuid (CurAlgo) ||
> +        CompareGuid (CurAlgo, &gEfiRngAlgorithmUnSafe))
> +    {
> +      break;
> +    }
> +  }
> +
> +  if (Index == mAvailableAlgoArrayCount) {
> +    // No valid Rng algorithm available.
> +    return;
> +  }
> +
> +  CopyMem (&TmpGuid, CurAlgo, sizeof (EFI_RNG_ALGORITHM));
> +  CopyMem (CurAlgo, &mAvailableAlgoArray[0], sizeof (EFI_RNG_ALGORITHM));
> +  CopyMem (&mAvailableAlgoArray[0], &TmpGuid, sizeof (EFI_RNG_ALGORITHM));
> +
> +  return;
> +}
> +
>   /** Allocate and initialize mAvailableAlgoArray with the available
>       Rng algorithms. Also update mAvailableAlgoArrayCount.
>   
> @@ -45,7 +89,7 @@ GetAvailableAlgorithms (
>     if (!EFI_ERROR (Status)) {
>       CopyMem (
>         &mAvailableAlgoArray[mAvailableAlgoArrayCount],
> -      RngGuid,
> +      &RngGuid,
>         sizeof (RngGuid)
>         );
>       mAvailableAlgoArrayCount++;
> @@ -68,5 +112,7 @@ GetAvailableAlgorithms (
>       mAvailableAlgoArrayCount++;
>     }
>   
> +  RngFindDefaultAlgo ();
> +
>     return EFI_SUCCESS;
>   }

Re: [edk2-devel] [PATCH v1 7/8] SecurityPkg/RngDxe: Select safe default Rng algorithm

[Kun Qin]

Hi Sami,

Your suggestion in https://edk2.groups.io/g/devel/message/106511 works 
properly during my test.

But I think we still need to keep the `+      &RngGuid, ` change below 
as a bug fix?

Thanks,
Kun

On 6/29/2023 3:28 AM, Sami Mujawar wrote:
> Hi Pierre,
>
> I think this patch would not be required if my suggestions for patch 
> 6/8 are adopted.
>
> Regards,
>
> Sami Mujawar
>
> On 09/05/2023 08:40 am, pierre.gondois@arm.com wrote:
>> From: Pierre Gondois <pierre.gondois@arm.com>
>>
>> The first element of mAvailableAlgoArray should be the default
>> algorithm to avoid going through a selection process at each
>> RngGetRNG() call.
>> Once all the available Rng algorithms have been probed, place
>> a safe Rng algorithm at the first position of mAvailableAlgoArray.
>>
>> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
>> ---
>>   .../RngDxe/AArch64/AArch64Algo.c              | 48 ++++++++++++++++++-
>>   1 file changed, 47 insertions(+), 1 deletion(-)
>>
>> diff --git 
>> a/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c 
>> b/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
>> index a1ff7bd58fda..ed236b2e8141 100644
>> --- a/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
>> +++ b/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
>> @@ -17,6 +17,50 @@
>>   // Maximum number of Rng algorithms.
>>   #define RNG_AVAILABLE_ALGO_MAX  2
>>   +/** mAvailableAlgoArray[0] should contain the default Rng algorithm.
>> +    The Rng algorithm at the first index might be unsafe.
>> +    If a safe algorithm is available, choose it as the default one.
>> +**/
>> +VOID
>> +EFIAPI
>> +RngFindDefaultAlgo (
>> +  VOID
>> +  )
>> +{
>> +  EFI_RNG_ALGORITHM  *CurAlgo;
>> +  EFI_RNG_ALGORITHM  TmpGuid;
>> +  UINTN              Index;
>> +
>> +  CurAlgo = &mAvailableAlgoArray[0];
>> +
>> +  if (IsZeroGuid (CurAlgo) ||
>> +      !CompareGuid (CurAlgo, &gEfiRngAlgorithmUnSafe))
>> +  {
>> +    // mAvailableAlgoArray[0] is a valid Rng algorithm.
>> +    return;
>> +  }
>> +
>> +  for (Index = 1; Index < mAvailableAlgoArrayCount; Index++) {
>> +    CurAlgo = &mAvailableAlgoArray[Index];
>> +    if (!IsZeroGuid (CurAlgo) ||
>> +        CompareGuid (CurAlgo, &gEfiRngAlgorithmUnSafe))
>> +    {
>> +      break;
>> +    }
>> +  }
>> +
>> +  if (Index == mAvailableAlgoArrayCount) {
>> +    // No valid Rng algorithm available.
>> +    return;
>> +  }
>> +
>> +  CopyMem (&TmpGuid, CurAlgo, sizeof (EFI_RNG_ALGORITHM));
>> +  CopyMem (CurAlgo, &mAvailableAlgoArray[0], sizeof 
>> (EFI_RNG_ALGORITHM));
>> +  CopyMem (&mAvailableAlgoArray[0], &TmpGuid, sizeof 
>> (EFI_RNG_ALGORITHM));
>> +
>> +  return;
>> +}
>> +
>>   /** Allocate and initialize mAvailableAlgoArray with the available
>>       Rng algorithms. Also update mAvailableAlgoArrayCount.
>>   @@ -45,7 +89,7 @@ GetAvailableAlgorithms (
>>     if (!EFI_ERROR (Status)) {
>>       CopyMem (
>>         &mAvailableAlgoArray[mAvailableAlgoArrayCount],
>> -      RngGuid,
>> +      &RngGuid,
>>         sizeof (RngGuid)
>>         );
>>       mAvailableAlgoArrayCount++;
>> @@ -68,5 +112,7 @@ GetAvailableAlgorithms (
>>       mAvailableAlgoArrayCount++;
>>     }
>>   +  RngFindDefaultAlgo ();
>> +
>>     return EFI_SUCCESS;
>>   }
>
>
> 
>
>

Re: [edk2-devel] [PATCH v1 7/8] SecurityPkg/RngDxe: Select safe default Rng algorithm

[Sami Mujawar]

Hi Kun,


On 30/06/2023, 00:08, "Kun Qin" <kuqin12@gmail.com <mailto:kuqin12@gmail.com>> wrote:


Hi Sami,


Your suggestion in https://edk2.groups.io/g/devel/message/106511 <https://edk2.groups.io/g/devel/message/106511> works 
properly during my test.
[SAMI] Thank you for trying this out and confirming. 

But I think we still need to keep the `+ &RngGuid, ` change below 
as a bug fix?
[SAMI] You are right &RngGuid is required. But I think it should be addressed as part of patch 6/8 as that is where the change was initially introduced.
I think in my suggestion the code fixes this issue. If not, then this needs to be addressed in patch 6/8.
[/SAMI]

Regards,

Sami Mujawar

Thanks,
Kun


On 6/29/2023 3:28 AM, Sami Mujawar wrote:
> Hi Pierre,
>
> I think this patch would not be required if my suggestions for patch 
> 6/8 are adopted.
>
> Regards,
>
> Sami Mujawar
>
> On 09/05/2023 08:40 am, pierre.gondois@arm.com <mailto:pierre.gondois@arm.com> wrote:
>> From: Pierre Gondois <pierre.gondois@arm.com <mailto:pierre.gondois@arm.com>>
>>
>> The first element of mAvailableAlgoArray should be the default
>> algorithm to avoid going through a selection process at each
>> RngGetRNG() call.
>> Once all the available Rng algorithms have been probed, place
>> a safe Rng algorithm at the first position of mAvailableAlgoArray.
>>
>> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com <mailto:pierre.gondois@arm.com>>
>> ---
>> .../RngDxe/AArch64/AArch64Algo.c | 48 ++++++++++++++++++-
>> 1 file changed, 47 insertions(+), 1 deletion(-)
>>
>> diff --git 
>> a/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c 
>> b/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
>> index a1ff7bd58fda..ed236b2e8141 100644
>> --- a/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
>> +++ b/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
>> @@ -17,6 +17,50 @@
>> // Maximum number of Rng algorithms.
>> #define RNG_AVAILABLE_ALGO_MAX 2
>> +/** mAvailableAlgoArray[0] should contain the default Rng algorithm.
>> + The Rng algorithm at the first index might be unsafe.
>> + If a safe algorithm is available, choose it as the default one.
>> +**/
>> +VOID
>> +EFIAPI
>> +RngFindDefaultAlgo (
>> + VOID
>> + )
>> +{
>> + EFI_RNG_ALGORITHM *CurAlgo;
>> + EFI_RNG_ALGORITHM TmpGuid;
>> + UINTN Index;
>> +
>> + CurAlgo = &mAvailableAlgoArray[0];
>> +
>> + if (IsZeroGuid (CurAlgo) ||
>> + !CompareGuid (CurAlgo, &gEfiRngAlgorithmUnSafe))
>> + {
>> + // mAvailableAlgoArray[0] is a valid Rng algorithm.
>> + return;
>> + }
>> +
>> + for (Index = 1; Index < mAvailableAlgoArrayCount; Index++) {
>> + CurAlgo = &mAvailableAlgoArray[Index];
>> + if (!IsZeroGuid (CurAlgo) ||
>> + CompareGuid (CurAlgo, &gEfiRngAlgorithmUnSafe))
>> + {
>> + break;
>> + }
>> + }
>> +
>> + if (Index == mAvailableAlgoArrayCount) {
>> + // No valid Rng algorithm available.
>> + return;
>> + }
>> +
>> + CopyMem (&TmpGuid, CurAlgo, sizeof (EFI_RNG_ALGORITHM));
>> + CopyMem (CurAlgo, &mAvailableAlgoArray[0], sizeof 
>> (EFI_RNG_ALGORITHM));
>> + CopyMem (&mAvailableAlgoArray[0], &TmpGuid, sizeof 
>> (EFI_RNG_ALGORITHM));
>> +
>> + return;
>> +}
>> +
>> /** Allocate and initialize mAvailableAlgoArray with the available
>> Rng algorithms. Also update mAvailableAlgoArrayCount.
>> @@ -45,7 +89,7 @@ GetAvailableAlgorithms (
>> if (!EFI_ERROR (Status)) {
>> CopyMem (
>> &mAvailableAlgoArray[mAvailableAlgoArrayCount],
>> - RngGuid,
>> + &RngGuid,
>> sizeof (RngGuid)
>> );
>> mAvailableAlgoArrayCount++;
>> @@ -68,5 +112,7 @@ GetAvailableAlgorithms (
>> mAvailableAlgoArrayCount++;
>> }
>> + RngFindDefaultAlgo ();
>> +
>> return EFI_SUCCESS;
>> }
>
>
> 
>
>

[PATCH v1 8/8] SecurityPkg/RngDxe: Simplify Rng algorithm selection for Arm

[PierreGondois]

From: Pierre Gondois <pierre.gondois@arm.com>

The first element of mAvailableAlgoArray is defined as the default
Rng algorithm to use. Don't go through the array at each RngGetRNG()
call and just return the first element of the array.

Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
---
 .../RandomNumberGenerator/RngDxe/ArmRngDxe.c    | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c b/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
index 78a18c5e1177..7a42e3cbe3d2 100644
--- a/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
+++ b/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
@@ -77,7 +77,6 @@ RngGetRNG (
   )
 {
   EFI_STATUS  Status;
-  UINTN       Index;
   GUID        RngGuid;
 
   if ((This == NULL) || (RNGValueLength == 0) || (RNGValue == NULL)) {
@@ -88,21 +87,13 @@ RngGetRNG (
     //
     // Use the default RNG algorithm if RNGAlgorithm is NULL.
     //
-    for (Index = 0; Index < mAvailableAlgoArrayCount; Index++) {
-      if (!IsZeroGuid (&mAvailableAlgoArray[Index])) {
-        RNGAlgorithm = &mAvailableAlgoArray[Index];
-        goto FoundAlgo;
-      }
-    }
-
-    if (Index == mAvailableAlgoArrayCount) {
-      // No algorithm available.
-      ASSERT (Index != mAvailableAlgoArrayCount);
-      return EFI_DEVICE_ERROR;
+    if (mAvailableAlgoArrayCount != 0) {
+      RNGAlgorithm = &mAvailableAlgoArray[0];
+    } else {
+      return EFI_UNSUPPORTED;
     }
   }
 
-FoundAlgo:
   Status = GetRngGuid (&RngGuid);
   if (!EFI_ERROR (Status) &&
       CompareGuid (RNGAlgorithm, &RngGuid))
-- 
2.25.1

Re: [PATCH v1 8/8] SecurityPkg/RngDxe: Simplify Rng algorithm selection for Arm

[Sami Mujawar]

Hi Pierre,

Thank you for this patch.

These changes look good to me.

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 first element of mAvailableAlgoArray is defined as the default
> Rng algorithm to use. Don't go through the array at each RngGetRNG()
> call and just return the first element of the array.
>
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>   .../RandomNumberGenerator/RngDxe/ArmRngDxe.c    | 17 ++++-------------
>   1 file changed, 4 insertions(+), 13 deletions(-)
>
> diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c b/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
> index 78a18c5e1177..7a42e3cbe3d2 100644
> --- a/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
> +++ b/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
> @@ -77,7 +77,6 @@ RngGetRNG (
>     )
>   {
>     EFI_STATUS  Status;
> -  UINTN       Index;
>     GUID        RngGuid;
>   
>     if ((This == NULL) || (RNGValueLength == 0) || (RNGValue == NULL)) {
> @@ -88,21 +87,13 @@ RngGetRNG (
>       //
>       // Use the default RNG algorithm if RNGAlgorithm is NULL.
>       //
> -    for (Index = 0; Index < mAvailableAlgoArrayCount; Index++) {
> -      if (!IsZeroGuid (&mAvailableAlgoArray[Index])) {
> -        RNGAlgorithm = &mAvailableAlgoArray[Index];
> -        goto FoundAlgo;
> -      }
> -    }
> -
> -    if (Index == mAvailableAlgoArrayCount) {
> -      // No algorithm available.
> -      ASSERT (Index != mAvailableAlgoArrayCount);
> -      return EFI_DEVICE_ERROR;
> +    if (mAvailableAlgoArrayCount != 0) {
> +      RNGAlgorithm = &mAvailableAlgoArray[0];
> +    } else {
> +      return EFI_UNSUPPORTED;
>       }
>     }
>   
> -FoundAlgo:
>     Status = GetRngGuid (&RngGuid);
>     if (!EFI_ERROR (Status) &&
>         CompareGuid (RNGAlgorithm, &RngGuid))