From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.136; helo=mga12.intel.com; envelope-from=jiewen.yao@intel.com; receiver=edk2-devel@lists.01.org Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 5DAC721122E4D for ; Sun, 10 Jun 2018 12:21:24 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Jun 2018 12:21:23 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,498,1520924400"; d="scan'208";a="62893100" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga001.fm.intel.com with ESMTP; 10 Jun 2018 12:21:23 -0700 Received: from fmsmsx111.amr.corp.intel.com (10.18.116.5) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sun, 10 Jun 2018 12:21:23 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx111.amr.corp.intel.com (10.18.116.5) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sun, 10 Jun 2018 12:21:23 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.223]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.116]) with mapi id 14.03.0319.002; Mon, 11 Jun 2018 03:21:19 +0800 From: "Yao, Jiewen" To: "Yao, Jiewen" , "Kinney, Michael D" , Ard Biesheuvel CC: "edk2-devel@lists.01.org" , "leif.lindholm@linaro.org" , "Zeng, Star" Thread-Topic: [edk2] [PATCH v2 2/5] MdeModulePkg/DxeCapsuleLibFmp: permit ProcessCapsules () to be called once Thread-Index: AQHT/vYbbSADsfB+hEWX7Kwk2kNBmKRWSrdA//98PACAAIb9kIACKYCAgACr9YCAALUdEIAABtFQ Date: Sun, 10 Jun 2018 19:21:18 +0000 Message-ID: <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> In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C503AC3ADC8@shsmsx102.ccr.corp.intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZTBmOWNmNDYtZDk2Zi00ZDAzLTkyOTEtNzhmZjk0ODA1M2YwIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoibXZxcXllTnozREhBSmxGWWlVcXRGb3dLaHRXRFA5SmtZK2JZcnN1SlhYVVEwVGhuUkZtenlNVUFrd2hiWGF6XC8ifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.200.100 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 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: Sun, 10 Jun 2018 19:21:24 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 EndOfD= xe as the lock event for system firmware, but an ARM platform may want to l= ock system firmware later, maybe in other lock event. As such, how about we generalize the lock event as PcdSystemFmpLockEventGui= d? We can use EndOfDxeGuid as default one to keep the compatibility. At sam= e time this brings the flexibility for platform overriding. We only need update DxeCapsuleLibConstructor() to use the new PcdGuid, inst= ead of hardcode EndOfDxe. We don't need update the logic in ProcessCapsules() and ProcessTheseCapsule= s(). The policy in current platform is already enforced, if the platform has a t= rusted console. For ARM platform, which wants to lock system firmware later. It can configu= re another lock event GUID explicitly in platform DSC. Thank you Yao Jiewen > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ya= o, > 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 >=20 > My concern is that *always allowing* processing SystemCapsule after EndOf= Dxe > has security risk. >=20 > IMHO, the risk is not *process*, if it is a platform choice. Instead, the= risk is > *allow*, in the core logic. >=20 > If we really want to do that, I hope we need a way to distinguish the dif= ference > between a platform dispatching SystemCapsule after EndOfDxe *purposely* a= nd > a platform dispatching SystemCapsule after EndOfDxe *by mistake*. >=20 > Maybe some policy enforcement in the core logic. Static policy, at build = time. >=20 > Thank you > Yao Jiewen >=20 > > -----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: permi= t > > 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