From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::244; helo=mail-it0-x244.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x244.google.com (mail-it0-x244.google.com [IPv6:2607:f8b0:4001:c0b::244]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id DB592211AFF2F for ; Mon, 11 Jun 2018 07:06:53 -0700 (PDT) Received: by mail-it0-x244.google.com with SMTP id 188-v6so10037825ita.5 for ; Mon, 11 Jun 2018 07:06:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=CdV+DVZIcz43hFBmANjBrOPxcOEaQRXDoOEjOe6DLMM=; b=Ms4bB/+99WU9ikqThgTToV5ZHjAIJ7PkhYjvWCrBQQAD0Hpy78sTEPVbpbKxAR/nvC SaWwcmVguXbbpd/wkIMSTdk2LsNpc/oYC1K9um8M1DljxFrZvWR8kn9Gr/ZgNeqJOKtE IzierVM0xPnhAxv0TeLz3WQLIER5r0ldz1X7E= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=CdV+DVZIcz43hFBmANjBrOPxcOEaQRXDoOEjOe6DLMM=; b=jI7U9wQYi+aGWjpUytXxwDDbXcYm6sa8qhbtlVlkzYpMrcY1gFm1pv7MnZ5zlnLrIB dK5U75uCxdbbFundGSkB+zxwCASNvEFTeoartBefBs6zL8BQRAqmG2JcDvH4CIkmn3zk 11rc0gc8gARKm0Sq6LUBY5jlJyUZJu5q1JUZ7Vc6gxNVZNeKSJN3db+Foavddvazj78L yJ2WiGr1sNQNocGLzc2YWd3A0kFtQJM0DuIIlNdOSzalVSF5ojionyWqVS93rGMs3Zkq lWV+k7pqnQpBdFbv0GaIepJ+RuLslrZOLiN76cD5B+8NpIRfULQazhhf9Wkfxx+aa3Eg M7PQ== X-Gm-Message-State: APt69E3+xIcMwXRy8M3yQJcPrecnvfqHSScaDdyfFXolGQ03oKQZoWdj KbzWdO5Y+PAoDoWNGpRZVINzHB3647AiKq6rFV75wg== X-Google-Smtp-Source: ADUXVKJUGDR9DLkifJZg7ej0Q3ik8Cwlb8apK0KgbnWhv5FU1N232LRxTPnBA+3jPcInE9ZCk16DmR8rDYC5Q5NaFMs= X-Received: by 2002:a24:e105:: with SMTP id n5-v6mr10530124ith.68.1528726012706; Mon, 11 Jun 2018 07:06:52 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:bbc7:0:0:0:0:0 with HTTP; Mon, 11 Jun 2018 07:06:51 -0700 (PDT) In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C503AC3CBE3@shsmsx102.ccr.corp.intel.com> References: <20180608065811.2065-1-ard.biesheuvel@linaro.org> <20180608065811.2065-3-ard.biesheuvel@linaro.org> <74D8A39837DF1E4DA445A8C0B3885C503AC3515B@shsmsx102.ccr.corp.intel.com> <6534F306-C3E7-40D2-84F4-D409BF1F7B68@linaro.org> <74D8A39837DF1E4DA445A8C0B3885C503AC3A387@shsmsx102.ccr.corp.intel.com> <74D8A39837DF1E4DA445A8C0B3885C503AC3ADC8@shsmsx102.ccr.corp.intel.com> <74D8A39837DF1E4DA445A8C0B3885C503AC3ADF5@shsmsx102.ccr.corp.intel.com> <87F9382F-EBA0-4715-B323-FB0C6F8DE5A2@intel.com> <74D8A39837DF1E4DA445A8C0B3885C503AC3CBE3@shsmsx102.ccr.corp.intel.com> From: Ard Biesheuvel Date: Mon, 11 Jun 2018 16:06:51 +0200 Message-ID: To: "Yao, Jiewen" Cc: "Kinney, Michael D" , "edk2-devel@lists.01.org" , "Zeng, Star" , "leif.lindholm@linaro.org" Subject: Re: [PATCH v2 2/5] MdeModulePkg/DxeCapsuleLibFmp: permit ProcessCapsules () to be called once X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 11 Jun 2018 14:06:54 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On 11 June 2018 at 15:55, Yao, Jiewen wrote: > Ah. Good point. > > I think the embedded driver dispatch can be controlled by EndOfDxe, which= is similar to MdeModulePkg\Universal\SecurityStubDxe. > > So we can have both EndOfDxe and PcdSystemFmpLockEventGuid. > EndOfDxe controls embedded driver dispatch. > PcdSystemFmpLockEventGuid controls 1st call and 2nd call. > But in our case, we don't need an event at all. The BDS just calls ProcessCapsules() whenever it wants to. I'm rather relucant to complicate the code with an additional event that we don't really need, while retaining some of the EndOfDxe handling as well. Could you think of another way to handle this use case? Thanks, Ard. >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of A= rd >> Biesheuvel >> Sent: Monday, June 11, 2018 5:40 AM >> To: Yao, Jiewen >> Cc: Kinney, Michael D ; edk2-devel@lists.01.= org; >> Zeng, Star ; leif.lindholm@linaro.org >> Subject: Re: [edk2] [PATCH v2 2/5] MdeModulePkg/DxeCapsuleLibFmp: permit >> ProcessCapsules () to be called once >> >> On 11 June 2018 at 14:37, Yao, Jiewen wrote: >> > If all fmp can be processed one time=EF=BC=8Cyou just need call once. = Then system >> reset. >> > >> >> I understand that. But if the capsule has embedded drivers, the >> library will only dispatch them on the second call, even if the >> platform does not require making that distinction. It seems to me that >> the phasing between the first and the second call is closely tied to >> the semantics of EndOfDxe and the associated policy on X86 not to >> allow third party code before it has been signalled. >> >> Could we not parameterize this in some way as well? >> >> >> > >> >> =E5=9C=A8 2018=E5=B9=B46=E6=9C=8811=E6=97=A5=EF=BC=8C=E4=B8=8A=E5=8D= =8812:27=EF=BC=8CArd Biesheuvel >> =E5=86=99=E9=81=93=EF=BC=9A >> >> >> >>> On 10 June 2018 at 21:21, Yao, Jiewen wrote: >> >>> Just got some idea since I am also reviewing FmpDevicePkg patch. >> >>> >> >>> >> >>> The key problem we are trying to resolve that is: The core code uses >> EndOfDxe as the lock event for system firmware, but an ARM platform may = want >> to lock system firmware later, maybe in other lock event. >> >>> >> >>> As such, how about we generalize the lock event as >> PcdSystemFmpLockEventGuid? We can use EndOfDxeGuid as default one to kee= p >> the compatibility. At same time this brings the flexibility for platform= overriding. >> >>> >> >>> >> >>> We only need update DxeCapsuleLibConstructor() to use the new PcdGui= d, >> instead of hardcode EndOfDxe. >> >>> We don't need update the logic in ProcessCapsules() and >> ProcessTheseCapsules(). >> >>> The policy in current platform is already enforced, if the platform = has a >> trusted console. >> >>> For ARM platform, which wants to lock system firmware later. It can >> configure another lock event GUID explicitly in platform DSC. >> >>> >> >> >> >> But that means we are still required to call ProcessCapsules () twice= , no? >> >> >> >>>> -----Original Message----- >> >>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf= Of >> Yao, >> >>>> Jiewen >> >>>> Sent: Sunday, June 10, 2018 12:02 PM >> >>>> To: Kinney, Michael D ; Ard Biesheuvel >> >>>> >> >>>> Cc: edk2-devel@lists.01.org; leif.lindholm@linaro.org; Zeng, Star >> >>>> >> >>>> Subject: Re: [edk2] [PATCH v2 2/5] MdeModulePkg/DxeCapsuleLibFmp: >> permit >> >>>> ProcessCapsules () to be called once >> >>>> >> >>>> My concern is that *always allowing* processing SystemCapsule after >> EndOfDxe >> >>>> has security risk. >> >>>> >> >>>> IMHO, the risk is not *process*, if it is a platform choice. Instea= d, the risk is >> >>>> *allow*, in the core logic. >> >>>> >> >>>> If we really want to do that, I hope we need a way to distinguish t= he >> difference >> >>>> between a platform dispatching SystemCapsule after EndOfDxe >> *purposely* and >> >>>> a platform dispatching SystemCapsule after EndOfDxe *by mistake*. >> >>>> >> >>>> Maybe some policy enforcement in the core logic. Static policy, at = build >> time. >> >>>> >> >>>> Thank you >> >>>> Yao Jiewen >> >>>> >> >>>>> -----Original Message----- >> >>>>> From: Kinney, Michael D >> >>>>> Sent: Sunday, June 10, 2018 8:57 AM >> >>>>> To: Ard Biesheuvel ; Yao, Jiewen >> >>>>> ; Kinney, Michael D >> >> >>>>> Cc: edk2-devel@lists.01.org; Zeng, Star ; >> >>>>> leif.lindholm@linaro.org >> >>>>> Subject: RE: [edk2] [PATCH v2 2/5] MdeModulePkg/DxeCapsuleLibFmp: >> permit >> >>>>> ProcessCapsules () to be called once >> >>>>> >> >>>>> Ard, >> >>>>> >> >>>>> I agree it should be a platform choice to process capsules >> >>>>> before or after End of DXE. It is even allowed in the >> >>>>> UEFI Spec for capsules to be processed immediately >> >>>>> which would include at OS runtime after ExitBootServices. >> >>>>> >> >>>>> There are 2 inputs to these choices: >> >>>>> * The flags set in the Capsule itself. See UEFI 2.7 Spec >> >>>>> Table 38 for the 5 allowed combinations. >> >>>>> * Platform policy for each of these 5 capsule types and >> >>>>> when each of these 5 capsule types are allowed to be >> >>>>> processed. >> >>>>> >> >>>>> Jiewen's comments point to some assumptions that have been >> >>>>> made in the logic. These assumptions may be considered a >> >>>>> safe default platform policy, but the code logic should >> >>>>> allow the platform to easily select alternate policies. >> >>>>> >> >>>>> I think the patch you have provided attempts to add one >> >>>>> additional policy. Perhaps we should look at this from >> >>>>> the UEFI Spec perspective and see how difficult it is to >> >>>>> express policies for the supported capsule types. >> >>>>> >> >>>>> I will review your patch in more detail tomorrow. >> >>>>> >> >>>>> Thanks, >> >>>>> >> >>>>> Mike >> >>>>> >> >>>>>> -----Original Message----- >> >>>>>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >> >>>>>> Sent: Saturday, June 9, 2018 10:42 PM >> >>>>>> To: Yao, Jiewen >> >>>>>> Cc: edk2-devel@lists.01.org; Kinney, Michael D >> >>>>>> ; Zeng, Star >> >>>>>> ; leif.lindholm@linaro.org >> >>>>>> Subject: Re: [edk2] [PATCH v2 2/5] >> >>>>>> MdeModulePkg/DxeCapsuleLibFmp: permit ProcessCapsules >> >>>>>> () to be called once >> >>>>>> >> >>>>>> On 10 June 2018 at 07:38, Yao, Jiewen >> >>>>>> wrote: >> >>>>>>> Hi Ard >> >>>>>>> According to PI spec, "Prior to invoking any UEFI >> >>>>>> drivers, or applications that are not from the platform >> >>>>>> manufacturer, or connecting consoles, the platform >> >>>>>> should signals the event EFI_END_OF_DXE_EVENT_GUID" >> >>>>>>> >> >>>>>>> In brief, EndOfDxe is signaled before 3rd part code >> >>>>>> running. >> >>>>>>> >> >>>>>>> As such, it is legal that a trusted console is >> >>>>>> connected before EndOfDxe. >> >>>>>>> You can report progress to user before EndOfDxe. >> >>>>>>> >> >>>>>> >> >>>>>> So how do I connect a trusted console on a system with >> >>>>>> a plugin graphics card? >> >>>>>> How can I report capsule update progress on such a >> >>>>>> system? >> >>>>>> On a system such as ARM where the actual flash update >> >>>>>> involves calls >> >>>>>> into the standalone MM layer, which makes the >> >>>>>> distinction unnecessary, >> >>>>>> how do you recommend to handle this if it is mandatory >> >>>>>> according to >> >>>>>> you to process the capsule before EndOfDxe? >> >>>>>> >> >>>>>> >> >>>>>>>> -----Original Message----- >> >>>>>>>> From: Ard Biesheuvel >> >>>>>> [mailto:ard.biesheuvel@linaro.org] >> >>>>>>>> Sent: Friday, June 8, 2018 8:38 AM >> >>>>>>>> To: Yao, Jiewen >> >>>>>>>> Cc: edk2-devel@lists.01.org; Kinney, Michael D >> >>>>>> ; >> >>>>>>>> Zeng, Star ; >> >>>>>> leif.lindholm@linaro.org >> >>>>>>>> Subject: Re: [edk2] [PATCH v2 2/5] >> >>>>>> MdeModulePkg/DxeCapsuleLibFmp: permit >> >>>>>>>> ProcessCapsules () to be called once >> >>>>>>>> >> >>>>>>>> >> >>>>>>>> >> >>>>>>>>> On 8 Jun 2018, at 14:34, Yao, Jiewen >> >>>>>> wrote: >> >>>>>>>>> >> >>>>>>>>> Hi Ard >> >>>>>>>>> We don't allow platform to update system firmware >> >>>>>> *after* EndOfDxe. >> >>>>>>>>> >> >>>>>>>>> According to PI spec, after EndOfDxe, 3rd part >> >>>>>> code start running. It brings >> >>>>>>>> security risk if we allow system firmware after >> >>>>>> EndOfDxe. >> >>>>>>>>> >> >>>>>>>>> In our X86 system design, we lock flash part >> >>>>>> *before* EndOfDxe in any boot >> >>>>>>>> mode. >> >>>>>>>>> Even in CapsuleUpdate boot mode, we also lock >> >>>>>> flash part at EndOfDxe, just in >> >>>>>>>> case the capsule update does not indicate a reset. >> >>>>>>>>> >> >>>>>>>>> Would you please share the info, why your platform >> >>>>>> need update system >> >>>>>>>> firmware after EndOfDxe? >> >>>>>>>>> Is that possible to make it earlier? >> >>>>>>>>> >> >>>>>>>>> >> >>>>>>>> >> >>>>>>>> Because we need some kind of console to report >> >>>>>> progress to the user. >> >>>>>>>> >> >>>>>>>> The secure platform execution context is completely >> >>>>>> separate from UEFI on Arm, >> >>>>>>>> so for us the distinction does not make sense. >> >>>>>>>> >> >>>>>>>>> Thank you >> >>>>>>>>> Yao Jiewen >> >>>>>>>>> >> >>>>>>>>>> -----Original Message----- >> >>>>>>>>>> From: edk2-devel [mailto:edk2-devel- >> >>>>>> bounces@lists.01.org] On Behalf Of >> >>>>>>>> Ard >> >>>>>>>>>> Biesheuvel >> >>>>>>>>>> Sent: Friday, June 8, 2018 2:58 AM >> >>>>>>>>>> To: edk2-devel@lists.01.org >> >>>>>>>>>> Cc: Kinney, Michael D >> >>>>>> ; Yao, Jiewen >> >>>>>>>>>> ; Zeng, Star >> >>>>>> ; >> >>>>>>>>>> leif.lindholm@linaro.org; Ard Biesheuvel >> >>>>>> >> >>>>>>>>>> Subject: [edk2] [PATCH v2 2/5] >> >>>>>> MdeModulePkg/DxeCapsuleLibFmp: permit >> >>>>>>>>>> ProcessCapsules () to be called once >> >>>>>>>>>> >> >>>>>>>>>> Permit ProcessCapsules () to be called only a >> >>>>>> single time, after >> >>>>>>>>>> EndOfDxe. This allows platforms that are able to >> >>>>>> update system >> >>>>>>>>>> firmware after EndOfDxe (e.g., because the flash >> >>>>>> ROM is not locked >> >>>>>>>>>> down) to do so at a time when a non-trusted >> >>>>>> console is up and running, >> >>>>>>>>>> and progress can be reported to the user. >> >>>>>>>>>> >> >>>>>>>>>> Contributed-under: TianoCore Contribution >> >>>>>> Agreement 1.1 >> >>>>>>>>>> Signed-off-by: Ard Biesheuvel >> >>>>>> >> >>>>>>>>>> --- >> >>>>>>>>>> >> >>>>>> MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleProcess >> >>>>>> Lib.c | 18 >> >>>>>>>>>> ++++++++++++------ >> >>>>>>>>>> 1 file changed, 12 insertions(+), 6 deletions(-) >> >>>>>>>>>> >> >>>>>>>>>> diff --git >> >>>>>>>> >> >>>>>> a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleProce >> >>>>>> ssLib.c >> >>>>>>>>>> >> >>>>>> b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleProce >> >>>>>> ssLib.c >> >>>>>>>>>> index 26ca4e295f20..ad83660f1737 100644 >> >>>>>>>>>> --- >> >>>>>> a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleProce >> >>>>>> ssLib.c >> >>>>>>>>>> +++ >> >>>>>> b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleProce >> >>>>>> ssLib.c >> >>>>>>>>>> @@ -100,6 +100,7 @@ IsValidCapsuleHeader ( >> >>>>>>>>>> >> >>>>>>>>>> extern BOOLEAN >> >>>>>> mDxeCapsuleLibEndOfDxe; >> >>>>>>>>>> BOOLEAN mNeedReset; >> >>>>>>>>>> +BOOLEAN mFirstRound =3D >> >>>>>> TRUE; >> >>>>>>>>>> >> >>>>>>>>>> VOID **mCapsulePtr; >> >>>>>>>>>> EFI_STATUS *mCapsuleStatusArray; >> >>>>>>>>>> @@ -364,8 +365,11 @@ >> >>>>>> PopulateCapsuleInConfigurationTable ( >> >>>>>>>>>> >> >>>>>>>>>> Each individual capsule result is recorded in >> >>>>>> capsule record variable. >> >>>>>>>>>> >> >>>>>>>>>> - @param[in] FirstRound TRUE: First >> >>>>>> round. Need skip the >> >>>>>>>> FMP >> >>>>>>>>>> capsules with non zero EmbeddedDriverCount. >> >>>>>>>>>> - FALSE: Process >> >>>>>> rest FMP capsules. >> >>>>>>>>>> + @param[in] FirstRound Whether this is >> >>>>>> the first invocation >> >>>>>>>>>> + @param[in] LastRound Whether this is >> >>>>>> the last invocation >> >>>>>>>>>> + FALSE: First >> >>>>>> of 2 rounds. Need skip >> >>>>>>>> the >> >>>>>>>>>> FMP >> >>>>>>>>>> + >> >>>>>> capsules with non zero >> >>>>>>>>>> EmbeddedDriverCount. >> >>>>>>>>>> + TRUE: Process >> >>>>>> rest FMP capsules. >> >>>>>>>>>> >> >>>>>>>>>> @retval EFI_SUCCESS There is no >> >>>>>> error when processing >> >>>>>>>>>> capsules. >> >>>>>>>>>> @retval EFI_OUT_OF_RESOURCES No enough >> >>>>>> resource to process >> >>>>>>>>>> capsules. >> >>>>>>>>>> @@ -373,7 +377,8 @@ >> >>>>>> PopulateCapsuleInConfigurationTable ( >> >>>>>>>>>> **/ >> >>>>>>>>>> EFI_STATUS >> >>>>>>>>>> ProcessTheseCapsules ( >> >>>>>>>>>> - IN BOOLEAN FirstRound >> >>>>>>>>>> + IN BOOLEAN FirstRound, >> >>>>>>>>>> + IN BOOLEAN LastRound >> >>>>>>>>>> ) >> >>>>>>>>>> { >> >>>>>>>>>> EFI_STATUS Status; >> >>>>>>>>>> @@ -453,7 +458,7 @@ ProcessTheseCapsules ( >> >>>>>>>>>> continue; >> >>>>>>>>>> } >> >>>>>>>>>> >> >>>>>>>>>> - if ((!FirstRound) || (EmbeddedDriverCount >> >>>>>> =3D=3D 0)) { >> >>>>>>>>>> + if (LastRound || (EmbeddedDriverCount =3D=3D >> >>>>>> 0)) { >> >>>>>>>>>> DEBUG((DEBUG_INFO, "ProcessCapsuleImage - >> >>>>>> 0x%x\n", >> >>>>>>>>>> CapsuleHeader)); >> >>>>>>>>>> Status =3D ProcessCapsuleImage >> >>>>>> (CapsuleHeader); >> >>>>>>>>>> mCapsuleStatusArray [Index] =3D Status; >> >>>>>>>>>> @@ -546,7 +551,7 @@ ProcessCapsules ( >> >>>>>>>>>> EFI_STATUS Status; >> >>>>>>>>>> >> >>>>>>>>>> if (!mDxeCapsuleLibEndOfDxe) { >> >>>>>>>>>> - Status =3D ProcessTheseCapsules(TRUE); >> >>>>>>>>>> + Status =3D ProcessTheseCapsules(TRUE, FALSE); >> >>>>>>>>>> >> >>>>>>>>>> // >> >>>>>>>>>> // Reboot System if and only if all capsule >> >>>>>> processed. >> >>>>>>>>>> @@ -555,8 +560,9 @@ ProcessCapsules ( >> >>>>>>>>>> if (mNeedReset && AreAllImagesProcessed()) { >> >>>>>>>>>> DoResetSystem(); >> >>>>>>>>>> } >> >>>>>>>>>> + mFirstRound =3D FALSE; >> >>>>>>>>>> } else { >> >>>>>>>>>> - Status =3D ProcessTheseCapsules(FALSE); >> >>>>>>>>>> + Status =3D ProcessTheseCapsules(mFirstRound, >> >>>>>> TRUE); >> >>>>>>>>>> // >> >>>>>>>>>> // Reboot System if required after all >> >>>>>> capsule processed >> >>>>>>>>>> // >> >>>>>>>>>> -- >> >>>>>>>>>> 2.17.0 >> >>>>>>>>>> >> >>>>>>>>>> _______________________________________________ >> >>>>>>>>>> edk2-devel mailing list >> >>>>>>>>>> edk2-devel@lists.01.org >> >>>>>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel >> >>>> _______________________________________________ >> >>>> edk2-devel mailing list >> >>>> edk2-devel@lists.01.org >> >>>> https://lists.01.org/mailman/listinfo/edk2-devel >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel