From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web11.38280.1678119751796748231 for ; Mon, 06 Mar 2023 08:22:32 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=kTtSO33L; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 054BE61041 for ; Mon, 6 Mar 2023 16:22:31 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id CC2EDC433EF for ; Mon, 6 Mar 2023 16:22:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1678119750; bh=clKG28pGvKI2gwXgCGFyUOv/Gpj2KdcbEvOp1GmiQeQ=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=kTtSO33LknN62M3PDCqw/2I7otv4j4RNivCy/WhafzZ8rkOfD2j8YIhwsvnS1OuzQ 9z6pMDPHN1xSioqFcJlrfN/RNW28i+HS+vdrMX38tSuTb/g+oPuYLLvroR6o92QjSs txTfRynFIrsicX5iN1kCsJMcmbgdqMQmOO8Xy+aqqSHZoVh51MXccahXXazTErT02d wH75LNk7U9LtB46EdXlCwH9IaU7s+FXFNhS37K2yYkWedxvJcpug0SHdmbRo2OdeZO dGjGtPhxBVWSC1ohPgEt+g6d3Jjt1cP45uWleIb9ALRJsWhjgeMyw0Xu7+fim2xZ0G U8QHnBAOi9YEg== Received: by mail-lj1-f175.google.com with SMTP id t14so10240663ljd.5 for ; Mon, 06 Mar 2023 08:22:30 -0800 (PST) X-Gm-Message-State: AO0yUKXaKF43fBZmqNhnM5H28mwCtIktdPDcGFHnZgPW60wiFNkFfHDD 89ui/sDVPkt520oAuHTTeAC9hdQgjIfxaEoZobg= X-Google-Smtp-Source: AK7set/c5wK7f8oJ8AOw1jEJ4UBL+Qfstp09wxWQMus7RGT/HmNWIOrcsApeXkQpdt4xggDlr1p04Cogwlg29jVaHbg= X-Received: by 2002:a2e:8341:0:b0:293:4ba5:f626 with SMTP id l1-20020a2e8341000000b002934ba5f626mr5974862ljh.2.1678119748776; Mon, 06 Mar 2023 08:22:28 -0800 (PST) MIME-Version: 1.0 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> In-Reply-To: From: "Ard Biesheuvel" Date: Mon, 6 Mar 2023 17:22:17 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH v3 0/4] ArmPkg/SecurityPkg: Fixes for ArmTrngLib/RngDxe To: "Yao, Jiewen" Cc: Pierre Gondois , "devel@edk2.groups.io" , Leif Lindholm , Sami Mujawar , "Wang, Jian J" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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. > These patches SecurityPkg/RngDxe: Correctly update mAvailableAlgoArrayCount SecurityPkg/RngDxe: Conditionally install EFI_RNG_PROTOCOL Reviewed-by: Ard Biesheuvel Jiewen, if you don't mind, I will merge those right away. 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? Thanks, Ard. > > > -----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 wa= s any > > >> concern with the patch set. The first patch of serie was taken, but = the > > 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 > > > > > >