On 10 June 2018 at 21:21, Yao, Jiewen <jiewen....@intel.com> 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 keep the compatibility. At same > time this brings the flexibility for platform overriding. > > > We only need update DxeCapsuleLibConstructor() to use the new PcdGuid, > 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-boun...@lists.01.org] On Behalf Of Yao, >> Jiewen >> Sent: Sunday, June 10, 2018 12:02 PM >> To: Kinney, Michael D <michael.d.kin...@intel.com>; Ard Biesheuvel >> <ard.biesheu...@linaro.org> >> Cc: edk2-devel@lists.01.org; leif.lindh...@linaro.org; Zeng, Star >> <star.z...@intel.com> >> 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. Instead, the >> risk is >> *allow*, in the core logic. >> >> If we really want to do that, I hope we need a way to distinguish the >> 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 <ard.biesheu...@linaro.org>; Yao, Jiewen >> > <jiewen....@intel.com>; Kinney, Michael D <michael.d.kin...@intel.com> >> > Cc: edk2-devel@lists.01.org; Zeng, Star <star.z...@intel.com>; >> > leif.lindh...@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.biesheu...@linaro.org] >> > > Sent: Saturday, June 9, 2018 10:42 PM >> > > To: Yao, Jiewen <jiewen....@intel.com> >> > > Cc: edk2-devel@lists.01.org; Kinney, Michael D >> > > <michael.d.kin...@intel.com>; Zeng, Star >> > > <star.z...@intel.com>; leif.lindh...@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 >> > > <jiewen....@intel.com> 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.biesheu...@linaro.org] >> > > >> Sent: Friday, June 8, 2018 8:38 AM >> > > >> To: Yao, Jiewen <jiewen....@intel.com> >> > > >> Cc: edk2-devel@lists.01.org; Kinney, Michael D >> > > <michael.d.kin...@intel.com>; >> > > >> Zeng, Star <star.z...@intel.com>; >> > > leif.lindh...@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 >> > > <jiewen....@intel.com> 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- >> > > boun...@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 >> > > <michael.d.kin...@intel.com>; Yao, Jiewen >> > > >> >> <jiewen....@intel.com>; Zeng, Star >> > > <star.z...@intel.com>; >> > > >> >> leif.lindh...@linaro.org; Ard Biesheuvel >> > > <ard.biesheu...@linaro.org> >> > > >> >> 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 >> > > <ard.biesheu...@linaro.org> >> > > >> >> --- >> > > >> >> >> > > 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 = >> > > 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 >> > > == 0)) { >> > > >> >> + if (LastRound || (EmbeddedDriverCount == >> > > 0)) { >> > > >> >> DEBUG((DEBUG_INFO, "ProcessCapsuleImage - >> > > 0x%x\n", >> > > >> >> CapsuleHeader)); >> > > >> >> Status = ProcessCapsuleImage >> > > (CapsuleHeader); >> > > >> >> mCapsuleStatusArray [Index] = Status; >> > > >> >> @@ -546,7 +551,7 @@ ProcessCapsules ( >> > > >> >> EFI_STATUS Status; >> > > >> >> >> > > >> >> if (!mDxeCapsuleLibEndOfDxe) { >> > > >> >> - Status = ProcessTheseCapsules(TRUE); >> > > >> >> + Status = ProcessTheseCapsules(TRUE, FALSE); >> > > >> >> >> > > >> >> // >> > > >> >> // Reboot System if and only if all capsule >> > > processed. >> > > >> >> @@ -555,8 +560,9 @@ ProcessCapsules ( >> > > >> >> if (mNeedReset && AreAllImagesProcessed()) { >> > > >> >> DoResetSystem(); >> > > >> >> } >> > > >> >> + mFirstRound = FALSE; >> > > >> >> } else { >> > > >> >> - Status = ProcessTheseCapsules(FALSE); >> > > >> >> + Status = 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
