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:c06::242; helo=mail-io0-x242.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x242.google.com (mail-io0-x242.google.com [IPv6:2607:f8b0:4001:c06::242]) (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 0CFD021195BF2 for ; Mon, 11 Jun 2018 00:27:18 -0700 (PDT) Received: by mail-io0-x242.google.com with SMTP id l25-v6so22728967ioh.12 for ; Mon, 11 Jun 2018 00:27:17 -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; bh=b2e4Bm8a9Ifr/yPgfquFYl/MaHggGmUC4dDN7hlqP40=; b=GQrxDodn1pgG4l1ggBHyNyoD9Hf/QalaDGM7ElXSX4sYcWKO7HkeZd6+P4qsfq1SYN DAcz/kNPoc9VQhpys/0e1ejgqS5npBe/aCTpwaNVXit76vqxVegwepV+RgVqfCwy5DIm oMTfjXMjwnayC0+SUHevYAi4XdLMun+fTCQK4= 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; bh=b2e4Bm8a9Ifr/yPgfquFYl/MaHggGmUC4dDN7hlqP40=; b=dreFJZR2VmBbJt+EhC9iCEaQ6ggKqByShyDlF5/k0ElQ6JEFMwis8zGIrYwO7niGIf qTwAh4t2ircHt0KDjvttZ4SMlPS+03SmS2O5kYfXz46z+YiCXL62bOQUYzEMqnWYvIE3 mAlq/HGIBdz24lsE7X+0TyndpbZpgItbGigOAnwfDJ3Sh/vGQYb5j8dmEZe2rskJ/r5d zOgXVmQBmcmjhHpmI4Rkj6s7Qp4PPiHojjDCVh7nmZFtW9LwLmTXy8Ze/zDsHSa704pP YyX5/PK3ZHInwepRnsPWUUBnrpDwnoJ9ZEenjYTRRxMsbnJJy6RLZpnmJxBdKhvDSLPK Jq5w== X-Gm-Message-State: APt69E1aLR8wQZy8zzDU5v8j309w31MhYl3IxSjEczklJR9A+skJ/86s rFfznnF74P+u7iXkhI74IS49LVKI0uZubud2SiFxZA== X-Google-Smtp-Source: ADUXVKIogNGw2VfPPh8umDqqxNB7RW3ymdS15c7eYAXip4WDL1inOWRApbstsG814b3PxcZMxh6m08MXcOKTMtAp1mo= X-Received: by 2002:a6b:6709:: with SMTP id b9-v6mr12777572ioc.170.1528702036944; Mon, 11 Jun 2018 00:27:16 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:bbc7:0:0:0:0:0 with HTTP; Mon, 11 Jun 2018 00:27:16 -0700 (PDT) In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C503AC3ADF5@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> From: Ard Biesheuvel Date: Mon, 11 Jun 2018 09:27:16 +0200 Message-ID: To: "Yao, Jiewen" Cc: "Kinney, Michael D" , "edk2-devel@lists.01.org" , "leif.lindholm@linaro.org" , "Zeng, Star" 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 07:27:18 -0000 Content-Type: text/plain; charset="UTF-8" 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 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-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. 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 ; 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 = >> > > 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