[PATCH v3 0/4] ArmPkg/SecurityPkg: Fixes for ArmTrngLib/RngDxe

[PATCH v3 0/4] ArmPkg/SecurityPkg: Fixes for ArmTrngLib/RngDxe

[PierreGondois]

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

v1:
- https://edk2.groups.io/g/devel/message/96356
v2:
- https://edk2.groups.io/g/devel/message/96434
- Reformulate commit message.
- Do not warn if no algorithm is found as the message
  would be printed on non-Arm platforms.
v3:
- Add the following patches:
  1. ArmPkg/ArmTrngLib: Remove ASSERTs in ArmTrngLibConstructor()
     Requested by Ard.
     Cf https://edk2.groups.io/g/devel/message/96495
  2. SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
     Do not install EFI_RNG_PROTOCOL if no RNG algorithm is available.
     Cf. https://edk2.groups.io/g/devel/message/96494
  3. SecurityPkg/RngDxe: Fix Rng algo selection for Arm
     Coming from v2 patch being split.

Some issues were found by Ard/Sami on the RngDxe/ArmTrngLib after
recent patches were merged. This patch serie intends to fix them.

Pierre Gondois (4):
  ArmPkg/ArmTrngLib: Remove ASSERTs in ArmTrngLibConstructor()
  SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount
  SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
  SecurityPkg/RngDxe: Fix Rng algo selection for Arm

 ArmPkg/Library/ArmTrngLib/ArmTrngLib.c        |  5 -----
 .../RandomNumberGenerator/RngDxe/ArmRngDxe.c  | 18 +++++-------------
 .../RngDxe/Rand/RngDxe.c                      |  9 ++++++++-
 .../RandomNumberGenerator/RngDxe/RngDxe.c     | 19 ++++++++++++++-----
 4 files changed, 27 insertions(+), 24 deletions(-)

-- 
2.25.1

[PATCH v3 1/4] ArmPkg/ArmTrngLib: Remove ASSERTs in ArmTrngLibConstructor()

[PierreGondois]

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

Remove ASSERTs in ArmTrngLibConstructor() that prevent from
booting on DEBUG builds.

Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
---
 ArmPkg/Library/ArmTrngLib/ArmTrngLib.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/ArmPkg/Library/ArmTrngLib/ArmTrngLib.c b/ArmPkg/Library/ArmTrngLib/ArmTrngLib.c
index 3278722320c8..c2555f3ea6fe 100644
--- a/ArmPkg/Library/ArmTrngLib/ArmTrngLib.c
+++ b/ArmPkg/Library/ArmTrngLib/ArmTrngLib.c
@@ -331,14 +331,12 @@ ArmTrngLibConstructor (
   ArmMonitorCall (&Parameters);
   Status = TrngStatusToReturnStatus ((INT32)Parameters.Arg0);
   if (RETURN_ERROR (Status)) {
-    ASSERT_RETURN_ERROR (Status);
     goto ErrorHandler;
   }
 
   // Cf [1] s2.1.3 'Caller responsibilities',
   // SMCCC version must be greater or equal than 1.1
   if ((INT32)Parameters.Arg0 < 0x10001) {
-    ASSERT_RETURN_ERROR (RETURN_UNSUPPORTED);
     goto ErrorHandler;
   }
 
@@ -350,14 +348,12 @@ ArmTrngLibConstructor (
   // Check that the required features are present.
   Status = GetArmTrngFeatures (ARM_SMC_ID_TRNG_RND, NULL);
   if (RETURN_ERROR (Status)) {
-    ASSERT_RETURN_ERROR (Status);
     goto ErrorHandler;
   }
 
   // Check if TRNG UUID is supported and if so trace the GUID.
   Status = GetArmTrngFeatures (ARM_SMC_ID_TRNG_GET_UUID, NULL);
   if (RETURN_ERROR (Status)) {
-    ASSERT_RETURN_ERROR (Status);
     goto ErrorHandler;
   }
 
@@ -365,7 +361,6 @@ ArmTrngLibConstructor (
 
   Status = GetArmTrngUuid (&Guid);
   if (RETURN_ERROR (Status)) {
-    ASSERT_RETURN_ERROR (Status);
     goto ErrorHandler;
   }
 
-- 
2.25.1

[PATCH v3 2/4] SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount

[PierreGondois]

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

mAvailableAlgoArrayCount holds the count of available RNG algorithms.
In a following patch, its value will be used to prevent the
EFI_RNG_PROTOCOL to be installed if no RNG algorithm is available.

Correctly set/reset the value for all implementations.

Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
---
 SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c   | 1 +
 SecurityPkg/RandomNumberGenerator/RngDxe/Rand/RngDxe.c | 9 ++++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c b/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
index 5ba319899ce9..ce49ff7ae661 100644
--- a/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
+++ b/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
@@ -40,6 +40,7 @@ FreeAvailableAlgorithms (
   VOID
   )
 {
+  mAvailableAlgoArrayCount = 0;
   FreePool (mAvailableAlgoArray);
   return;
 }
diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/Rand/RngDxe.c b/SecurityPkg/RandomNumberGenerator/RngDxe/Rand/RngDxe.c
index 677600bed7ab..7e06e16e4be5 100644
--- a/SecurityPkg/RandomNumberGenerator/RngDxe/Rand/RngDxe.c
+++ b/SecurityPkg/RandomNumberGenerator/RngDxe/Rand/RngDxe.c
@@ -26,6 +26,11 @@
 
 #include "RngDxeInternals.h"
 
+//
+// Count of Rng algorithms.
+//
+#define RNG_ALGORITHM_COUNT  2
+
 /** Allocate and initialize mAvailableAlgoArray with the available
     Rng algorithms. Also update mAvailableAlgoArrayCount.
 
@@ -38,6 +43,7 @@ GetAvailableAlgorithms (
   VOID
   )
 {
+  mAvailableAlgoArrayCount = RNG_ALGORITHM_COUNT;
   return EFI_SUCCESS;
 }
 
@@ -49,6 +55,7 @@ FreeAvailableAlgorithms (
   VOID
   )
 {
+  mAvailableAlgoArrayCount = 0;
   return;
 }
 
@@ -164,7 +171,7 @@ RngGetInfo (
     return EFI_INVALID_PARAMETER;
   }
 
-  RequiredSize = 2 * sizeof (EFI_RNG_ALGORITHM);
+  RequiredSize = RNG_ALGORITHM_COUNT * sizeof (EFI_RNG_ALGORITHM);
 
   if (*RNGAlgorithmListSize < RequiredSize) {
     *RNGAlgorithmListSize = RequiredSize;
-- 
2.25.1

[PATCH v3 3/4] SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL

[PierreGondois]

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

On Arm platforms, the number of available RNG algorithms is
dynamically detected and can be 0 in the absence of FEAT_RNG
and firmware TRNG.
In this case, the EFI_RNG_PROTOCOL should not be installed to
prevent from installing an empty protocol.

Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
---
 .../RandomNumberGenerator/RngDxe/RngDxe.c     | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.c b/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.c
index 421abb52b8bf..d30cb7f47696 100644
--- a/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.c
+++ b/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.c
@@ -63,6 +63,18 @@ RngDriverEntry (
   EFI_STATUS  Status;
   EFI_HANDLE  Handle;
 
+  //
+  // Get the list of available algorithm.
+  //
+  Status = GetAvailableAlgorithms ();
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  if (mAvailableAlgoArrayCount == 0) {
+    return EFI_UNSUPPORTED;
+  }
+
   //
   // Install UEFI RNG (Random Number Generator) Protocol
   //
@@ -74,13 +86,10 @@ RngDriverEntry (
                   NULL
                   );
   if (EFI_ERROR (Status)) {
-    return Status;
+    FreeAvailableAlgorithms ();
   }
 
-  //
-  // Get the list of available algorithm.
-  //
-  return GetAvailableAlgorithms ();
+  return Status;
 }
 
 /**
-- 
2.25.1

[PATCH v3 4/4] SecurityPkg/RngDxe: Fix Rng algo selection for Arm

[PierreGondois]

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

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

The EFI_RNG_PROTOCOL can advertise multiple algorithms through
Guids. The PcdCpuRngSupportedAlgorithm contains a Guid that
can be configured. It represents the algorithm used in RngLib.
PcdCpuRngSupportedAlgorithm is set to the Zero Guid for KvmTool.

When running KvmTool on a platform platform only having the RngLib,
the only Guid available for EFI_RNG_PROTOCOL will be the zero Guid.

To select the default algorithm in EFI_RNG_PROTOCOL.GetRng():
a. Zero Guids are skipped
b. If no algorithm is found, an ASSERT is triggered

To allow using the RngLib to be used for the case above, Zero Guids
should not be skipped (a.).
If no algorithm is found, don't prevent from booting on DEBUG builds
(b.).

Allow Zero Guids to be selected and don't ASSERT if no algorithm is
found. Also simplify the selection of the Rng algorithm when the
default one is selected by just picking up the first element of
mAvailableAlgoArray.

Reported-by: Sami Mujawar <sami.mujawar@arm.com>
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 ce49ff7ae661..b8a343e3d397 100644
--- a/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
+++ b/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
@@ -77,7 +77,6 @@ RngGetRNG (
   )
 {
   EFI_STATUS  Status;
-  UINTN       Index;
 
   if ((This == NULL) || (RNGValueLength == 0) || (RNGValue == NULL)) {
     return EFI_INVALID_PARAMETER;
@@ -87,21 +86,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:
   if (CompareGuid (RNGAlgorithm, PcdGetPtr (PcdCpuRngSupportedAlgorithm))) {
     Status = RngGetBytes (RNGValueLength, RNGValue);
     return Status;
-- 
2.25.1

Re: [PATCH v3 0/4] ArmPkg/SecurityPkg: Fixes for ArmTrngLib/RngDxe

[Ard Biesheuvel]

On Thu, 24 Nov 2022 at 17:18, <Pierre.Gondois@arm.com> wrote:
>
> From: Pierre Gondois <pierre.gondois@arm.com>
>
> v1:
> - https://edk2.groups.io/g/devel/message/96356
> v2:
> - https://edk2.groups.io/g/devel/message/96434
> - Reformulate commit message.
> - Do not warn if no algorithm is found as the message
>   would be printed on non-Arm platforms.
> v3:
> - Add the following patches:
>   1. ArmPkg/ArmTrngLib: Remove ASSERTs in ArmTrngLibConstructor()
>      Requested by Ard.
>      Cf https://edk2.groups.io/g/devel/message/96495
>   2. SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
>      Do not install EFI_RNG_PROTOCOL if no RNG algorithm is available.
>      Cf. https://edk2.groups.io/g/devel/message/96494
>   3. SecurityPkg/RngDxe: Fix Rng algo selection for Arm
>      Coming from v2 patch being split.
>
> Some issues were found by Ard/Sami on the RngDxe/ArmTrngLib after
> recent patches were merged. This patch serie intends to fix them.
>
> Pierre Gondois (4):
>   ArmPkg/ArmTrngLib: Remove ASSERTs in ArmTrngLibConstructor()

Thanks for the fixed

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

I pushed this one as #3663 (pending CI verification atm)

>   SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount
>   SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
>   SecurityPkg/RngDxe: Fix Rng algo selection for Arm
>

The remaining code still looks a bit clunky to me. Can't we just
return an error from the library constructor of the library cannot
initialize due to a missing prerequisite?


>  ArmPkg/Library/ArmTrngLib/ArmTrngLib.c        |  5 -----
>  .../RandomNumberGenerator/RngDxe/ArmRngDxe.c  | 18 +++++-------------
>  .../RngDxe/Rand/RngDxe.c                      |  9 ++++++++-
>  .../RandomNumberGenerator/RngDxe/RngDxe.c     | 19 ++++++++++++++-----
>  4 files changed, 27 insertions(+), 24 deletions(-)
>
> --
> 2.25.1
>

Re: [PATCH v3 0/4] ArmPkg/SecurityPkg: Fixes for ArmTrngLib/RngDxe

[PierreGondois]

Hello Ard,

On 11/26/22 15:33, Ard Biesheuvel wrote:
> On Thu, 24 Nov 2022 at 17:18, <Pierre.Gondois@arm.com> wrote:
>>
>> From: Pierre Gondois <pierre.gondois@arm.com>
>>
>> v1:
>> - https://edk2.groups.io/g/devel/message/96356
>> v2:
>> - https://edk2.groups.io/g/devel/message/96434
>> - Reformulate commit message.
>> - Do not warn if no algorithm is found as the message
>>    would be printed on non-Arm platforms.
>> v3:
>> - Add the following patches:
>>    1. ArmPkg/ArmTrngLib: Remove ASSERTs in ArmTrngLibConstructor()
>>       Requested by Ard.
>>       Cf https://edk2.groups.io/g/devel/message/96495
>>    2. SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
>>       Do not install EFI_RNG_PROTOCOL if no RNG algorithm is available.
>>       Cf. https://edk2.groups.io/g/devel/message/96494
>>    3. SecurityPkg/RngDxe: Fix Rng algo selection for Arm
>>       Coming from v2 patch being split.
>>
>> Some issues were found by Ard/Sami on the RngDxe/ArmTrngLib after
>> recent patches were merged. This patch serie intends to fix them.
>>
>> Pierre Gondois (4):
>>    ArmPkg/ArmTrngLib: Remove ASSERTs in ArmTrngLibConstructor()
> 
> Thanks for the fixed
> 
> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> 
> I pushed this one as #3663 (pending CI verification atm)
> 
>>    SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount
>>    SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
>>    SecurityPkg/RngDxe: Fix Rng algo selection for Arm
>>
> 
> The remaining code still looks a bit clunky to me. Can't we just
> return an error from the library constructor of the library cannot
> initialize due to a missing prerequisite?

In RngDriverEntry(), GetAvailableAlgorithms() probe the available
RNG algorithms. If there are none, then we return an error code.
Otherwise we install EFI_RNG_PROTOCOL.

I assume the prerequisite you a referring to is 'checking there is
at least one available RNG algorithm that RngDxe can use', so if
ArmTrngLib's constructor fails, we should not prevent RngDxe to be
loaded (as the RngLib could be used for instance).

Does it answer your question, or did I understand it incorrectly ?

Regards,
Pierre

> 
> 
>>   ArmPkg/Library/ArmTrngLib/ArmTrngLib.c        |  5 -----
>>   .../RandomNumberGenerator/RngDxe/ArmRngDxe.c  | 18 +++++-------------
>>   .../RngDxe/Rand/RngDxe.c                      |  9 ++++++++-
>>   .../RandomNumberGenerator/RngDxe/RngDxe.c     | 19 ++++++++++++++-----
>>   4 files changed, 27 insertions(+), 24 deletions(-)
>>
>> --
>> 2.25.1
>>

Re: [edk2-devel] [PATCH v3 0/4] ArmPkg/SecurityPkg: Fixes for ArmTrngLib/RngDxe

[Sami Mujawar]

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

Hi Ard,
On Sat, Nov 26, 2022 at 06:34 AM, Ard Biesheuvel wrote:

> 
> The remaining code still looks a bit clunky to me. Can't we just
> return an error from the library constructor of the library cannot
> initialize due to a missing prerequisite?

The problem with returning an error code from a library constructor is that it generates an ASSERT for debug builds. The reason is that the generated code in AutoGen.c looks as shown below:

VOID
EFIAPI
ProcessLibraryConstructorList (
IN EFI_HANDLE        ImageHandle,
IN EFI_SYSTEM_TABLE  *SystemTable
)
{
EFI_STATUS  Status;

Status = HobLibConstructor (ImageHandle, SystemTable);
ASSERT_EFI_ERROR (Status);
...
Status = ArmTrngLibConstructor ();
ASSERT_RETURN_ERROR (Status);

}

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

Re: [edk2-devel] [PATCH v3 0/4] ArmPkg/SecurityPkg: Fixes for ArmTrngLib/RngDxe

[Sami Mujawar]

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

Apologies, I accidentally sent the message before I finished.  The remaining part of my reply is as below.

I think having the asserts in ProcessLibraryConstructorList() creates more problem. It is very hard to debug issues if the serial port initialisation fails.

Regards,

Sami Mujawar

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

Re: [edk2-devel] [PATCH v3 0/4] ArmPkg/SecurityPkg: Fixes for ArmTrngLib/RngDxe

[PierreGondois]

Hello,
I was wondering if I should re-arrange the patch in any way/there was any
concern with the patch set. The first patch of serie was taken, but the other
ones are pending,

Regards,
Pierre

On 11/28/22 14:34, PierreGondois via groups.io wrote:
> Hello Ard,
> 
> On 11/26/22 15:33, Ard Biesheuvel wrote:
>> On Thu, 24 Nov 2022 at 17:18, <Pierre.Gondois@arm.com> wrote:
>>>
>>> From: Pierre Gondois <pierre.gondois@arm.com>
>>>
>>> v1:
>>> - https://edk2.groups.io/g/devel/message/96356
>>> v2:
>>> - https://edk2.groups.io/g/devel/message/96434
>>> - Reformulate commit message.
>>> - Do not warn if no algorithm is found as the message
>>>     would be printed on non-Arm platforms.
>>> v3:
>>> - Add the following patches:
>>>     1. ArmPkg/ArmTrngLib: Remove ASSERTs in ArmTrngLibConstructor()
>>>        Requested by Ard.
>>>        Cf https://edk2.groups.io/g/devel/message/96495
>>>     2. SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
>>>        Do not install EFI_RNG_PROTOCOL if no RNG algorithm is available.
>>>        Cf. https://edk2.groups.io/g/devel/message/96494
>>>     3. SecurityPkg/RngDxe: Fix Rng algo selection for Arm
>>>        Coming from v2 patch being split.
>>>
>>> Some issues were found by Ard/Sami on the RngDxe/ArmTrngLib after
>>> recent patches were merged. This patch serie intends to fix them.
>>>
>>> Pierre Gondois (4):
>>>     ArmPkg/ArmTrngLib: Remove ASSERTs in ArmTrngLibConstructor()
>>
>> Thanks for the fixed
>>
>> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
>>
>> I pushed this one as #3663 (pending CI verification atm)
>>
>>>     SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount
>>>     SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
>>>     SecurityPkg/RngDxe: Fix Rng algo selection for Arm
>>>
>>
>> The remaining code still looks a bit clunky to me. Can't we just
>> return an error from the library constructor of the library cannot
>> initialize due to a missing prerequisite?
> 
> In RngDriverEntry(), GetAvailableAlgorithms() probe the available
> RNG algorithms. If there are none, then we return an error code.
> Otherwise we install EFI_RNG_PROTOCOL.
> 
> I assume the prerequisite you a referring to is 'checking there is
> at least one available RNG algorithm that RngDxe can use', so if
> ArmTrngLib's constructor fails, we should not prevent RngDxe to be
> loaded (as the RngLib could be used for instance).
> 
> Does it answer your question, or did I understand it incorrectly ?
> 
> Regards,
> Pierre
> 
>>
>>
>>>    ArmPkg/Library/ArmTrngLib/ArmTrngLib.c        |  5 -----
>>>    .../RandomNumberGenerator/RngDxe/ArmRngDxe.c  | 18 +++++-------------
>>>    .../RngDxe/Rand/RngDxe.c                      |  9 ++++++++-
>>>    .../RandomNumberGenerator/RngDxe/RngDxe.c     | 19 ++++++++++++++-----
>>>    4 files changed, 27 insertions(+), 24 deletions(-)
>>>
>>> --
>>> 2.25.1
>>>
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v3 0/4] ArmPkg/SecurityPkg: Fixes for ArmTrngLib/RngDxe

[PierreGondois]

Hello Jiewen, Jian,
Just a reminder in case this was forgotten,
Regards,
Pierre

On 1/9/23 13:26, Pierre Gondois wrote:
> Hello,
> I was wondering if I should re-arrange the patch in any way/there was any
> concern with the patch set. The first patch of serie was taken, but the other
> ones are pending,
> 
> Regards,
> Pierre
> 
> On 11/28/22 14:34, PierreGondois via groups.io wrote:
>> Hello Ard,
>>
>> On 11/26/22 15:33, Ard Biesheuvel wrote:
>>> On Thu, 24 Nov 2022 at 17:18, <Pierre.Gondois@arm.com> wrote:
>>>>
>>>> From: Pierre Gondois <pierre.gondois@arm.com>
>>>>
>>>> v1:
>>>> - https://edk2.groups.io/g/devel/message/96356
>>>> v2:
>>>> - https://edk2.groups.io/g/devel/message/96434
>>>> - Reformulate commit message.
>>>> - Do not warn if no algorithm is found as the message
>>>>      would be printed on non-Arm platforms.
>>>> v3:
>>>> - Add the following patches:
>>>>      1. ArmPkg/ArmTrngLib: Remove ASSERTs in ArmTrngLibConstructor()
>>>>         Requested by Ard.
>>>>         Cf https://edk2.groups.io/g/devel/message/96495
>>>>      2. SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
>>>>         Do not install EFI_RNG_PROTOCOL if no RNG algorithm is available.
>>>>         Cf. https://edk2.groups.io/g/devel/message/96494
>>>>      3. SecurityPkg/RngDxe: Fix Rng algo selection for Arm
>>>>         Coming from v2 patch being split.
>>>>
>>>> Some issues were found by Ard/Sami on the RngDxe/ArmTrngLib after
>>>> recent patches were merged. This patch serie intends to fix them.
>>>>
>>>> Pierre Gondois (4):
>>>>      ArmPkg/ArmTrngLib: Remove ASSERTs in ArmTrngLibConstructor()
>>>
>>> Thanks for the fixed
>>>
>>> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
>>>
>>> I pushed this one as #3663 (pending CI verification atm)
>>>
>>>>      SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount
>>>>      SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
>>>>      SecurityPkg/RngDxe: Fix Rng algo selection for Arm
>>>>
>>>
>>> The remaining code still looks a bit clunky to me. Can't we just
>>> return an error from the library constructor of the library cannot
>>> initialize due to a missing prerequisite?
>>
>> In RngDriverEntry(), GetAvailableAlgorithms() probe the available
>> RNG algorithms. If there are none, then we return an error code.
>> Otherwise we install EFI_RNG_PROTOCOL.
>>
>> I assume the prerequisite you a referring to is 'checking there is
>> at least one available RNG algorithm that RngDxe can use', so if
>> ArmTrngLib's constructor fails, we should not prevent RngDxe to be
>> loaded (as the RngLib could be used for instance).
>>
>> Does it answer your question, or did I understand it incorrectly ?
>>
>> Regards,
>> Pierre
>>
>>>
>>>
>>>>     ArmPkg/Library/ArmTrngLib/ArmTrngLib.c        |  5 -----
>>>>     .../RandomNumberGenerator/RngDxe/ArmRngDxe.c  | 18 +++++-------------
>>>>     .../RngDxe/Rand/RngDxe.c                      |  9 ++++++++-
>>>>     .../RandomNumberGenerator/RngDxe/RngDxe.c     | 19 ++++++++++++++-----
>>>>     4 files changed, 27 insertions(+), 24 deletions(-)
>>>>
>>>> --
>>>> 2.25.1
>>>>
>>
>>
>> 
>>
>>

Re: [edk2-devel] [PATCH v3 0/4] ArmPkg/SecurityPkg: Fixes for ArmTrngLib/RngDxe

[PierreGondois]

Hello Jiewen, Jian,
Do these patches in this set look ok to you ?
- SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount
- SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
- SecurityPkg/RngDxe: Fix Rng algo selection for Arm

Regards,
Pierre

On 2/10/23 10:26, PierreGondois via groups.io wrote:
> Hello Jiewen, Jian,
> Just a reminder in case this was forgotten,
> Regards,
> Pierre
> 
> On 1/9/23 13:26, Pierre Gondois wrote:
>> Hello,
>> I was wondering if I should re-arrange the patch in any way/there was any
>> concern with the patch set. The first patch of serie was taken, but the other
>> ones are pending,
>>
>> Regards,
>> Pierre
>>
>> On 11/28/22 14:34, PierreGondois via groups.io wrote:
>>> Hello Ard,
>>>
>>> On 11/26/22 15:33, Ard Biesheuvel wrote:
>>>> On Thu, 24 Nov 2022 at 17:18, <Pierre.Gondois@arm.com> wrote:
>>>>>
>>>>> From: Pierre Gondois <pierre.gondois@arm.com>
>>>>>
>>>>> v1:
>>>>> - https://edk2.groups.io/g/devel/message/96356
>>>>> v2:
>>>>> - https://edk2.groups.io/g/devel/message/96434
>>>>> - Reformulate commit message.
>>>>> - Do not warn if no algorithm is found as the message
>>>>>       would be printed on non-Arm platforms.
>>>>> v3:
>>>>> - Add the following patches:
>>>>>       1. ArmPkg/ArmTrngLib: Remove ASSERTs in ArmTrngLibConstructor()
>>>>>          Requested by Ard.
>>>>>          Cf https://edk2.groups.io/g/devel/message/96495
>>>>>       2. SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
>>>>>          Do not install EFI_RNG_PROTOCOL if no RNG algorithm is available.
>>>>>          Cf. https://edk2.groups.io/g/devel/message/96494
>>>>>       3. SecurityPkg/RngDxe: Fix Rng algo selection for Arm
>>>>>          Coming from v2 patch being split.
>>>>>
>>>>> Some issues were found by Ard/Sami on the RngDxe/ArmTrngLib after
>>>>> recent patches were merged. This patch serie intends to fix them.
>>>>>
>>>>> Pierre Gondois (4):
>>>>>       ArmPkg/ArmTrngLib: Remove ASSERTs in ArmTrngLibConstructor()
>>>>
>>>> Thanks for the fixed
>>>>
>>>> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
>>>>
>>>> I pushed this one as #3663 (pending CI verification atm)
>>>>
>>>>>       SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount
>>>>>       SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
>>>>>       SecurityPkg/RngDxe: Fix Rng algo selection for Arm
>>>>>
>>>>
>>>> The remaining code still looks a bit clunky to me. Can't we just
>>>> return an error from the library constructor of the library cannot
>>>> initialize due to a missing prerequisite?
>>>
>>> In RngDriverEntry(), GetAvailableAlgorithms() probe the available
>>> RNG algorithms. If there are none, then we return an error code.
>>> Otherwise we install EFI_RNG_PROTOCOL.
>>>
>>> I assume the prerequisite you a referring to is 'checking there is
>>> at least one available RNG algorithm that RngDxe can use', so if
>>> ArmTrngLib's constructor fails, we should not prevent RngDxe to be
>>> loaded (as the RngLib could be used for instance).
>>>
>>> Does it answer your question, or did I understand it incorrectly ?
>>>
>>> Regards,
>>> Pierre
>>>
>>>>
>>>>
>>>>>      ArmPkg/Library/ArmTrngLib/ArmTrngLib.c        |  5 -----
>>>>>      .../RandomNumberGenerator/RngDxe/ArmRngDxe.c  | 18 +++++-------------
>>>>>      .../RngDxe/Rand/RngDxe.c                      |  9 ++++++++-
>>>>>      .../RandomNumberGenerator/RngDxe/RngDxe.c     | 19 ++++++++++++++-----
>>>>>      4 files changed, 27 insertions(+), 24 deletions(-)
>>>>>
>>>>> --
>>>>> 2.25.1
>>>>>
>>>
>>>
>>>
>>>
>>>
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v3 0/4] ArmPkg/SecurityPkg: Fixes for ArmTrngLib/RngDxe

[Yao, Jiewen]

Hi Pierre
I don’t have strong opinion.

For ARM specific patch, would you please get R-B from ARM expert?

I think we need to wait for the response from Ard to confirm.


> -----Original Message-----
> From: Pierre Gondois <pierre.gondois@arm.com>
> Sent: Monday, March 6, 2023 11:38 PM
> To: devel@edk2.groups.io
> Cc: Leif Lindholm <leif@nuviainc.com>; Sami Mujawar
> <sami.mujawar@arm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Wang,
> Jian J <jian.j.wang@intel.com>; Ard Biesheuvel <ardb@kernel.org>
> Subject: Re: [edk2-devel] [PATCH v3 0/4] ArmPkg/SecurityPkg: Fixes for
> ArmTrngLib/RngDxe
> 
> Hello Jiewen, Jian,
> Do these patches in this set look ok to you ?
> - SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount
> - SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
> - SecurityPkg/RngDxe: Fix Rng algo selection for Arm
> 
> Regards,
> Pierre
> 
> On 2/10/23 10:26, PierreGondois via groups.io wrote:
> > Hello Jiewen, Jian,
> > Just a reminder in case this was forgotten,
> > Regards,
> > Pierre
> >
> > On 1/9/23 13:26, Pierre Gondois wrote:
> >> Hello,
> >> I was wondering if I should re-arrange the patch in any way/there was any
> >> concern with the patch set. The first patch of serie was taken, but the
> other
> >> ones are pending,
> >>
> >> Regards,
> >> Pierre
> >>
> >> On 11/28/22 14:34, PierreGondois via groups.io wrote:
> >>> Hello Ard,
> >>>
> >>> On 11/26/22 15:33, Ard Biesheuvel wrote:
> >>>> On Thu, 24 Nov 2022 at 17:18, <Pierre.Gondois@arm.com> wrote:
> >>>>>
> >>>>> From: Pierre Gondois <pierre.gondois@arm.com>
> >>>>>
> >>>>> v1:
> >>>>> - https://edk2.groups.io/g/devel/message/96356
> >>>>> v2:
> >>>>> - https://edk2.groups.io/g/devel/message/96434
> >>>>> - Reformulate commit message.
> >>>>> - Do not warn if no algorithm is found as the message
> >>>>>       would be printed on non-Arm platforms.
> >>>>> v3:
> >>>>> - Add the following patches:
> >>>>>       1. ArmPkg/ArmTrngLib: Remove ASSERTs in
> ArmTrngLibConstructor()
> >>>>>          Requested by Ard.
> >>>>>          Cf https://edk2.groups.io/g/devel/message/96495
> >>>>>       2. SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
> >>>>>          Do not install EFI_RNG_PROTOCOL if no RNG algorithm is
> available.
> >>>>>          Cf. https://edk2.groups.io/g/devel/message/96494
> >>>>>       3. SecurityPkg/RngDxe: Fix Rng algo selection for Arm
> >>>>>          Coming from v2 patch being split.
> >>>>>
> >>>>> Some issues were found by Ard/Sami on the RngDxe/ArmTrngLib after
> >>>>> recent patches were merged. This patch serie intends to fix them.
> >>>>>
> >>>>> Pierre Gondois (4):
> >>>>>       ArmPkg/ArmTrngLib: Remove ASSERTs in ArmTrngLibConstructor()
> >>>>
> >>>> Thanks for the fixed
> >>>>
> >>>> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> >>>>
> >>>> I pushed this one as #3663 (pending CI verification atm)
> >>>>
> >>>>>       SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount
> >>>>>       SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
> >>>>>       SecurityPkg/RngDxe: Fix Rng algo selection for Arm
> >>>>>
> >>>>
> >>>> The remaining code still looks a bit clunky to me. Can't we just
> >>>> return an error from the library constructor of the library cannot
> >>>> initialize due to a missing prerequisite?
> >>>
> >>> In RngDriverEntry(), GetAvailableAlgorithms() probe the available
> >>> RNG algorithms. If there are none, then we return an error code.
> >>> Otherwise we install EFI_RNG_PROTOCOL.
> >>>
> >>> I assume the prerequisite you a referring to is 'checking there is
> >>> at least one available RNG algorithm that RngDxe can use', so if
> >>> ArmTrngLib's constructor fails, we should not prevent RngDxe to be
> >>> loaded (as the RngLib could be used for instance).
> >>>
> >>> Does it answer your question, or did I understand it incorrectly ?
> >>>
> >>> Regards,
> >>> Pierre
> >>>
> >>>>
> >>>>
> >>>>>      ArmPkg/Library/ArmTrngLib/ArmTrngLib.c        |  5 -----
> >>>>>      .../RandomNumberGenerator/RngDxe/ArmRngDxe.c  | 18 +++++---
> ----------
> >>>>>      .../RngDxe/Rand/RngDxe.c                      |  9 ++++++++-
> >>>>>      .../RandomNumberGenerator/RngDxe/RngDxe.c     | 19
> ++++++++++++++-----
> >>>>>      4 files changed, 27 insertions(+), 24 deletions(-)
> >>>>>
> >>>>> --
> >>>>> 2.25.1
> >>>>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >
> >
> > 
> >
> >

Re: [edk2-devel] [PATCH v3 0/4] ArmPkg/SecurityPkg: Fixes for ArmTrngLib/RngDxe

[Ard Biesheuvel]

On Mon, 6 Mar 2023 at 16:42, Yao, Jiewen <jiewen.yao@intel.com> wrote:
>
> Hi Pierre
> I don’t have strong opinion.
>
> For ARM specific patch, would you please get R-B from ARM expert?
>
> I think we need to wait for the response from Ard to confirm.
>

These patches

  SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount
  SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

Jiewen, if you don't mind, I will merge those right away.

For the remaining patch, I am not sure I understand why the behavior
regarding the zero GUID is correct. Perhaps we could
revisit/resend/review that patch in isolation?

Thanks,
Ard.




>
> > -----Original Message-----
> > From: Pierre Gondois <pierre.gondois@arm.com>
> > Sent: Monday, March 6, 2023 11:38 PM
> > To: devel@edk2.groups.io
> > Cc: Leif Lindholm <leif@nuviainc.com>; Sami Mujawar
> > <sami.mujawar@arm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Wang,
> > Jian J <jian.j.wang@intel.com>; Ard Biesheuvel <ardb@kernel.org>
> > Subject: Re: [edk2-devel] [PATCH v3 0/4] ArmPkg/SecurityPkg: Fixes for
> > ArmTrngLib/RngDxe
> >
> > Hello Jiewen, Jian,
> > Do these patches in this set look ok to you ?
> > - SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount
> > - SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
> > - SecurityPkg/RngDxe: Fix Rng algo selection for Arm
> >
> > Regards,
> > Pierre
> >
> > On 2/10/23 10:26, PierreGondois via groups.io wrote:
> > > Hello Jiewen, Jian,
> > > Just a reminder in case this was forgotten,
> > > Regards,
> > > Pierre
> > >
> > > On 1/9/23 13:26, Pierre Gondois wrote:
> > >> Hello,
> > >> I was wondering if I should re-arrange the patch in any way/there was any
> > >> concern with the patch set. The first patch of serie was taken, but the
> > other
> > >> ones are pending,
> > >>
> > >> Regards,
> > >> Pierre
> > >>
> > >> On 11/28/22 14:34, PierreGondois via groups.io wrote:
> > >>> Hello Ard,
> > >>>
> > >>> On 11/26/22 15:33, Ard Biesheuvel wrote:
> > >>>> On Thu, 24 Nov 2022 at 17:18, <Pierre.Gondois@arm.com> wrote:
> > >>>>>
> > >>>>> From: Pierre Gondois <pierre.gondois@arm.com>
> > >>>>>
> > >>>>> v1:
> > >>>>> - https://edk2.groups.io/g/devel/message/96356
> > >>>>> v2:
> > >>>>> - https://edk2.groups.io/g/devel/message/96434
> > >>>>> - Reformulate commit message.
> > >>>>> - Do not warn if no algorithm is found as the message
> > >>>>>       would be printed on non-Arm platforms.
> > >>>>> v3:
> > >>>>> - Add the following patches:
> > >>>>>       1. ArmPkg/ArmTrngLib: Remove ASSERTs in
> > ArmTrngLibConstructor()
> > >>>>>          Requested by Ard.
> > >>>>>          Cf https://edk2.groups.io/g/devel/message/96495
> > >>>>>       2. SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
> > >>>>>          Do not install EFI_RNG_PROTOCOL if no RNG algorithm is
> > available.
> > >>>>>          Cf. https://edk2.groups.io/g/devel/message/96494
> > >>>>>       3. SecurityPkg/RngDxe: Fix Rng algo selection for Arm
> > >>>>>          Coming from v2 patch being split.
> > >>>>>
> > >>>>> Some issues were found by Ard/Sami on the RngDxe/ArmTrngLib after
> > >>>>> recent patches were merged. This patch serie intends to fix them.
> > >>>>>
> > >>>>> Pierre Gondois (4):
> > >>>>>       ArmPkg/ArmTrngLib: Remove ASSERTs in ArmTrngLibConstructor()
> > >>>>
> > >>>> Thanks for the fixed
> > >>>>
> > >>>> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> > >>>>
> > >>>> I pushed this one as #3663 (pending CI verification atm)
> > >>>>
> > >>>>>       SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount
> > >>>>>       SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
> > >>>>>       SecurityPkg/RngDxe: Fix Rng algo selection for Arm
> > >>>>>
> > >>>>
> > >>>> The remaining code still looks a bit clunky to me. Can't we just
> > >>>> return an error from the library constructor of the library cannot
> > >>>> initialize due to a missing prerequisite?
> > >>>
> > >>> In RngDriverEntry(), GetAvailableAlgorithms() probe the available
> > >>> RNG algorithms. If there are none, then we return an error code.
> > >>> Otherwise we install EFI_RNG_PROTOCOL.
> > >>>
> > >>> I assume the prerequisite you a referring to is 'checking there is
> > >>> at least one available RNG algorithm that RngDxe can use', so if
> > >>> ArmTrngLib's constructor fails, we should not prevent RngDxe to be
> > >>> loaded (as the RngLib could be used for instance).
> > >>>
> > >>> Does it answer your question, or did I understand it incorrectly ?
> > >>>
> > >>> Regards,
> > >>> Pierre
> > >>>
> > >>>>
> > >>>>
> > >>>>>      ArmPkg/Library/ArmTrngLib/ArmTrngLib.c        |  5 -----
> > >>>>>      .../RandomNumberGenerator/RngDxe/ArmRngDxe.c  | 18 +++++---
> > ----------
> > >>>>>      .../RngDxe/Rand/RngDxe.c                      |  9 ++++++++-
> > >>>>>      .../RandomNumberGenerator/RngDxe/RngDxe.c     | 19
> > ++++++++++++++-----
> > >>>>>      4 files changed, 27 insertions(+), 24 deletions(-)
> > >>>>>
> > >>>>> --
> > >>>>> 2.25.1
> > >>>>>
> > >>>
> > >>>
> > >>>
> > >>>
> > >>>
> > >
> > >
> > > 
> > >
> > >

Re: [edk2-devel] [PATCH v3 0/4] ArmPkg/SecurityPkg: Fixes for ArmTrngLib/RngDxe

[PierreGondois]

Hello Jiewen, Ard,
Thanks for the review.

On 3/6/23 17:22, Ard Biesheuvel wrote:
> On Mon, 6 Mar 2023 at 16:42, Yao, Jiewen <jiewen.yao@intel.com> wrote:
>>
>> Hi Pierre
>> I don’t have strong opinion.
>>
>> For ARM specific patch, would you please get R-B from ARM expert?
>>
>> I think we need to wait for the response from Ard to confirm.
>>
> 
> These patches
> 
>    SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount
>    SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
> 
> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> 
> Jiewen, if you don't mind, I will merge those right away.
> 
> For the remaining patch, I am not sure I understand why the behavior
> regarding the zero GUID is correct. Perhaps we could
> revisit/resend/review that patch in isolation?

About the zero GUID, the PcdCpuRngSupportedAlgorithm allows to describe
the platform specific rng algorithm used. However KvmTool could run
on any platform, so PcdCpuRngSupportedAlgorithm cannot be set to a proper
GUID value.
A zero GUID is not really compliant to the UEFI spec (s37.5.1 EFI RNG
Algorithm Definitions), but I am not sure which other choice could be
made,

I'm not sure this was your question, please let know if it wasn't,

Regards,
Pierre


> 
> Thanks,
> Ard.
> 
> 
> 
> 
>>
>>> -----Original Message-----
>>> From: Pierre Gondois <pierre.gondois@arm.com>
>>> Sent: Monday, March 6, 2023 11:38 PM
>>> To: devel@edk2.groups.io
>>> Cc: Leif Lindholm <leif@nuviainc.com>; Sami Mujawar
>>> <sami.mujawar@arm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Wang,
>>> Jian J <jian.j.wang@intel.com>; Ard Biesheuvel <ardb@kernel.org>
>>> Subject: Re: [edk2-devel] [PATCH v3 0/4] ArmPkg/SecurityPkg: Fixes for
>>> ArmTrngLib/RngDxe
>>>
>>> Hello Jiewen, Jian,
>>> Do these patches in this set look ok to you ?
>>> - SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount
>>> - SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
>>> - SecurityPkg/RngDxe: Fix Rng algo selection for Arm
>>>
>>> Regards,
>>> Pierre
>>>
>>> On 2/10/23 10:26, PierreGondois via groups.io wrote:
>>>> Hello Jiewen, Jian,
>>>> Just a reminder in case this was forgotten,
>>>> Regards,
>>>> Pierre
>>>>
>>>> On 1/9/23 13:26, Pierre Gondois wrote:
>>>>> Hello,
>>>>> I was wondering if I should re-arrange the patch in any way/there was any
>>>>> concern with the patch set. The first patch of serie was taken, but the
>>> other
>>>>> ones are pending,
>>>>>
>>>>> Regards,
>>>>> Pierre
>>>>>
>>>>> On 11/28/22 14:34, PierreGondois via groups.io wrote:
>>>>>> Hello Ard,
>>>>>>
>>>>>> On 11/26/22 15:33, Ard Biesheuvel wrote:
>>>>>>> On Thu, 24 Nov 2022 at 17:18, <Pierre.Gondois@arm.com> wrote:
>>>>>>>>
>>>>>>>> From: Pierre Gondois <pierre.gondois@arm.com>
>>>>>>>>
>>>>>>>> v1:
>>>>>>>> - https://edk2.groups.io/g/devel/message/96356
>>>>>>>> v2:
>>>>>>>> - https://edk2.groups.io/g/devel/message/96434
>>>>>>>> - Reformulate commit message.
>>>>>>>> - Do not warn if no algorithm is found as the message
>>>>>>>>        would be printed on non-Arm platforms.
>>>>>>>> v3:
>>>>>>>> - Add the following patches:
>>>>>>>>        1. ArmPkg/ArmTrngLib: Remove ASSERTs in
>>> ArmTrngLibConstructor()
>>>>>>>>           Requested by Ard.
>>>>>>>>           Cf https://edk2.groups.io/g/devel/message/96495
>>>>>>>>        2. SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
>>>>>>>>           Do not install EFI_RNG_PROTOCOL if no RNG algorithm is
>>> available.
>>>>>>>>           Cf. https://edk2.groups.io/g/devel/message/96494
>>>>>>>>        3. SecurityPkg/RngDxe: Fix Rng algo selection for Arm
>>>>>>>>           Coming from v2 patch being split.
>>>>>>>>
>>>>>>>> Some issues were found by Ard/Sami on the RngDxe/ArmTrngLib after
>>>>>>>> recent patches were merged. This patch serie intends to fix them.
>>>>>>>>
>>>>>>>> Pierre Gondois (4):
>>>>>>>>        ArmPkg/ArmTrngLib: Remove ASSERTs in ArmTrngLibConstructor()
>>>>>>>
>>>>>>> Thanks for the fixed
>>>>>>>
>>>>>>> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
>>>>>>>
>>>>>>> I pushed this one as #3663 (pending CI verification atm)
>>>>>>>
>>>>>>>>        SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount
>>>>>>>>        SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
>>>>>>>>        SecurityPkg/RngDxe: Fix Rng algo selection for Arm
>>>>>>>>
>>>>>>>
>>>>>>> The remaining code still looks a bit clunky to me. Can't we just
>>>>>>> return an error from the library constructor of the library cannot
>>>>>>> initialize due to a missing prerequisite?
>>>>>>
>>>>>> In RngDriverEntry(), GetAvailableAlgorithms() probe the available
>>>>>> RNG algorithms. If there are none, then we return an error code.
>>>>>> Otherwise we install EFI_RNG_PROTOCOL.
>>>>>>
>>>>>> I assume the prerequisite you a referring to is 'checking there is
>>>>>> at least one available RNG algorithm that RngDxe can use', so if
>>>>>> ArmTrngLib's constructor fails, we should not prevent RngDxe to be
>>>>>> loaded (as the RngLib could be used for instance).
>>>>>>
>>>>>> Does it answer your question, or did I understand it incorrectly ?
>>>>>>
>>>>>> Regards,
>>>>>> Pierre
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>       ArmPkg/Library/ArmTrngLib/ArmTrngLib.c        |  5 -----
>>>>>>>>       .../RandomNumberGenerator/RngDxe/ArmRngDxe.c  | 18 +++++---
>>> ----------
>>>>>>>>       .../RngDxe/Rand/RngDxe.c                      |  9 ++++++++-
>>>>>>>>       .../RandomNumberGenerator/RngDxe/RngDxe.c     | 19
>>> ++++++++++++++-----
>>>>>>>>       4 files changed, 27 insertions(+), 24 deletions(-)
>>>>>>>>
>>>>>>>> --
>>>>>>>> 2.25.1
>>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>>> 
>>>>
>>>>

Re: [edk2-devel] [PATCH v3 0/4] ArmPkg/SecurityPkg: Fixes for ArmTrngLib/RngDxe

[Ard Biesheuvel]

On Mon, 6 Mar 2023 at 18:09, Pierre Gondois <pierre.gondois@arm.com> wrote:
>
> Hello Jiewen, Ard,
> Thanks for the review.
>
> On 3/6/23 17:22, Ard Biesheuvel wrote:
> > On Mon, 6 Mar 2023 at 16:42, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> >>
> >> Hi Pierre
> >> I don’t have strong opinion.
> >>
> >> For ARM specific patch, would you please get R-B from ARM expert?
> >>
> >> I think we need to wait for the response from Ard to confirm.
> >>
> >
> > These patches
> >
> >    SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount
> >    SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
> >
> > Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> >
> > Jiewen, if you don't mind, I will merge those right away.
> >
> > For the remaining patch, I am not sure I understand why the behavior
> > regarding the zero GUID is correct. Perhaps we could
> > revisit/resend/review that patch in isolation?
>
> About the zero GUID, the PcdCpuRngSupportedAlgorithm allows to describe
> the platform specific rng algorithm used. However KvmTool could run
> on any platform, so PcdCpuRngSupportedAlgorithm cannot be set to a proper
> GUID value.

OK so the problem is that we don't know which exact algorithm is being
used to back the RNDR/RNDRRS system registers?

In that case, we just invent a GUID and document it as 'unspecified
NIST SP800-90A Rev 1 conformant algorithm', and use that as the
default.

Then, we can treat the zero guid as 'not implemented', and ignore it.
That means not installing the RNG protocol at all if neither the
system register nor the hypercall based RNG is available.


> A zero GUID is not really compliant to the UEFI spec (s37.5.1 EFI RNG
> Algorithm Definitions), but I am not sure which other choice could be
> made,
>
> I'm not sure this was your question, please let know if it wasn't,
>

Re: [edk2-devel] [PATCH v3 0/4] ArmPkg/SecurityPkg: Fixes for ArmTrngLib/RngDxe

[Yao, Jiewen]

I don’t mind, please go ahead.

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> Biesheuvel
> Sent: Tuesday, March 7, 2023 12:22 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Pierre Gondois <pierre.gondois@arm.com>; devel@edk2.groups.io; Leif
> Lindholm <leif@nuviainc.com>; Sami Mujawar <sami.mujawar@arm.com>;
> Wang, Jian J <jian.j.wang@intel.com>
> Subject: Re: [edk2-devel] [PATCH v3 0/4] ArmPkg/SecurityPkg: Fixes for
> ArmTrngLib/RngDxe
> 
> On Mon, 6 Mar 2023 at 16:42, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> >
> > Hi Pierre
> > I don’t have strong opinion.
> >
> > For ARM specific patch, would you please get R-B from ARM expert?
> >
> > I think we need to wait for the response from Ard to confirm.
> >
> 
> These patches
> 
>   SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount
>   SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
> 
> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> 
> Jiewen, if you don't mind, I will merge those right away.
> 
> For the remaining patch, I am not sure I understand why the behavior
> regarding the zero GUID is correct. Perhaps we could
> revisit/resend/review that patch in isolation?
> 
> Thanks,
> Ard.
> 
> 
> 
> 
> >
> > > -----Original Message-----
> > > From: Pierre Gondois <pierre.gondois@arm.com>
> > > Sent: Monday, March 6, 2023 11:38 PM
> > > To: devel@edk2.groups.io
> > > Cc: Leif Lindholm <leif@nuviainc.com>; Sami Mujawar
> > > <sami.mujawar@arm.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Wang,
> > > Jian J <jian.j.wang@intel.com>; Ard Biesheuvel <ardb@kernel.org>
> > > Subject: Re: [edk2-devel] [PATCH v3 0/4] ArmPkg/SecurityPkg: Fixes for
> > > ArmTrngLib/RngDxe
> > >
> > > Hello Jiewen, Jian,
> > > Do these patches in this set look ok to you ?
> > > - SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount
> > > - SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
> > > - SecurityPkg/RngDxe: Fix Rng algo selection for Arm
> > >
> > > Regards,
> > > Pierre
> > >
> > > On 2/10/23 10:26, PierreGondois via groups.io wrote:
> > > > Hello Jiewen, Jian,
> > > > Just a reminder in case this was forgotten,
> > > > Regards,
> > > > Pierre
> > > >
> > > > On 1/9/23 13:26, Pierre Gondois wrote:
> > > >> Hello,
> > > >> I was wondering if I should re-arrange the patch in any way/there was
> any
> > > >> concern with the patch set. The first patch of serie was taken, but the
> > > other
> > > >> ones are pending,
> > > >>
> > > >> Regards,
> > > >> Pierre
> > > >>
> > > >> On 11/28/22 14:34, PierreGondois via groups.io wrote:
> > > >>> Hello Ard,
> > > >>>
> > > >>> On 11/26/22 15:33, Ard Biesheuvel wrote:
> > > >>>> On Thu, 24 Nov 2022 at 17:18, <Pierre.Gondois@arm.com> wrote:
> > > >>>>>
> > > >>>>> From: Pierre Gondois <pierre.gondois@arm.com>
> > > >>>>>
> > > >>>>> v1:
> > > >>>>> - https://edk2.groups.io/g/devel/message/96356
> > > >>>>> v2:
> > > >>>>> - https://edk2.groups.io/g/devel/message/96434
> > > >>>>> - Reformulate commit message.
> > > >>>>> - Do not warn if no algorithm is found as the message
> > > >>>>>       would be printed on non-Arm platforms.
> > > >>>>> v3:
> > > >>>>> - Add the following patches:
> > > >>>>>       1. ArmPkg/ArmTrngLib: Remove ASSERTs in
> > > ArmTrngLibConstructor()
> > > >>>>>          Requested by Ard.
> > > >>>>>          Cf https://edk2.groups.io/g/devel/message/96495
> > > >>>>>       2. SecurityPkg/RngDxe: Conditionally install
> EFI_RNG_PROTOCOL
> > > >>>>>          Do not install EFI_RNG_PROTOCOL if no RNG algorithm is
> > > available.
> > > >>>>>          Cf. https://edk2.groups.io/g/devel/message/96494
> > > >>>>>       3. SecurityPkg/RngDxe: Fix Rng algo selection for Arm
> > > >>>>>          Coming from v2 patch being split.
> > > >>>>>
> > > >>>>> Some issues were found by Ard/Sami on the RngDxe/ArmTrngLib
> after
> > > >>>>> recent patches were merged. This patch serie intends to fix them.
> > > >>>>>
> > > >>>>> Pierre Gondois (4):
> > > >>>>>       ArmPkg/ArmTrngLib: Remove ASSERTs in
> ArmTrngLibConstructor()
> > > >>>>
> > > >>>> Thanks for the fixed
> > > >>>>
> > > >>>> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> > > >>>>
> > > >>>> I pushed this one as #3663 (pending CI verification atm)
> > > >>>>
> > > >>>>>       SecurityPkg/RngDxe: Correctly update
> mAvailableAlgoArrayCount
> > > >>>>>       SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
> > > >>>>>       SecurityPkg/RngDxe: Fix Rng algo selection for Arm
> > > >>>>>
> > > >>>>
> > > >>>> The remaining code still looks a bit clunky to me. Can't we just
> > > >>>> return an error from the library constructor of the library cannot
> > > >>>> initialize due to a missing prerequisite?
> > > >>>
> > > >>> In RngDriverEntry(), GetAvailableAlgorithms() probe the available
> > > >>> RNG algorithms. If there are none, then we return an error code.
> > > >>> Otherwise we install EFI_RNG_PROTOCOL.
> > > >>>
> > > >>> I assume the prerequisite you a referring to is 'checking there is
> > > >>> at least one available RNG algorithm that RngDxe can use', so if
> > > >>> ArmTrngLib's constructor fails, we should not prevent RngDxe to be
> > > >>> loaded (as the RngLib could be used for instance).
> > > >>>
> > > >>> Does it answer your question, or did I understand it incorrectly ?
> > > >>>
> > > >>> Regards,
> > > >>> Pierre
> > > >>>
> > > >>>>
> > > >>>>
> > > >>>>>      ArmPkg/Library/ArmTrngLib/ArmTrngLib.c        |  5 -----
> > > >>>>>      .../RandomNumberGenerator/RngDxe/ArmRngDxe.c  | 18
> +++++---
> > > ----------
> > > >>>>>      .../RngDxe/Rand/RngDxe.c                      |  9 ++++++++-
> > > >>>>>      .../RandomNumberGenerator/RngDxe/RngDxe.c     | 19
> > > ++++++++++++++-----
> > > >>>>>      4 files changed, 27 insertions(+), 24 deletions(-)
> > > >>>>>
> > > >>>>> --
> > > >>>>> 2.25.1
> > > >>>>>
> > > >>>
> > > >>>
> > > >>>
> > > >>>
> > > >>>
> > > >
> > > >
> > > >
> > > >
> > > >
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v3 0/4] ArmPkg/SecurityPkg: Fixes for ArmTrngLib/RngDxe

[Ard Biesheuvel]

On Tue, 7 Mar 2023 at 08:53, Yao, Jiewen <jiewen.yao@intel.com> wrote:
>
> I don’t mind, please go ahead.
>

Thanks. Merged as #4109

Note that I replaced the 'return EFI_UNSUPPORTED' with 'return
EFI_REQUEST_UNLOAD_IMAGE' so that RngDxe is simply unloaded again
without an error if no RNG sources are available to it.


> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> > Biesheuvel
> > Sent: Tuesday, March 7, 2023 12:22 AM
> > To: Yao, Jiewen <jiewen.yao@intel.com>
> > Cc: Pierre Gondois <pierre.gondois@arm.com>; devel@edk2.groups.io; Leif
> > Lindholm <leif@nuviainc.com>; Sami Mujawar <sami.mujawar@arm.com>;
> > Wang, Jian J <jian.j.wang@intel.com>
> > Subject: Re: [edk2-devel] [PATCH v3 0/4] ArmPkg/SecurityPkg: Fixes for
> > ArmTrngLib/RngDxe
> >
> > On Mon, 6 Mar 2023 at 16:42, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> > >
> > > Hi Pierre
> > > I don’t have strong opinion.
> > >
> > > For ARM specific patch, would you please get R-B from ARM expert?
> > >
> > > I think we need to wait for the response from Ard to confirm.
> > >
> >
> > These patches
> >
> >   SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount
> >   SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
> >
> > Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> >
> > Jiewen, if you don't mind, I will merge those right away.
> >
> > For the remaining patch, I am not sure I understand why the behavior
> > regarding the zero GUID is correct. Perhaps we could
> > revisit/resend/review that patch in isolation?
> >
> > Thanks,
> > Ard.
> >
> >
> >
> >
> > >
> > > > -----Original Message-----
> > > > From: Pierre Gondois <pierre.gondois@arm.com>
> > > > Sent: Monday, March 6, 2023 11:38 PM
> > > > To: devel@edk2.groups.io
> > > > Cc: Leif Lindholm <leif@nuviainc.com>; Sami Mujawar
> > > > <sami.mujawar@arm.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> > Wang,
> > > > Jian J <jian.j.wang@intel.com>; Ard Biesheuvel <ardb@kernel.org>
> > > > Subject: Re: [edk2-devel] [PATCH v3 0/4] ArmPkg/SecurityPkg: Fixes for
> > > > ArmTrngLib/RngDxe
> > > >
> > > > Hello Jiewen, Jian,
> > > > Do these patches in this set look ok to you ?
> > > > - SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount
> > > > - SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
> > > > - SecurityPkg/RngDxe: Fix Rng algo selection for Arm
> > > >
> > > > Regards,
> > > > Pierre
> > > >
> > > > On 2/10/23 10:26, PierreGondois via groups.io wrote:
> > > > > Hello Jiewen, Jian,
> > > > > Just a reminder in case this was forgotten,
> > > > > Regards,
> > > > > Pierre
> > > > >
> > > > > On 1/9/23 13:26, Pierre Gondois wrote:
> > > > >> Hello,
> > > > >> I was wondering if I should re-arrange the patch in any way/there was
> > any
> > > > >> concern with the patch set. The first patch of serie was taken, but the
> > > > other
> > > > >> ones are pending,
> > > > >>
> > > > >> Regards,
> > > > >> Pierre
> > > > >>
> > > > >> On 11/28/22 14:34, PierreGondois via groups.io wrote:
> > > > >>> Hello Ard,
> > > > >>>
> > > > >>> On 11/26/22 15:33, Ard Biesheuvel wrote:
> > > > >>>> On Thu, 24 Nov 2022 at 17:18, <Pierre.Gondois@arm.com> wrote:
> > > > >>>>>
> > > > >>>>> From: Pierre Gondois <pierre.gondois@arm.com>
> > > > >>>>>
> > > > >>>>> v1:
> > > > >>>>> - https://edk2.groups.io/g/devel/message/96356
> > > > >>>>> v2:
> > > > >>>>> - https://edk2.groups.io/g/devel/message/96434
> > > > >>>>> - Reformulate commit message.
> > > > >>>>> - Do not warn if no algorithm is found as the message
> > > > >>>>>       would be printed on non-Arm platforms.
> > > > >>>>> v3:
> > > > >>>>> - Add the following patches:
> > > > >>>>>       1. ArmPkg/ArmTrngLib: Remove ASSERTs in
> > > > ArmTrngLibConstructor()
> > > > >>>>>          Requested by Ard.
> > > > >>>>>          Cf https://edk2.groups.io/g/devel/message/96495
> > > > >>>>>       2. SecurityPkg/RngDxe: Conditionally install
> > EFI_RNG_PROTOCOL
> > > > >>>>>          Do not install EFI_RNG_PROTOCOL if no RNG algorithm is
> > > > available.
> > > > >>>>>          Cf. https://edk2.groups.io/g/devel/message/96494
> > > > >>>>>       3. SecurityPkg/RngDxe: Fix Rng algo selection for Arm
> > > > >>>>>          Coming from v2 patch being split.
> > > > >>>>>
> > > > >>>>> Some issues were found by Ard/Sami on the RngDxe/ArmTrngLib
> > after
> > > > >>>>> recent patches were merged. This patch serie intends to fix them.
> > > > >>>>>
> > > > >>>>> Pierre Gondois (4):
> > > > >>>>>       ArmPkg/ArmTrngLib: Remove ASSERTs in
> > ArmTrngLibConstructor()
> > > > >>>>
> > > > >>>> Thanks for the fixed
> > > > >>>>
> > > > >>>> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> > > > >>>>
> > > > >>>> I pushed this one as #3663 (pending CI verification atm)
> > > > >>>>
> > > > >>>>>       SecurityPkg/RngDxe: Correctly update
> > mAvailableAlgoArrayCount
> > > > >>>>>       SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL
> > > > >>>>>       SecurityPkg/RngDxe: Fix Rng algo selection for Arm
> > > > >>>>>
> > > > >>>>
> > > > >>>> The remaining code still looks a bit clunky to me. Can't we just
> > > > >>>> return an error from the library constructor of the library cannot
> > > > >>>> initialize due to a missing prerequisite?
> > > > >>>
> > > > >>> In RngDriverEntry(), GetAvailableAlgorithms() probe the available
> > > > >>> RNG algorithms. If there are none, then we return an error code.
> > > > >>> Otherwise we install EFI_RNG_PROTOCOL.
> > > > >>>
> > > > >>> I assume the prerequisite you a referring to is 'checking there is
> > > > >>> at least one available RNG algorithm that RngDxe can use', so if
> > > > >>> ArmTrngLib's constructor fails, we should not prevent RngDxe to be
> > > > >>> loaded (as the RngLib could be used for instance).
> > > > >>>
> > > > >>> Does it answer your question, or did I understand it incorrectly ?
> > > > >>>
> > > > >>> Regards,
> > > > >>> Pierre
> > > > >>>
> > > > >>>>
> > > > >>>>
> > > > >>>>>      ArmPkg/Library/ArmTrngLib/ArmTrngLib.c        |  5 -----
> > > > >>>>>      .../RandomNumberGenerator/RngDxe/ArmRngDxe.c  | 18
> > +++++---
> > > > ----------
> > > > >>>>>      .../RngDxe/Rand/RngDxe.c                      |  9 ++++++++-
> > > > >>>>>      .../RandomNumberGenerator/RngDxe/RngDxe.c     | 19
> > > > ++++++++++++++-----
> > > > >>>>>      4 files changed, 27 insertions(+), 24 deletions(-)
> > > > >>>>>
> > > > >>>>> --
> > > > >>>>> 2.25.1
> > > > >>>>>
> > > > >>>
> > > > >>>
> > > > >>>
> > > > >>>
> > > > >>>
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> >
> >
> > 
> >
>