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

["Kinney, Michael D" <michael.d.kinney@intel.com>]

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