SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c | 9 +++++---- SecurityPkg/RandomNumberGenerator/RngDxe/Arm/ArmAlgo.c | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-)
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4491 On an ARM system that does not support firmware TRNG, the current logic from RngDxe will cause the system to assert at the below line: `ASSERT (Index != mAvailableAlgoArrayCount);` The reason seems to be: 1. When initializing the number of `mAvailableAlgoArrayCount`, the logic will only treat the zero guid of "PcdCpuRngSupportedAlgorithm" as a warning and still increment the counter because "RngGetBytes" might still succeed: https://github.com/tianocore/edk2/blob/1a39bdf2c53858ebb39e6de1362203c65c163c63/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c#L51C3-L51C3. 2. This will cause the main entry to publish the RNG protocol and accept further usage. 3. However, during usage, the zero guid is always filtered out: https://github.com/tianocore/edk2/blob/1a39bdf2c53858ebb39e6de1362203c65c163c63/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c#L91. Thus, this will cause the system to always not able to find the algorithm and fail the boot with an assert. The suggestion is to at least make the logic of initializing "mAvailableAlgoArrayCount" consistent and filtering algorithm consistent. In addition, the usage of `mAvailableAlgoArray` will always trigger a data abortion error, which is caused by buffer allocated is `RNG_AVAILABLE_ALGO_MAX` number of bytes, which should be `RNG_AVAILABLE_ALGO_MAX` nubmer of EFI_RNG_ALGORITHM. This patch fixed the 2 issues above. The change is verified on QEMU virtual platform and proprietary physical platform. Patch v1 branch: https://github.com/kuqin12/edk2/tree/fix_rng_edk2_v1 Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Sami Mujawar <Sami.Mujawar@arm.com> Cc: Pierre Gondois <pierre.gondois@arm.com> Kun Qin (2): SecurityPkg: RngDxe: Unify handling of zero guid SecurityPkg: RngDxe: Fixing mAvailableAlgoArray allocator SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c | 9 +++++---- SecurityPkg/RandomNumberGenerator/RngDxe/Arm/ArmAlgo.c | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) -- 2.41.0.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#106483): https://edk2.groups.io/g/devel/message/106483 Mute This Topic: https://groups.io/mt/99839045/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hello Kun, Thanks for the patch-set, there is another patch-set that also aims to fix the logic at: https://edk2.groups.io/g/devel/message/104341 but I haven't got feedback so far. Would it be possible to try it out to see if it also solves your issue ? Regards, Pierre On 6/28/23 22:33, Kun Qin wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4491 > > On an ARM system that does not support firmware TRNG, the current logic > from RngDxe will cause the system to assert at the below line: > `ASSERT (Index != mAvailableAlgoArrayCount);` > > The reason seems to be: > 1. When initializing the number of `mAvailableAlgoArrayCount`, the logic > will only treat the zero guid of "PcdCpuRngSupportedAlgorithm" as a > warning and still increment the counter because "RngGetBytes" might still > succeed: > https://github.com/tianocore/edk2/blob/1a39bdf2c53858ebb39e6de1362203c65c163c63/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c#L51C3-L51C3. > 2. This will cause the main entry to publish the RNG protocol and accept > further usage. > 3. However, during usage, the zero guid is always filtered out: > https://github.com/tianocore/edk2/blob/1a39bdf2c53858ebb39e6de1362203c65c163c63/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c#L91. > Thus, this will cause the system to always not able to find the algorithm > and fail the boot with an assert. > > The suggestion is to at least make the logic of initializing > "mAvailableAlgoArrayCount" consistent and filtering algorithm consistent. > > In addition, the usage of `mAvailableAlgoArray` will always trigger a > data abortion error, which is caused by buffer allocated is > `RNG_AVAILABLE_ALGO_MAX` number of bytes, which should be > `RNG_AVAILABLE_ALGO_MAX` nubmer of EFI_RNG_ALGORITHM. > > This patch fixed the 2 issues above. The change is verified on QEMU > virtual platform and proprietary physical platform. > > Patch v1 branch: https://github.com/kuqin12/edk2/tree/fix_rng_edk2_v1 > > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > Cc: Sami Mujawar <Sami.Mujawar@arm.com> > Cc: Pierre Gondois <pierre.gondois@arm.com> > > Kun Qin (2): > SecurityPkg: RngDxe: Unify handling of zero guid > SecurityPkg: RngDxe: Fixing mAvailableAlgoArray allocator > > SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c | 9 +++++---- > SecurityPkg/RandomNumberGenerator/RngDxe/Arm/ArmAlgo.c | 2 +- > 2 files changed, 6 insertions(+), 5 deletions(-) > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#106496): https://edk2.groups.io/g/devel/message/106496 Mute This Topic: https://groups.io/mt/99839045/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi Pierre/Sami, Thanks for the information. I tried the patch set you referred, it seems to have resolved the zero GUID handling issue! So I am okay to drop the commit here: https://edk2.groups.io/g/devel/message/106484 I also have a few questions on the patch set beyond functionality, we can discuss it there. However, this specific patch is still needed to fix the access violation issue (thanks for the reviewed-by tag, Sami): https://edk2.groups.io/g/devel/message/106485 I can send a v2 to reflect the feedback above. Thanks, Kun On 6/28/2023 11:43 PM, Pierre Gondois wrote: > Hello Kun, > > Thanks for the patch-set, there is another patch-set that also aims to > fix the logic at: > https://edk2.groups.io/g/devel/message/104341 > > but I haven't got feedback so far. Would it be possible to try it out > to see if > it also solves your issue ? > > Regards, > Pierre > > On 6/28/23 22:33, Kun Qin wrote: >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4491 >> >> On an ARM system that does not support firmware TRNG, the current logic >> from RngDxe will cause the system to assert at the below line: >> `ASSERT (Index != mAvailableAlgoArrayCount);` >> >> The reason seems to be: >> 1. When initializing the number of `mAvailableAlgoArrayCount`, the logic >> will only treat the zero guid of "PcdCpuRngSupportedAlgorithm" as a >> warning and still increment the counter because "RngGetBytes" might >> still >> succeed: >> https://github.com/tianocore/edk2/blob/1a39bdf2c53858ebb39e6de1362203c65c163c63/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c#L51C3-L51C3. >> >> 2. This will cause the main entry to publish the RNG protocol and accept >> further usage. >> 3. However, during usage, the zero guid is always filtered out: >> https://github.com/tianocore/edk2/blob/1a39bdf2c53858ebb39e6de1362203c65c163c63/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c#L91. >> >> Thus, this will cause the system to always not able to find the >> algorithm >> and fail the boot with an assert. >> >> The suggestion is to at least make the logic of initializing >> "mAvailableAlgoArrayCount" consistent and filtering algorithm >> consistent. >> >> In addition, the usage of `mAvailableAlgoArray` will always trigger a >> data abortion error, which is caused by buffer allocated is >> `RNG_AVAILABLE_ALGO_MAX` number of bytes, which should be >> `RNG_AVAILABLE_ALGO_MAX` nubmer of EFI_RNG_ALGORITHM. >> >> This patch fixed the 2 issues above. The change is verified on QEMU >> virtual platform and proprietary physical platform. >> >> Patch v1 branch: https://github.com/kuqin12/edk2/tree/fix_rng_edk2_v1 >> >> Cc: Jiewen Yao <jiewen.yao@intel.com> >> Cc: Jian J Wang <jian.j.wang@intel.com> >> Cc: Sami Mujawar <Sami.Mujawar@arm.com> >> Cc: Pierre Gondois <pierre.gondois@arm.com> >> >> Kun Qin (2): >> SecurityPkg: RngDxe: Unify handling of zero guid >> SecurityPkg: RngDxe: Fixing mAvailableAlgoArray allocator >> >> SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c | 9 >> +++++---- >> SecurityPkg/RandomNumberGenerator/RngDxe/Arm/ArmAlgo.c | 2 +- >> 2 files changed, 6 insertions(+), 5 deletions(-) >> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#106528): https://edk2.groups.io/g/devel/message/106528 Mute This Topic: https://groups.io/mt/99839045/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.