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


PierreGondois
 

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

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(-)

--=20
2.25.1


Ard Biesheuvel
 

On Thu, 24 Nov 2022 at 17:18, <Pierre.Gondois@...> wrote:

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

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@...>

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


PierreGondois
 

Hello Ard,

On 11/26/22 15:33, Ard Biesheuvel wrote:
On Thu, 24 Nov 2022 at 17:18, <Pierre.Gondois@...> wrote:

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

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@...>
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


Sami Mujawar
 

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);

}


Sami Mujawar
 

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


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@...> wrote:

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

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@...>

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