Re: [PATCH v2 2/5] MdeModulePkg/DxeCapsuleLibFmp: permit ProcessCapsules () to be called once

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 10 June 2018 at 21:21, Yao, Jiewen <jiewen.yao@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-bounces@lists.01.org] On Behalf Of Yao,
>> Jiewen
>> Sent: Sunday, June 10, 2018 12:02 PM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; Ard Biesheuvel
>> <ard.biesheuvel@linaro.org>
>> Cc: edk2-devel@lists.01.org; leif.lindholm@linaro.org; Zeng, Star
>> <star.zeng@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.biesheuvel@linaro.org>; Yao, Jiewen
>> > <jiewen.yao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
>> > Cc: edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com>;
>> > 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 <jiewen.yao@intel.com>
>> > > Cc: edk2-devel@lists.01.org; Kinney, Michael D
>> > > <michael.d.kinney@intel.com>; Zeng, Star
>> > > <star.zeng@intel.com>; 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
>> > > <jiewen.yao@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.biesheuvel@linaro.org]
>> > > >> Sent: Friday, June 8, 2018 8:38 AM
>> > > >> To: Yao, Jiewen <jiewen.yao@intel.com>
>> > > >> Cc: edk2-devel@lists.01.org; Kinney, Michael D
>> > > <michael.d.kinney@intel.com>;
>> > > >> Zeng, Star <star.zeng@intel.com>;
>> > > 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
>> > > <jiewen.yao@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-
>> > > 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
>> > > <michael.d.kinney@intel.com>; Yao, Jiewen
>> > > >> >> <jiewen.yao@intel.com>; Zeng, Star
>> > > <star.zeng@intel.com>;
>> > > >> >> leif.lindholm@linaro.org; Ard Biesheuvel
>> > > <ard.biesheuvel@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.biesheuvel@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