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

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

If all fmp can be processed one time，you just need call once. Then system reset.

thank you!
Yao, Jiewen


> 在 2018年6月11日，上午12:27，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