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.web11.284.1678122554333081039 for ; Mon, 06 Mar 2023 09:09:14 -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 208801063; Mon, 6 Mar 2023 09:09:57 -0800 (PST) Received: from [10.57.52.218] (unknown [10.57.52.218]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id AFD8F3F67D; Mon, 6 Mar 2023 09:09:12 -0800 (PST) Message-ID: Date: Mon, 6 Mar 2023 18:09:10 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1 Subject: Re: [edk2-devel] [PATCH v3 0/4] ArmPkg/SecurityPkg: Fixes for ArmTrngLib/RngDxe To: Ard Biesheuvel , "Yao, Jiewen" Cc: "devel@edk2.groups.io" , Leif Lindholm , Sami Mujawar , "Wang, Jian J" References: <20221124161756.216996-1-Pierre.Gondois@arm.com> <172BC2FE233A5592.368@groups.io> <19cafc02-6589-462c-e200-00018486bb1f@arm.com> <17426C66FE22D73E.5513@groups.io> <9404e123-ff27-f184-a63b-a587f842f428@arm.com> From: "PierreGondois" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Hello Jiewen, Ard, Thanks for the review. On 3/6/23 17:22, Ard Biesheuvel wrote: > On Mon, 6 Mar 2023 at 16:42, Yao, Jiewen wrote: >> >> Hi Pierre >> I don=E2=80=99t have strong opinion. >> >> For ARM specific patch, would you please get R-B from ARM expert? >> >> I think we need to wait for the response from Ard to confirm. >> >=20 > These patches >=20 > SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount > SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL >=20 > Reviewed-by: Ard Biesheuvel >=20 > Jiewen, if you don't mind, I will merge those right away. >=20 > For the remaining patch, I am not sure I understand why the behavior > regarding the zero GUID is correct. Perhaps we could > revisit/resend/review that patch in isolation? About the zero GUID, the PcdCpuRngSupportedAlgorithm allows to describe the platform specific rng algorithm used. However KvmTool could run on any platform, so PcdCpuRngSupportedAlgorithm cannot be set to a proper GUID value. A zero GUID is not really compliant to the UEFI spec (s37.5.1 EFI RNG Algorithm Definitions), but I am not sure which other choice could be made, I'm not sure this was your question, please let know if it wasn't, Regards, Pierre >=20 > Thanks, > Ard. >=20 >=20 >=20 >=20 >> >>> -----Original Message----- >>> From: Pierre Gondois >>> Sent: Monday, March 6, 2023 11:38 PM >>> To: devel@edk2.groups.io >>> Cc: Leif Lindholm ; Sami Mujawar >>> ; Yao, Jiewen ; Wang, >>> Jian J ; Ard Biesheuvel >>> Subject: Re: [edk2-devel] [PATCH v3 0/4] ArmPkg/SecurityPkg: Fixes for >>> ArmTrngLib/RngDxe >>> >>> Hello Jiewen, Jian, >>> Do these patches in this set look ok to you ? >>> - SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount >>> - SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL >>> - SecurityPkg/RngDxe: Fix Rng algo selection for Arm >>> >>> Regards, >>> Pierre >>> >>> On 2/10/23 10:26, PierreGondois via groups.io wrote: >>>> Hello Jiewen, Jian, >>>> Just a reminder in case this was forgotten, >>>> Regards, >>>> Pierre >>>> >>>> On 1/9/23 13:26, Pierre Gondois wrote: >>>>> Hello, >>>>> I was wondering if I should re-arrange the patch in any way/there was= any >>>>> concern with the patch set. The first patch of serie was taken, but t= he >>> other >>>>> ones are pending, >>>>> >>>>> Regards, >>>>> Pierre >>>>> >>>>> On 11/28/22 14:34, PierreGondois via groups.io wrote: >>>>>> Hello Ard, >>>>>> >>>>>> On 11/26/22 15:33, Ard Biesheuvel wrote: >>>>>>> On Thu, 24 Nov 2022 at 17:18, wrote: >>>>>>>> >>>>>>>> From: Pierre Gondois >>>>>>>> >>>>>>>> v1: >>>>>>>> - https://edk2.groups.io/g/devel/message/96356 >>>>>>>> v2: >>>>>>>> - https://edk2.groups.io/g/devel/message/96434 >>>>>>>> - Reformulate commit message. >>>>>>>> - Do not warn if no algorithm is found as the message >>>>>>>> would be printed on non-Arm platforms. >>>>>>>> v3: >>>>>>>> - Add the following patches: >>>>>>>> 1. ArmPkg/ArmTrngLib: Remove ASSERTs in >>> ArmTrngLibConstructor() >>>>>>>> Requested by Ard. >>>>>>>> Cf https://edk2.groups.io/g/devel/message/96495 >>>>>>>> 2. SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOC= OL >>>>>>>> Do not install EFI_RNG_PROTOCOL if no RNG algorithm is >>> available. >>>>>>>> Cf. https://edk2.groups.io/g/devel/message/96494 >>>>>>>> 3. SecurityPkg/RngDxe: Fix Rng algo selection for Arm >>>>>>>> Coming from v2 patch being split. >>>>>>>> >>>>>>>> Some issues were found by Ard/Sami on the RngDxe/ArmTrngLib after >>>>>>>> recent patches were merged. This patch serie intends to fix them. >>>>>>>> >>>>>>>> Pierre Gondois (4): >>>>>>>> ArmPkg/ArmTrngLib: Remove ASSERTs in ArmTrngLibConstructor(= ) >>>>>>> >>>>>>> Thanks for the fixed >>>>>>> >>>>>>> Reviewed-by: Ard Biesheuvel >>>>>>> >>>>>>> I pushed this one as #3663 (pending CI verification atm) >>>>>>> >>>>>>>> SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCou= nt >>>>>>>> SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL >>>>>>>> SecurityPkg/RngDxe: Fix Rng algo selection for Arm >>>>>>>> >>>>>>> >>>>>>> The remaining code still looks a bit clunky to me. Can't we just >>>>>>> return an error from the library constructor of the library cannot >>>>>>> initialize due to a missing prerequisite? >>>>>> >>>>>> In RngDriverEntry(), GetAvailableAlgorithms() probe the available >>>>>> RNG algorithms. If there are none, then we return an error code. >>>>>> Otherwise we install EFI_RNG_PROTOCOL. >>>>>> >>>>>> I assume the prerequisite you a referring to is 'checking there is >>>>>> at least one available RNG algorithm that RngDxe can use', so if >>>>>> ArmTrngLib's constructor fails, we should not prevent RngDxe to be >>>>>> loaded (as the RngLib could be used for instance). >>>>>> >>>>>> Does it answer your question, or did I understand it incorrectly ? >>>>>> >>>>>> Regards, >>>>>> Pierre >>>>>> >>>>>>> >>>>>>> >>>>>>>> ArmPkg/Library/ArmTrngLib/ArmTrngLib.c | 5 ----- >>>>>>>> .../RandomNumberGenerator/RngDxe/ArmRngDxe.c | 18 +++++--- >>> ---------- >>>>>>>> .../RngDxe/Rand/RngDxe.c | 9 ++++++++- >>>>>>>> .../RandomNumberGenerator/RngDxe/RngDxe.c | 19 >>> ++++++++++++++----- >>>>>>>> 4 files changed, 27 insertions(+), 24 deletions(-) >>>>>>>> >>>>>>>> -- >>>>>>>> 2.25.1 >>>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>> >>>> >>>>=20 >>>> >>>>