[edk2-devel] [PATCH 1/1][edk2-stable202211] SecurityPkg/RngDxe: Fix Rng algo selection for Arm

[PierreGondois]

On 11/14/22 20:12, Ard Biesheuvel wrote:
> On Mon, 14 Nov 2022 at 19:14, PierreGondois <pierre.gondois at arm.com> wrote:
>>
>> From: Pierre Gondois <pierre.gondois at arm.com>
>>
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4151
>>
>> PcdCpuRngSupportedAlgorithm is set to the Zero Guid for KvmTool
>> since it is not possible to predict which algorithm will be
>> implemented for Arm's FEAT_RNG in the host. Current handling is:
>> - skipping the Zero Guid, which should not happen to handle
>>    KvmTool's case,
>> - triggering an ASSERT if no algorithm was found. However having
>>    no Rng algorithm is a valid case,
>>
>> Correctly handle the Zero Guid case and replace the ASSERT by a
>> warning message when no Rng 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.
>>
> 
> Can you explain a bit more please
> 
> What is supposed to happen?
> 
> What happens instead?
> 
> Why is this patch the correct way to address this issue?

Hello Ard,
mAvailableAlgoArray in RngDxe stores the availble Rng algorithms.
The array is populated in GetAvailableAlgorithms() by testing the
possible algorithms. This is done when the driver is loaded.
PcdCpuRngSupportedAlgorithm is the Guid used to identify the algorithm
behind the RngLib, i.e. the RNDR instruction. It is set to a zero Guid
for KvmTool since it isn't possible to set a unique value for all the
hosts.
When using RngDxe, it is possible to either specify which Rng algorithm
to use or to use a default one. When requesting the default one, the
first element of mAvailableAlgoArray should be used. This is an
arbitrary decision, but mAvailableAlgoArray will always be populated
in the same the order.

On arm64, the 2 possible algorithms are, in order:
- the RngLib, which should use the RNDR instruction
- the TRNG
If there is no TRNG, the only Guid available will be the one for RNDR,
i.e. PcdCpuRngSupportedAlgorithm (zero Guid).

The current code:
- skips the zero Guid, which should not be the case since it represents
   a valid algorithm,
- ASSERT if no algorithm is found. If no algorithm is found, we
   should not stop, just return an error code, and the caller will act
   on this error. A warning is however added in case there is no
   algorithm found,

Regards,
Pierre


> 
> 
>> Reported-by: Sami Mujawar <sami.mujawar at arm.com>
>> Signed-off-by: Pierre Gondois <Pierre.Gondois at arm.com>
>> ---
>>   .../RandomNumberGenerator/RngDxe/ArmRngDxe.c      | 15 +++------------
>>   SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.c |  8 +++++++-
>>   2 files changed, 10 insertions(+), 13 deletions(-)
>>
>> diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c b/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
>> index 5ba319899ce9..722d53386373 100644
>> --- a/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
>> +++ b/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
>> @@ -76,7 +76,6 @@ RngGetRNG (
>>     )
>>   {
>>     EFI_STATUS  Status;
>> -  UINTN       Index;
>>
>>     if ((This == NULL) || (RNGValueLength == 0) || (RNGValue == NULL)) {
>>       return EFI_INVALID_PARAMETER;
>> @@ -86,21 +85,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);
>> +    if (mAvailableAlgoArrayCount != 0) {
>> +      RNGAlgorithm = &mAvailableAlgoArray[0];
>> +    } else {
>>         return EFI_DEVICE_ERROR;
>>       }
>>     }
>>
>> -FoundAlgo:
>>     if (CompareGuid (RNGAlgorithm, PcdGetPtr (PcdCpuRngSupportedAlgorithm))) {
>>       Status = RngGetBytes (RNGValueLength, RNGValue);
>>       return Status;
>> diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.c b/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.c
>> index 421abb52b8bf..403b31b73609 100644
>> --- a/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.c
>> +++ b/SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.c
>> @@ -21,6 +21,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>>
>>   #include <Library/BaseLib.h>
>>   #include <Library/BaseMemoryLib.h>
>> +#include <Library/DebugLib.h>
>>   #include <Library/UefiBootServicesTableLib.h>
>>   #include <Library/RngLib.h>
>>   #include <Protocol/Rng.h>
>> @@ -80,7 +81,12 @@ RngDriverEntry (
>>     //
>>     // Get the list of available algorithm.
>>     //
>> -  return GetAvailableAlgorithms ();
>> +  Status = GetAvailableAlgorithms ();
>> +  if (mAvailableAlgoArrayCount == 0) {
>> +    DEBUG ((DEBUG_WARN, "No Rng algorithm found in RngDxe.\n"));
>> +  }
>> +
>> +  return Status;
>>   }
>>
>>   /**
>> --
>> 2.25.1
>>
>>
>>
>> ------------
>> Groups.io Links: You receive all messages sent to this group.
>> View/Reply Online (#96356): https://edk2.groups.io/g/devel/message/96356
>> Mute This Topic: https://groups.io/mt/95025606/5717338
>> Group Owner: devel+owner at edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ardb+tianocore at kernel.org]
>> ------------
>>
>>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#96389): https://edk2.groups.io/g/devel/message/96389
Mute This Topic: https://groups.io/mt/95025606/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-