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

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

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