From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web10.3424.1668500835193687903 for ; Tue, 15 Nov 2022 00:27:15 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: pierre.gondois@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id C257711FB; Tue, 15 Nov 2022 00:27:20 -0800 (PST) Received: from [10.34.100.128] (pierre123.nice.arm.com [10.34.100.128]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 752973F587; Tue, 15 Nov 2022 00:27:13 -0800 (PST) Message-ID: Date: Tue, 15 Nov 2022 09:27:07 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.2 Subject: Re: [edk2-devel] [PATCH 1/1][edk2-stable202211] SecurityPkg/RngDxe: Fix Rng algo selection for Arm To: Ard Biesheuvel , devel@edk2.groups.io Cc: Sami Mujawar , Ard Biesheuvel , Liming Gao , Jiewen Yao , Jian J Wang References: <20221114181351.1813748-1-Pierre.Gondois@arm.com> From: "PierreGondois" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 11/14/22 20:12, Ard Biesheuvel wrote: > On Mon, 14 Nov 2022 at 19:14, PierreGondois wrote: >> >> From: Pierre Gondois >> >> 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 >> Signed-off-by: Pierre Gondois >> --- >> .../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 >> #include >> +#include >> #include >> #include >> #include >> @@ -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@edk2.groups.io >> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ardb+tianocore@kernel.org] >> ------------ >> >>