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

["Yao, Jiewen" <jiewen.yao@intel.com>]

Good question. That is because there may be some flash hardware feature to lock the flash part early, and unlockable even in SMM.

As defense in depth, SMM protection can still be enabled. And the flash update must happen before EndOfDxe.

Thank You
Yao Jiewen


> -----Original Message-----
> From: Udit Kumar [mailto:udit.kumar@nxp.com]
> Sent: Monday, June 18, 2018 3:36 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Kinney, Michael D
> <michael.d.kinney@intel.com>; 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
> 
> Hi Jiewen,
> 
> I don't know x86 in details , so ignore stupid question,
> Also thanks to bear with unrelated question.
> 
> > 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.
> 
> On x86, we have SMM mode and I think this is assumed to be secured .
> What is restriction that flash update is done after reset. This could be
> done in same call of OS in SMM mode.
> 
> Regards
> Udit
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Yao, Jiewen
> > Sent: Friday, June 8, 2018 6:05 PM
> > To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>;
> > 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
> >
> > 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?
> >
> >
> > 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/DxeCapsuleProcessLib.c | 18
> > > ++++++++++++------
> > >  1 file changed, 12 insertions(+), 6 deletions(-)
> > >
> > > diff --git
> > > a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleProcessLib.c
> > > b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleProcessLib.c
> > > index 26ca4e295f20..ad83660f1737 100644
> > > --- a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleProcessLib.c
> > > +++
> > b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleProcessLib.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://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
> > > ts.01.org%2Fmailman%2Flistinfo%2Fedk2-
> > devel&data=02%7C01%7Cudit.kumar%
> > >
> > 40nxp.com%7C0701a8fc1bd5448675df08d5cd3c3827%7C686ea1d3bc2b4
> > c6fa92cd99
> > >
> > c5c301635%7C0%7C0%7C636640580906249469&sdata=Fi56Xg%2B1p5e6
> > 9qbD5ITsw8v
> > > ve397ThhomLr9wcY9Ljc%3D&reserved=0
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
> > ts.01.org%2Fmailman%2Flistinfo%2Fedk2-
> > devel&data=02%7C01%7Cudit.kumar%40nxp.com%7C0701a8fc1bd544867
> > 5df08d5cd3c3827%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7
> > C636640580906249469&sdata=Fi56Xg%2B1p5e69qbD5ITsw8vve397Thho
> > mLr9wcY9Ljc%3D&reserved=0