* [PATCH 1/1][edk2-stable202211] SecurityPkg/RngDxe: Fix Rng algo selection for Arm @ 2022-11-14 18:13 PierreGondois 2022-11-14 19:12 ` [edk2-devel] " Ard Biesheuvel 0 siblings, 1 reply; 4+ messages in thread From: PierreGondois @ 2022-11-14 18:13 UTC (permalink / raw) To: devel; +Cc: Sami Mujawar, Ard Biesheuvel, Liming Gao, Jiewen Yao, Jian J Wang From: Pierre Gondois <pierre.gondois@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. Reported-by: Sami Mujawar <sami.mujawar@arm.com> Signed-off-by: Pierre Gondois <Pierre.Gondois@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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH 1/1][edk2-stable202211] SecurityPkg/RngDxe: Fix Rng algo selection for Arm 2022-11-14 18:13 [PATCH 1/1][edk2-stable202211] SecurityPkg/RngDxe: Fix Rng algo selection for Arm PierreGondois @ 2022-11-14 19:12 ` Ard Biesheuvel 2022-11-15 8:27 ` PierreGondois 0 siblings, 1 reply; 4+ messages in thread From: Ard Biesheuvel @ 2022-11-14 19:12 UTC (permalink / raw) To: devel, pierre.gondois Cc: Sami Mujawar, Ard Biesheuvel, Liming Gao, Jiewen Yao, Jian J Wang On Mon, 14 Nov 2022 at 19:14, PierreGondois <pierre.gondois@arm.com> wrote: > > From: Pierre Gondois <pierre.gondois@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? > Reported-by: Sami Mujawar <sami.mujawar@arm.com> > Signed-off-by: Pierre Gondois <Pierre.Gondois@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@edk2.groups.io > Unsubscribe: https://edk2.groups.io/g/devel/unsub [ardb+tianocore@kernel.org] > ------------ > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH 1/1][edk2-stable202211] SecurityPkg/RngDxe: Fix Rng algo selection for Arm 2022-11-14 19:12 ` [edk2-devel] " Ard Biesheuvel @ 2022-11-15 8:27 ` PierreGondois 2022-11-16 15:04 ` PierreGondois 0 siblings, 1 reply; 4+ messages in thread From: PierreGondois @ 2022-11-15 8:27 UTC (permalink / raw) To: Ard Biesheuvel, devel Cc: Sami Mujawar, Ard Biesheuvel, Liming Gao, Jiewen Yao, Jian J Wang On 11/14/22 20:12, Ard Biesheuvel wrote: > On Mon, 14 Nov 2022 at 19:14, PierreGondois <pierre.gondois@arm.com> wrote: >> >> From: Pierre Gondois <pierre.gondois@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@arm.com> >> Signed-off-by: Pierre Gondois <Pierre.Gondois@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@edk2.groups.io >> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ardb+tianocore@kernel.org] >> ------------ >> >> ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH 1/1][edk2-stable202211] SecurityPkg/RngDxe: Fix Rng algo selection for Arm 2022-11-15 8:27 ` PierreGondois @ 2022-11-16 15:04 ` PierreGondois 0 siblings, 0 replies; 4+ messages in thread From: PierreGondois @ 2022-11-16 15:04 UTC (permalink / raw) To: PierreGondois, devel [-- Attachment #1: Type: text/plain, Size: 206 bytes --] Hello, Please ignore this version as the warning message added in this patch would have been printed on non-Arm platforms. A v2 is at: https://edk2.groups.io/g/devel/message/96434 Regards, Pierre [-- Attachment #2: Type: text/html, Size: 328 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-11-16 15:04 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-11-14 18:13 [PATCH 1/1][edk2-stable202211] SecurityPkg/RngDxe: Fix Rng algo selection for Arm PierreGondois 2022-11-14 19:12 ` [edk2-devel] " Ard Biesheuvel 2022-11-15 8:27 ` PierreGondois 2022-11-16 15:04 ` PierreGondois
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox