[PATCH v2 0/5] MdeModulePkg ArmPkg: support for persistent capsules and progress reporting

[PATCH v2 0/5] MdeModulePkg ArmPkg: support for persistent capsules and progress reporting

[Ard Biesheuvel]

This is the delta of code required to implement PersistAcrossReset on ARM
systems, and to wire up the capsule handling routines in a way that makes
the new progress reporting code do something meaningful on such platforms.

Changes since v1:
- incorporate Star's feedback (#1, #2)
- add Leif's ack (#4)

Patch #1 ensures that the capsule data which is preserved in DRAM across
a reboot is written back to main memory before attempting to access it
with the caches off.

Patch #2 modifies the logic in DxeCapsuleLibFmp so it can deal with a
platform that chooses to call ProcessCapsules() only a single time after
EndOfDxe.

Patch #3 updates DxeCapsuleLibFmp so it does not pass down the progress
indication callback if its own attempt to invoke it has already failed.

Patch #4 updates ArmPkg's generic PlatformBootManagerLib implementation
to only call ProcessCapsules() after the [potentially non-trusted]
console is up and running, to ensure that firmware update progress can
be reported to the user.

Patch #5 modifies ArmSmcPsciResetSystemLib to emulate a proper warm reboot
by reentering PEI with interrupts, MMU and caches enabled. This works
around the lack of an architected warm reboot in most current implementations.
(The PSCI spec does cover warm reboot, but it was added recently and most
secure firmware implementations haven't caught up yet)

Note that these patches apply on top of Mike's pending changes to
DxeCapsuleLibFmp implementing progress reporting.

Ard Biesheuvel (5):
  MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data
  MdeModulePkg/DxeCapsuleLibFmp: permit ProcessCapsules () to be called
    once
  MdeModulePkg/DxeCapsuleLibFmp: pass progress callback only if it works
  ArmPkg/PlatformBootManagerLib: call ProcessCapsules() only once
  ArmPkg/ArmSmcPsciResetSystemLib: implement fallback for warm reboot

 ArmPkg/ArmPkg.dec                             |  4 ++
 .../ArmSmcPsciResetSystemLib.c                | 21 +++++++++-
 .../ArmSmcPsciResetSystemLib.inf              |  9 +++++
 .../PlatformBootManagerLib/PlatformBm.c       | 15 --------
 .../Library/DxeCapsuleLibFmp/DxeCapsuleLib.c  | 13 +++++--
 .../DxeCapsuleLibFmp/DxeCapsuleProcessLib.c   | 18 ++++++---
 .../Universal/CapsulePei/CapsulePei.inf       |  1 +
 .../CapsulePei/Common/CapsuleCoalesce.c       | 38 +++++++++++++------
 8 files changed, 82 insertions(+), 37 deletions(-)

-- 
2.17.0

[PATCH v2 1/5] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data

[Ard Biesheuvel]

When capsule updates are staged for processing after a warm reboot,
they are copied into memory with the MMU and caches enabled. When
the capsule PEI gets around to coalescing the capsule, the MMU and
caches may still be disabled, and so on architectures where uncached
accesses are incoherent with the caches (such as ARM and AARCH64),
we may read stale data if we don't clean the caches to memory first.

Note that this cache maintenance cannot be done during the invocation
of UpdateCapsule(), since the ScatterGatherList structures are only
identified by physical address, and at runtime, the firmware doesn't
know whether and where this memory is mapped, and cache maintenance
requires a virtual address.

Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdeModulePkg/Universal/CapsulePei/CapsulePei.inf           |  1 +
 MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c | 38 ++++++++++++++------
 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
index c54bc21a95a8..594e110d1f8a 100644
--- a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
+++ b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
@@ -48,6 +48,7 @@ [Packages]
 
 [LibraryClasses]
   BaseLib
+  CacheMaintenanceLib
   HobLib
   BaseMemoryLib
   PeiServicesLib
diff --git a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
index 3e7054cd38a9..52b80e30b479 100644
--- a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
+++ b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
@@ -27,6 +27,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #include <Guid/CapsuleVendor.h>
 
 #include <Library/BaseMemoryLib.h>
+#include <Library/CacheMaintenanceLib.h>
 #include <Library/DebugLib.h>
 #include <Library/PrintLib.h>
 #include <Library/BaseLib.h>
@@ -253,6 +254,7 @@ ValidateCapsuleByMemoryResource (
   )
 {
   UINTN             Index;
+  BOOLEAN           Valid;
 
   //
   // Sanity Check
@@ -270,25 +272,39 @@ ValidateCapsuleByMemoryResource (
     return FALSE;
   }
 
+  Valid = FALSE;
   if (MemoryResource == NULL) {
     //
     // No memory resource descriptor reported in HOB list before capsule Coalesce.
     //
-    return TRUE;
+    Valid = TRUE;
+  } else {
+    for (Index = 0; MemoryResource[Index].ResourceLength != 0; Index++) {
+      if ((Address >= MemoryResource[Index].PhysicalStart) &&
+          ((Address + Size) <= (MemoryResource[Index].PhysicalStart + MemoryResource[Index].ResourceLength))) {
+        DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",
+                            Address, Size,
+                            Index, MemoryResource[Index].PhysicalStart, MemoryResource[Index].ResourceLength));
+        Valid = TRUE;
+        break;
+      }
+    }
+    if (!Valid) {
+      DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx) Size(0x%lx) not in any MemoryResource\n", Address, Size));
+    }
   }
 
-  for (Index = 0; MemoryResource[Index].ResourceLength != 0; Index++) {
-    if ((Address >= MemoryResource[Index].PhysicalStart) &&
-        ((Address + Size) <= (MemoryResource[Index].PhysicalStart + MemoryResource[Index].ResourceLength))) {
-      DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",
-                          Address, Size,
-                          Index, MemoryResource[Index].PhysicalStart, MemoryResource[Index].ResourceLength));
-      return TRUE;
-    }
+  if (Valid) {
+    //
+    // At this point, we may still be running with the MMU and caches disabled,
+    // and on architectures such as ARM or AARCH64, capsule [meta]data loaded
+    // into memory with the caches on is only guaranteed to be visible to the
+    // CPU running with the caches off after performing an explicit writeback.
+    //
+    WriteBackDataCacheRange ((VOID *)(UINTN)Address, (UINTN)Size);
   }
 
-  DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx) Size(0x%lx) not in any MemoryResource\n", Address, Size));
-  return FALSE;
+  return Valid;
 }
 
 /**
-- 
2.17.0

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

[Ard Biesheuvel]

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

[PATCH v2 3/5] MdeModulePkg/DxeCapsuleLibFmp: pass progress callback only if it works

[Ard Biesheuvel]

If the first call to UpdateImageProgress() fails, there is no point
in passing a pointer to it to Fmp->SetImage(), since it is highly
unlikely to succeed on any subsequent calls.

This permits the FMP implementation to fall back to an alternate means
of providing feedback to the user, e.g., via the console.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c
index f0226eafa576..ab41df0eb0a4 100644
--- a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c
+++ b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c
@@ -841,6 +841,7 @@ SetFmpImageData (
   UINT8                                         *Image;
   VOID                                          *VendorCode;
   CHAR16                                        *AbortReason;
+  EFI_FIRMWARE_MANAGEMENT_UPDATE_IMAGE_PROGRESS ProgressCallback;
 
   Status = gBS->HandleProtocol(
                   Handle,
@@ -892,7 +893,11 @@ SetFmpImageData (
   //
   // Before calling SetImage(), reset the progress bar to 0%
   //
-  UpdateImageProgress (0);
+  ProgressCallback = UpdateImageProgress;
+  Status = UpdateImageProgress (0);
+  if (EFI_ERROR (Status)) {
+    ProgressCallback = NULL;
+  }
 
   Status = Fmp->SetImage(
                   Fmp,
@@ -900,13 +905,15 @@ SetFmpImageData (
                   Image,                                  // Image
                   ImageHeader->UpdateImageSize,           // ImageSize
                   VendorCode,                             // VendorCode
-                  UpdateImageProgress,                    // Progress
+                  ProgressCallback,                       // Progress
                   &AbortReason                            // AbortReason
                   );
   //
   // Set the progress bar to 100% after returning from SetImage()
   //
-  UpdateImageProgress (100);
+  if (ProgressCallback != NULL) {
+    UpdateImageProgress (100);
+  }
 
   DEBUG((DEBUG_INFO, "Fmp->SetImage - %r\n", Status));
   if (AbortReason != NULL) {
-- 
2.17.0

[PATCH v2 4/5] ArmPkg/PlatformBootManagerLib: call ProcessCapsules() only once

[Ard Biesheuvel]

ARM platforms have no restriction on when a system firmware update
capsule can be applied, and so it is not necessary to call
ProcessCapsules() twice. So let's drop the first invocation that
occurs before EndOfDxe, so that capsule updates will be applied
when the console is up and able to provide progress feedback.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
index 3456a71fbb9c..8e1ecdc01564 100644
--- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -553,21 +553,6 @@ PlatformBootManagerBeforeConsole (
   VOID
   )
 {
-  EFI_STATUS                    Status;
-  ESRT_MANAGEMENT_PROTOCOL      *EsrtManagement;
-
-  if (GetBootModeHob() == BOOT_ON_FLASH_UPDATE) {
-    DEBUG ((DEBUG_INFO, "ProcessCapsules Before EndOfDxe ......\n"));
-    Status = ProcessCapsules ();
-    DEBUG ((DEBUG_INFO, "ProcessCapsules returned %r\n", Status));
-  } else {
-    Status = gBS->LocateProtocol (&gEsrtManagementProtocolGuid, NULL,
-                    (VOID **)&EsrtManagement);
-    if (!EFI_ERROR (Status)) {
-      EsrtManagement->SyncEsrtFmp ();
-    }
-  }
-
   //
   // Signal EndOfDxe PI Event
   //
-- 
2.17.0

[PATCH v2 5/5] ArmPkg/ArmSmcPsciResetSystemLib: implement fallback for warm reboot

[Ard Biesheuvel]

Implement ResetSystemLib's EnterS3WithImmediateWake() routine using
a jump back to the PEI entry point with interrupts and MMU+caches
disabled. This is only possible at boot time, when we are sure that
the current CPU is the only one up and running. Also, it depends on
the platform whether the PEI code is preserved in memory (it may be
copied to DRAM rather than execute in place), so also add a feature
PCD to selectively enable this feature.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 ArmPkg/ArmPkg.dec                                                    |  4 ++++
 ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c   | 21 ++++++++++++++++++--
 ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf |  9 +++++++++
 3 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
index debe066b6f7b..3aa229fe2ec9 100644
--- a/ArmPkg/ArmPkg.dec
+++ b/ArmPkg/ArmPkg.dec
@@ -85,6 +85,10 @@ [PcdsFeatureFlag.common]
   # Define if the GICv3 controller should use the GICv2 legacy
   gArmTokenSpaceGuid.PcdArmGicV3WithV2Legacy|FALSE|BOOLEAN|0x00000042
 
+  # Whether to implement warm reboot for capsule update using a jump back to the
+  # PEI entry point with caches and interrupts disabled.
+  gArmTokenSpaceGuid.PcdArmReenterPeiForCapsuleWarmReboot|FALSE|BOOLEAN|0x0000001F
+
 [PcdsFeatureFlag.ARM]
   # Whether to map normal memory as non-shareable. FALSE is the safe choice, but
   # TRUE may be appropriate to fix performance problems if you don't care about
diff --git a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c
index d6d26bce5009..10ceafd14d5d 100644
--- a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c
+++ b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c
@@ -15,10 +15,13 @@
 
 #include <PiDxe.h>
 
+#include <Library/ArmMmuLib.h>
+#include <Library/ArmSmcLib.h>
 #include <Library/BaseLib.h>
 #include <Library/DebugLib.h>
 #include <Library/ResetSystemLib.h>
-#include <Library/ArmSmcLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/UefiRuntimeLib.h>
 
 #include <IndustryStandard/ArmStdSmc.h>
 
@@ -89,7 +92,21 @@ EnterS3WithImmediateWake (
   VOID
   )
 {
-  // Not implemented
+  VOID (*Reset)(VOID);
+
+  if (FeaturePcdGet (PcdArmReenterPeiForCapsuleWarmReboot) &&
+      !EfiAtRuntime ()) {
+    //
+    // At boot time, we are the only core running, so we can implement the
+    // immediate wake (which is used by capsule update) by disabling the MMU
+    // and interrupts, and jumping to the PEI entry point.
+    //
+    Reset = (VOID (*)(VOID))(UINTN)FixedPcdGet64 (PcdFvBaseAddress);
+
+    gBS->RaiseTPL (TPL_HIGH_LEVEL);
+    ArmDisableMmu ();
+    Reset ();
+  }
 }
 
 /**
diff --git a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf
index 5a1ee976e5bc..19021cd1e8b6 100644
--- a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf
+++ b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf
@@ -30,6 +30,15 @@ [Packages]
   MdePkg/MdePkg.dec
 
 [LibraryClasses]
+  ArmMmuLib
   ArmSmcLib
   BaseLib
   DebugLib
+  UefiBootServicesTableLib
+  UefiRuntimeLib
+
+[FeaturePcd]
+  gArmTokenSpaceGuid.PcdArmReenterPeiForCapsuleWarmReboot
+
+[FixedPcd]
+  gArmTokenSpaceGuid.PcdFvBaseAddress
-- 
2.17.0

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

[Yao, Jiewen]

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://lists.01.org/mailman/listinfo/edk2-devel

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

[Ard Biesheuvel]

> 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/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://lists.01.org/mailman/listinfo/edk2-devel

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

[Yao, Jiewen]

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.

Thank you
Yao Jiewen


> -----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/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://lists.01.org/mailman/listinfo/edk2-devel

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

[Ard Biesheuvel]

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/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://lists.01.org/mailman/listinfo/edk2-devel

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

[Kinney, Michael D]

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

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

[Yao, Jiewen]

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

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

[Yao, Jiewen]

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.


Thank you
Yao Jiewen

> -----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

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

[Ard Biesheuvel]

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

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

[Yao, Jiewen]

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

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

[Ard Biesheuvel]

On 11 June 2018 at 14:37, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> If all fmp can be processed one time，you just need call once. Then system reset.
>

I understand that. But if the capsule has embedded drivers, the
library will only dispatch them on the second call, even if the
platform does not require making that distinction. It seems to me that
the phasing between the first and the second call is closely tied to
the semantics of EndOfDxe and the associated policy on X86 not to
allow third party code before it has been signalled.

Could we not parameterize this in some way as well?


>
>> 在 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

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

[Yao, Jiewen]

Ah. Good point.

I think the embedded driver dispatch can be controlled by EndOfDxe, which is similar to MdeModulePkg\Universal\SecurityStubDxe.

So we can have both EndOfDxe and PcdSystemFmpLockEventGuid.
EndOfDxe controls embedded driver dispatch.
PcdSystemFmpLockEventGuid controls 1st call and 2nd call.

Thank you
Yao Jiewen


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
> Biesheuvel
> Sent: Monday, June 11, 2018 5:40 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; 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
> 
> On 11 June 2018 at 14:37, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> > If all fmp can be processed one time，you just need call once. Then system
> reset.
> >
> 
> I understand that. But if the capsule has embedded drivers, the
> library will only dispatch them on the second call, even if the
> platform does not require making that distinction. It seems to me that
> the phasing between the first and the second call is closely tied to
> the semantics of EndOfDxe and the associated policy on X86 not to
> allow third party code before it has been signalled.
> 
> Could we not parameterize this in some way as well?
> 
> 
> >
> >> 在 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
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Ard Biesheuvel]

On 11 June 2018 at 15:55, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> Ah. Good point.
>
> I think the embedded driver dispatch can be controlled by EndOfDxe, which is similar to MdeModulePkg\Universal\SecurityStubDxe.
>
> So we can have both EndOfDxe and PcdSystemFmpLockEventGuid.
> EndOfDxe controls embedded driver dispatch.
> PcdSystemFmpLockEventGuid controls 1st call and 2nd call.
>

But in our case, we don't need an event at all. The BDS just calls
ProcessCapsules() whenever it wants to.

I'm rather relucant to complicate the code with an additional event
that we don't really need, while retaining some of the EndOfDxe
handling as well. Could you think of another way to handle this use
case?

Thanks,
Ard.

>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
>> Biesheuvel
>> Sent: Monday, June 11, 2018 5:40 AM
>> To: Yao, Jiewen <jiewen.yao@intel.com>
>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; 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
>>
>> On 11 June 2018 at 14:37, Yao, Jiewen <jiewen.yao@intel.com> wrote:
>> > If all fmp can be processed one time，you just need call once. Then system
>> reset.
>> >
>>
>> I understand that. But if the capsule has embedded drivers, the
>> library will only dispatch them on the second call, even if the
>> platform does not require making that distinction. It seems to me that
>> the phasing between the first and the second call is closely tied to
>> the semantics of EndOfDxe and the associated policy on X86 not to
>> allow third party code before it has been signalled.
>>
>> Could we not parameterize this in some way as well?
>>
>>
>> >
>> >> 在 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
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Kinney, Michael D]

Jiewen,

A platform can have multiple FMP protocols to
update system firmware and may be mapped to
different FW storage devices with different
security attributes.  I think your proposal
would treat them all the same. Right?

Mike



> -----Original Message-----
> From: Yao, Jiewen
> Sent: Monday, June 11, 2018 6:56 AM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>;
> 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
> 
> Ah. Good point.
> 
> I think the embedded driver dispatch can be controlled
> by EndOfDxe, which is similar to
> MdeModulePkg\Universal\SecurityStubDxe.
> 
> So we can have both EndOfDxe and
> PcdSystemFmpLockEventGuid.
> EndOfDxe controls embedded driver dispatch.
> PcdSystemFmpLockEventGuid controls 1st call and 2nd
> call.
> 
> Thank you
> Yao Jiewen
> 
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-
> bounces@lists.01.org] On Behalf Of Ard
> > Biesheuvel
> > Sent: Monday, June 11, 2018 5:40 AM
> > To: Yao, Jiewen <jiewen.yao@intel.com>
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>;
> 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
> >
> > On 11 June 2018 at 14:37, Yao, Jiewen
> <jiewen.yao@intel.com> wrote:
> > > If all fmp can be processed one time，you just need
> call once. Then system
> > reset.
> > >
> >
> > I understand that. But if the capsule has embedded
> drivers, the
> > library will only dispatch them on the second call,
> even if the
> > platform does not require making that distinction. It
> seems to me that
> > the phasing between the first and the second call is
> closely tied to
> > the semantics of EndOfDxe and the associated policy
> on X86 not to
> > allow third party code before it has been signalled.
> >
> > Could we not parameterize this in some way as well?
> >
> >
> > >
> > >> 在 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
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel

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

[Yao, Jiewen]

The difference between 1st call and 2nd call is that: The system will reset even there are capsules not dispatched in 2nd call.

If a platform just calls once and there is 1 device FMP not dispatched yet, how the core lib knows it should reset the system or it should wait for the second call after connect all?

If you have any thought, please share with us.


Summarize some requirement:
1) Embedded driver must be dispatched after EndOfDxe.
2) System Capsule must be dispatched before system firmware lock. (Ideally it should be EndOfDxe, a platform may choose others)
3) When the system capsule is dispatched, the driver FMP might be ready or might not be ready yet.
4) The core code need make sure the platform code dispatch the capsule in a right time.

Thank you
Yao Jiewen

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
> Biesheuvel
> Sent: Monday, June 11, 2018 7:07 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: 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
> 
> On 11 June 2018 at 15:55, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> > Ah. Good point.
> >
> > I think the embedded driver dispatch can be controlled by EndOfDxe, which is
> similar to MdeModulePkg\Universal\SecurityStubDxe.
> >
> > So we can have both EndOfDxe and PcdSystemFmpLockEventGuid.
> > EndOfDxe controls embedded driver dispatch.
> > PcdSystemFmpLockEventGuid controls 1st call and 2nd call.
> >
> 
> But in our case, we don't need an event at all. The BDS just calls
> ProcessCapsules() whenever it wants to.
> 
> I'm rather relucant to complicate the code with an additional event
> that we don't really need, while retaining some of the EndOfDxe
> handling as well. Could you think of another way to handle this use
> case?
> 
> Thanks,
> Ard.
> 
> >> -----Original Message-----
> >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Ard
> >> Biesheuvel
> >> Sent: Monday, June 11, 2018 5:40 AM
> >> To: Yao, Jiewen <jiewen.yao@intel.com>
> >> Cc: Kinney, Michael D <michael.d.kinney@intel.com>;
> 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
> >>
> >> On 11 June 2018 at 14:37, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> >> > If all fmp can be processed one time，you just need call once. Then system
> >> reset.
> >> >
> >>
> >> I understand that. But if the capsule has embedded drivers, the
> >> library will only dispatch them on the second call, even if the
> >> platform does not require making that distinction. It seems to me that
> >> the phasing between the first and the second call is closely tied to
> >> the semantics of EndOfDxe and the associated policy on X86 not to
> >> allow third party code before it has been signalled.
> >>
> >> Could we not parameterize this in some way as well?
> >>
> >>
> >> >
> >> >> 在 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
> >> _______________________________________________
> >> 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

Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data

[Ard Biesheuvel]

On 8 June 2018 at 08:58, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> When capsule updates are staged for processing after a warm reboot,
> they are copied into memory with the MMU and caches enabled. When
> the capsule PEI gets around to coalescing the capsule, the MMU and
> caches may still be disabled, and so on architectures where uncached
> accesses are incoherent with the caches (such as ARM and AARCH64),
> we may read stale data if we don't clean the caches to memory first.
>
> Note that this cache maintenance cannot be done during the invocation
> of UpdateCapsule(), since the ScatterGatherList structures are only
> identified by physical address, and at runtime, the firmware doesn't
> know whether and where this memory is mapped, and cache maintenance
> requires a virtual address.
>
> Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Star,

If you are ok with this version of the patch, please let me know.

This patch and the PsciResetSystemLib one are prerequisites for making
PersistAcrossReset capsules work at all on ARM systems. The remaining
patches are only relevant when using the new progress reporting APIs,
so those can wait, but I would like to merge this one as soon as it is
ready.

Thanks,
Ard.


> ---
>  MdeModulePkg/Universal/CapsulePei/CapsulePei.inf           |  1 +
>  MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c | 38 ++++++++++++++------
>  2 files changed, 28 insertions(+), 11 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
> index c54bc21a95a8..594e110d1f8a 100644
> --- a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
> +++ b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
> @@ -48,6 +48,7 @@ [Packages]
>
>  [LibraryClasses]
>    BaseLib
> +  CacheMaintenanceLib
>    HobLib
>    BaseMemoryLib
>    PeiServicesLib
> diff --git a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
> index 3e7054cd38a9..52b80e30b479 100644
> --- a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
> +++ b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
> @@ -27,6 +27,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #include <Guid/CapsuleVendor.h>
>
>  #include <Library/BaseMemoryLib.h>
> +#include <Library/CacheMaintenanceLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/PrintLib.h>
>  #include <Library/BaseLib.h>
> @@ -253,6 +254,7 @@ ValidateCapsuleByMemoryResource (
>    )
>  {
>    UINTN             Index;
> +  BOOLEAN           Valid;
>
>    //
>    // Sanity Check
> @@ -270,25 +272,39 @@ ValidateCapsuleByMemoryResource (
>      return FALSE;
>    }
>
> +  Valid = FALSE;
>    if (MemoryResource == NULL) {
>      //
>      // No memory resource descriptor reported in HOB list before capsule Coalesce.
>      //
> -    return TRUE;
> +    Valid = TRUE;
> +  } else {
> +    for (Index = 0; MemoryResource[Index].ResourceLength != 0; Index++) {
> +      if ((Address >= MemoryResource[Index].PhysicalStart) &&
> +          ((Address + Size) <= (MemoryResource[Index].PhysicalStart + MemoryResource[Index].ResourceLength))) {
> +        DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",
> +                            Address, Size,
> +                            Index, MemoryResource[Index].PhysicalStart, MemoryResource[Index].ResourceLength));
> +        Valid = TRUE;
> +        break;
> +      }
> +    }
> +    if (!Valid) {
> +      DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx) Size(0x%lx) not in any MemoryResource\n", Address, Size));
> +    }
>    }
>
> -  for (Index = 0; MemoryResource[Index].ResourceLength != 0; Index++) {
> -    if ((Address >= MemoryResource[Index].PhysicalStart) &&
> -        ((Address + Size) <= (MemoryResource[Index].PhysicalStart + MemoryResource[Index].ResourceLength))) {
> -      DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",
> -                          Address, Size,
> -                          Index, MemoryResource[Index].PhysicalStart, MemoryResource[Index].ResourceLength));
> -      return TRUE;
> -    }
> +  if (Valid) {
> +    //
> +    // At this point, we may still be running with the MMU and caches disabled,
> +    // and on architectures such as ARM or AARCH64, capsule [meta]data loaded
> +    // into memory with the caches on is only guaranteed to be visible to the
> +    // CPU running with the caches off after performing an explicit writeback.
> +    //
> +    WriteBackDataCacheRange ((VOID *)(UINTN)Address, (UINTN)Size);
>    }
>
> -  DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx) Size(0x%lx) not in any MemoryResource\n", Address, Size));
> -  return FALSE;
> +  return Valid;
>  }
>
>  /**
> --
> 2.17.0
>

Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data

[Yao, Jiewen]

Hi Ard
May I suggest that we split the Capsule Dispatch patch from the cache maintenance patch?

I think the former may require more time for design discussion.

Thank you
Yao Jiewen


> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Monday, June 11, 2018 2:25 PM
> To: edk2-devel@lists.01.org
> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Zeng, Star <star.zeng@intel.com>;
> Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei: clean Dcache before
> consuming capsule data
> 
> On 8 June 2018 at 08:58, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > When capsule updates are staged for processing after a warm reboot,
> > they are copied into memory with the MMU and caches enabled. When
> > the capsule PEI gets around to coalescing the capsule, the MMU and
> > caches may still be disabled, and so on architectures where uncached
> > accesses are incoherent with the caches (such as ARM and AARCH64),
> > we may read stale data if we don't clean the caches to memory first.
> >
> > Note that this cache maintenance cannot be done during the invocation
> > of UpdateCapsule(), since the ScatterGatherList structures are only
> > identified by physical address, and at runtime, the firmware doesn't
> > know whether and where this memory is mapped, and cache maintenance
> > requires a virtual address.
> >
> > Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> Star,
> 
> If you are ok with this version of the patch, please let me know.
> 
> This patch and the PsciResetSystemLib one are prerequisites for making
> PersistAcrossReset capsules work at all on ARM systems. The remaining
> patches are only relevant when using the new progress reporting APIs,
> so those can wait, but I would like to merge this one as soon as it is
> ready.
> 
> Thanks,
> Ard.
> 
> 
> > ---
> >  MdeModulePkg/Universal/CapsulePei/CapsulePei.inf           |  1 +
> >  MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c | 38
> ++++++++++++++------
> >  2 files changed, 28 insertions(+), 11 deletions(-)
> >
> > diff --git a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
> b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
> > index c54bc21a95a8..594e110d1f8a 100644
> > --- a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
> > +++ b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
> > @@ -48,6 +48,7 @@ [Packages]
> >
> >  [LibraryClasses]
> >    BaseLib
> > +  CacheMaintenanceLib
> >    HobLib
> >    BaseMemoryLib
> >    PeiServicesLib
> > diff --git a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
> b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
> > index 3e7054cd38a9..52b80e30b479 100644
> > --- a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
> > +++ b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
> > @@ -27,6 +27,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
> >  #include <Guid/CapsuleVendor.h>
> >
> >  #include <Library/BaseMemoryLib.h>
> > +#include <Library/CacheMaintenanceLib.h>
> >  #include <Library/DebugLib.h>
> >  #include <Library/PrintLib.h>
> >  #include <Library/BaseLib.h>
> > @@ -253,6 +254,7 @@ ValidateCapsuleByMemoryResource (
> >    )
> >  {
> >    UINTN             Index;
> > +  BOOLEAN           Valid;
> >
> >    //
> >    // Sanity Check
> > @@ -270,25 +272,39 @@ ValidateCapsuleByMemoryResource (
> >      return FALSE;
> >    }
> >
> > +  Valid = FALSE;
> >    if (MemoryResource == NULL) {
> >      //
> >      // No memory resource descriptor reported in HOB list before capsule
> Coalesce.
> >      //
> > -    return TRUE;
> > +    Valid = TRUE;
> > +  } else {
> > +    for (Index = 0; MemoryResource[Index].ResourceLength != 0; Index++) {
> > +      if ((Address >= MemoryResource[Index].PhysicalStart) &&
> > +          ((Address + Size) <= (MemoryResource[Index].PhysicalStart +
> MemoryResource[Index].ResourceLength))) {
> > +        DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in
> MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",
> > +                            Address, Size,
> > +                            Index,
> MemoryResource[Index].PhysicalStart,
> MemoryResource[Index].ResourceLength));
> > +        Valid = TRUE;
> > +        break;
> > +      }
> > +    }
> > +    if (!Valid) {
> > +      DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx) Size(0x%lx) not in
> any MemoryResource\n", Address, Size));
> > +    }
> >    }
> >
> > -  for (Index = 0; MemoryResource[Index].ResourceLength != 0; Index++) {
> > -    if ((Address >= MemoryResource[Index].PhysicalStart) &&
> > -        ((Address + Size) <= (MemoryResource[Index].PhysicalStart +
> MemoryResource[Index].ResourceLength))) {
> > -      DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in
> MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",
> > -                          Address, Size,
> > -                          Index, MemoryResource[Index].PhysicalStart,
> MemoryResource[Index].ResourceLength));
> > -      return TRUE;
> > -    }
> > +  if (Valid) {
> > +    //
> > +    // At this point, we may still be running with the MMU and caches
> disabled,
> > +    // and on architectures such as ARM or AARCH64, capsule [meta]data
> loaded
> > +    // into memory with the caches on is only guaranteed to be visible to the
> > +    // CPU running with the caches off after performing an explicit
> writeback.
> > +    //
> > +    WriteBackDataCacheRange ((VOID *)(UINTN)Address, (UINTN)Size);
> >    }
> >
> > -  DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx) Size(0x%lx) not in any
> MemoryResource\n", Address, Size));
> > -  return FALSE;
> > +  return Valid;
> >  }
> >
> >  /**
> > --
> > 2.17.0
> >

Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data

[Ard Biesheuvel]

On 11 June 2018 at 23:27, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> Hi Ard
> May I suggest that we split the Capsule Dispatch patch from the cache maintenance patch?
>
> I think the former may require more time for design discussion.
>

Yes, that is fine. As I explained, it has mainly to do with
dispatching the capsule at a time when the console or GOP is available
to report progress. This is separate from the cache maintenance issue.


>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Monday, June 11, 2018 2:25 PM
>> To: edk2-devel@lists.01.org
>> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Zeng, Star <star.zeng@intel.com>;
>> Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Subject: Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei: clean Dcache before
>> consuming capsule data
>>
>> On 8 June 2018 at 08:58, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> > When capsule updates are staged for processing after a warm reboot,
>> > they are copied into memory with the MMU and caches enabled. When
>> > the capsule PEI gets around to coalescing the capsule, the MMU and
>> > caches may still be disabled, and so on architectures where uncached
>> > accesses are incoherent with the caches (such as ARM and AARCH64),
>> > we may read stale data if we don't clean the caches to memory first.
>> >
>> > Note that this cache maintenance cannot be done during the invocation
>> > of UpdateCapsule(), since the ScatterGatherList structures are only
>> > identified by physical address, and at runtime, the firmware doesn't
>> > know whether and where this memory is mapped, and cache maintenance
>> > requires a virtual address.
>> >
>> > Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
>> > Contributed-under: TianoCore Contribution Agreement 1.1
>> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>> Star,
>>
>> If you are ok with this version of the patch, please let me know.
>>
>> This patch and the PsciResetSystemLib one are prerequisites for making
>> PersistAcrossReset capsules work at all on ARM systems. The remaining
>> patches are only relevant when using the new progress reporting APIs,
>> so those can wait, but I would like to merge this one as soon as it is
>> ready.
>>
>> Thanks,
>> Ard.
>>
>>
>> > ---
>> >  MdeModulePkg/Universal/CapsulePei/CapsulePei.inf           |  1 +
>> >  MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c | 38
>> ++++++++++++++------
>> >  2 files changed, 28 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
>> b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
>> > index c54bc21a95a8..594e110d1f8a 100644
>> > --- a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
>> > +++ b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
>> > @@ -48,6 +48,7 @@ [Packages]
>> >
>> >  [LibraryClasses]
>> >    BaseLib
>> > +  CacheMaintenanceLib
>> >    HobLib
>> >    BaseMemoryLib
>> >    PeiServicesLib
>> > diff --git a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
>> b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
>> > index 3e7054cd38a9..52b80e30b479 100644
>> > --- a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
>> > +++ b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
>> > @@ -27,6 +27,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
>> KIND, EITHER EXPRESS OR IMPLIED.
>> >  #include <Guid/CapsuleVendor.h>
>> >
>> >  #include <Library/BaseMemoryLib.h>
>> > +#include <Library/CacheMaintenanceLib.h>
>> >  #include <Library/DebugLib.h>
>> >  #include <Library/PrintLib.h>
>> >  #include <Library/BaseLib.h>
>> > @@ -253,6 +254,7 @@ ValidateCapsuleByMemoryResource (
>> >    )
>> >  {
>> >    UINTN             Index;
>> > +  BOOLEAN           Valid;
>> >
>> >    //
>> >    // Sanity Check
>> > @@ -270,25 +272,39 @@ ValidateCapsuleByMemoryResource (
>> >      return FALSE;
>> >    }
>> >
>> > +  Valid = FALSE;
>> >    if (MemoryResource == NULL) {
>> >      //
>> >      // No memory resource descriptor reported in HOB list before capsule
>> Coalesce.
>> >      //
>> > -    return TRUE;
>> > +    Valid = TRUE;
>> > +  } else {
>> > +    for (Index = 0; MemoryResource[Index].ResourceLength != 0; Index++) {
>> > +      if ((Address >= MemoryResource[Index].PhysicalStart) &&
>> > +          ((Address + Size) <= (MemoryResource[Index].PhysicalStart +
>> MemoryResource[Index].ResourceLength))) {
>> > +        DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in
>> MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",
>> > +                            Address, Size,
>> > +                            Index,
>> MemoryResource[Index].PhysicalStart,
>> MemoryResource[Index].ResourceLength));
>> > +        Valid = TRUE;
>> > +        break;
>> > +      }
>> > +    }
>> > +    if (!Valid) {
>> > +      DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx) Size(0x%lx) not in
>> any MemoryResource\n", Address, Size));
>> > +    }
>> >    }
>> >
>> > -  for (Index = 0; MemoryResource[Index].ResourceLength != 0; Index++) {
>> > -    if ((Address >= MemoryResource[Index].PhysicalStart) &&
>> > -        ((Address + Size) <= (MemoryResource[Index].PhysicalStart +
>> MemoryResource[Index].ResourceLength))) {
>> > -      DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in
>> MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",
>> > -                          Address, Size,
>> > -                          Index, MemoryResource[Index].PhysicalStart,
>> MemoryResource[Index].ResourceLength));
>> > -      return TRUE;
>> > -    }
>> > +  if (Valid) {
>> > +    //
>> > +    // At this point, we may still be running with the MMU and caches
>> disabled,
>> > +    // and on architectures such as ARM or AARCH64, capsule [meta]data
>> loaded
>> > +    // into memory with the caches on is only guaranteed to be visible to the
>> > +    // CPU running with the caches off after performing an explicit
>> writeback.
>> > +    //
>> > +    WriteBackDataCacheRange ((VOID *)(UINTN)Address, (UINTN)Size);
>> >    }
>> >
>> > -  DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx) Size(0x%lx) not in any
>> MemoryResource\n", Address, Size));
>> > -  return FALSE;
>> > +  return Valid;
>> >  }
>> >
>> >  /**
>> > --
>> > 2.17.0
>> >

Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data

[Kinney, Michael D]

Hi Ard,

I would still prefer the cache maintenance be done independent
of capsules in case there is other memory ranges that need to
be flushed for other features.

The CacheMaintenanceLib for ARM has several services that are
not implemented and ASSERT().  I found some ARM documentation 
that says that caches can be flushed by looping through the 
indexes.  

http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0201d/ch03s03s05.html

I know you mentioned that there can be alternate MMU
implementations that may not work with that algorithm.  However
that could require a customized CacheMaintenanceLib.  Can you
provide the flush all using the documented algorithm and use that
instance for systems that are compatible with the documented algorithm.
Then, the flush all API can be used in ResetSystem() or SEC Phase
when the boot mode is warm reset.

Mike

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Monday, June 11, 2018 2:29 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: edk2-devel@lists.01.org; Leif Lindholm
> <leif.lindholm@linaro.org>; Zeng, Star
> <star.zeng@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei: clean
> Dcache before consuming capsule data
> 
> On 11 June 2018 at 23:27, Yao, Jiewen
> <jiewen.yao@intel.com> wrote:
> > Hi Ard
> > May I suggest that we split the Capsule Dispatch patch
> from the cache maintenance patch?
> >
> > I think the former may require more time for design
> discussion.
> >
> 
> Yes, that is fine. As I explained, it has mainly to do with
> dispatching the capsule at a time when the console or GOP
> is available
> to report progress. This is separate from the cache
> maintenance issue.
> 
> 
> >> -----Original Message-----
> >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> >> Sent: Monday, June 11, 2018 2:25 PM
> >> To: edk2-devel@lists.01.org
> >> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Zeng, Star
> <star.zeng@intel.com>;
> >> Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D
> >> <michael.d.kinney@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> >> Subject: Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei:
> clean Dcache before
> >> consuming capsule data
> >>
> >> On 8 June 2018 at 08:58, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> >> > When capsule updates are staged for processing after a
> warm reboot,
> >> > they are copied into memory with the MMU and caches
> enabled. When
> >> > the capsule PEI gets around to coalescing the capsule,
> the MMU and
> >> > caches may still be disabled, and so on architectures
> where uncached
> >> > accesses are incoherent with the caches (such as ARM
> and AARCH64),
> >> > we may read stale data if we don't clean the caches to
> memory first.
> >> >
> >> > Note that this cache maintenance cannot be done during
> the invocation
> >> > of UpdateCapsule(), since the ScatterGatherList
> structures are only
> >> > identified by physical address, and at runtime, the
> firmware doesn't
> >> > know whether and where this memory is mapped, and
> cache maintenance
> >> > requires a virtual address.
> >> >
> >> > Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
> >> > Contributed-under: TianoCore Contribution Agreement
> 1.1
> >> > Signed-off-by: Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> >>
> >> Star,
> >>
> >> If you are ok with this version of the patch, please let
> me know.
> >>
> >> This patch and the PsciResetSystemLib one are
> prerequisites for making
> >> PersistAcrossReset capsules work at all on ARM systems.
> The remaining
> >> patches are only relevant when using the new progress
> reporting APIs,
> >> so those can wait, but I would like to merge this one as
> soon as it is
> >> ready.
> >>
> >> Thanks,
> >> Ard.
> >>
> >>
> >> > ---
> >> >  MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
> |  1 +
> >> >
> MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
> | 38
> >> ++++++++++++++------
> >> >  2 files changed, 28 insertions(+), 11 deletions(-)
> >> >
> >> > diff --git
> a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
> >> b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
> >> > index c54bc21a95a8..594e110d1f8a 100644
> >> > --- a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
> >> > +++ b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
> >> > @@ -48,6 +48,7 @@ [Packages]
> >> >
> >> >  [LibraryClasses]
> >> >    BaseLib
> >> > +  CacheMaintenanceLib
> >> >    HobLib
> >> >    BaseMemoryLib
> >> >    PeiServicesLib
> >> > diff --git
> a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.
> c
> >>
> b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.
> c
> >> > index 3e7054cd38a9..52b80e30b479 100644
> >> > ---
> a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.
> c
> >> > +++
> b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.
> c
> >> > @@ -27,6 +27,7 @@ WITHOUT WARRANTIES OR
> REPRESENTATIONS OF ANY
> >> KIND, EITHER EXPRESS OR IMPLIED.
> >> >  #include <Guid/CapsuleVendor.h>
> >> >
> >> >  #include <Library/BaseMemoryLib.h>
> >> > +#include <Library/CacheMaintenanceLib.h>
> >> >  #include <Library/DebugLib.h>
> >> >  #include <Library/PrintLib.h>
> >> >  #include <Library/BaseLib.h>
> >> > @@ -253,6 +254,7 @@ ValidateCapsuleByMemoryResource (
> >> >    )
> >> >  {
> >> >    UINTN             Index;
> >> > +  BOOLEAN           Valid;
> >> >
> >> >    //
> >> >    // Sanity Check
> >> > @@ -270,25 +272,39 @@ ValidateCapsuleByMemoryResource
> (
> >> >      return FALSE;
> >> >    }
> >> >
> >> > +  Valid = FALSE;
> >> >    if (MemoryResource == NULL) {
> >> >      //
> >> >      // No memory resource descriptor reported in HOB
> list before capsule
> >> Coalesce.
> >> >      //
> >> > -    return TRUE;
> >> > +    Valid = TRUE;
> >> > +  } else {
> >> > +    for (Index = 0;
> MemoryResource[Index].ResourceLength != 0; Index++) {
> >> > +      if ((Address >=
> MemoryResource[Index].PhysicalStart) &&
> >> > +          ((Address + Size) <=
> (MemoryResource[Index].PhysicalStart +
> >> MemoryResource[Index].ResourceLength))) {
> >> > +        DEBUG ((EFI_D_INFO, "Address(0x%lx)
> Size(0x%lx) in
> >> MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",
> >> > +                            Address, Size,
> >> > +                            Index,
> >> MemoryResource[Index].PhysicalStart,
> >> MemoryResource[Index].ResourceLength));
> >> > +        Valid = TRUE;
> >> > +        break;
> >> > +      }
> >> > +    }
> >> > +    if (!Valid) {
> >> > +      DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx)
> Size(0x%lx) not in
> >> any MemoryResource\n", Address, Size));
> >> > +    }
> >> >    }
> >> >
> >> > -  for (Index = 0;
> MemoryResource[Index].ResourceLength != 0; Index++) {
> >> > -    if ((Address >=
> MemoryResource[Index].PhysicalStart) &&
> >> > -        ((Address + Size) <=
> (MemoryResource[Index].PhysicalStart +
> >> MemoryResource[Index].ResourceLength))) {
> >> > -      DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx)
> in
> >> MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",
> >> > -                          Address, Size,
> >> > -                          Index,
> MemoryResource[Index].PhysicalStart,
> >> MemoryResource[Index].ResourceLength));
> >> > -      return TRUE;
> >> > -    }
> >> > +  if (Valid) {
> >> > +    //
> >> > +    // At this point, we may still be running with
> the MMU and caches
> >> disabled,
> >> > +    // and on architectures such as ARM or AARCH64,
> capsule [meta]data
> >> loaded
> >> > +    // into memory with the caches on is only
> guaranteed to be visible to the
> >> > +    // CPU running with the caches off after
> performing an explicit
> >> writeback.
> >> > +    //
> >> > +    WriteBackDataCacheRange ((VOID *)(UINTN)Address,
> (UINTN)Size);
> >> >    }
> >> >
> >> > -  DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx)
> Size(0x%lx) not in any
> >> MemoryResource\n", Address, Size));
> >> > -  return FALSE;
> >> > +  return Valid;
> >> >  }
> >> >
> >> >  /**
> >> > --
> >> > 2.17.0
> >> >

Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data

[Ard Biesheuvel]

On 11 June 2018 at 23:40, Kinney, Michael D <michael.d.kinney@intel.com> wrote:
> Hi Ard,
>
> I would still prefer the cache maintenance be done independent
> of capsules in case there is other memory ranges that need to
> be flushed for other features.
>
> The CacheMaintenanceLib for ARM has several services that are
> not implemented and ASSERT().  I found some ARM documentation
> that says that caches can be flushed by looping through the
> indexes.
>
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0201d/ch03s03s05.html
>

Please don't quote severely outdated documentation out of context.
ARM9 is a ~15 year old core that predates SMP on ARM by several years.

> implementations that may not work with that algorithm.  However
> that could require a customized CacheMaintenanceLib.  Can you
> provide the flush all using the documented algorithm and use that
> instance for systems that are compatible with the documented algorithm.

None of the ARM cores we support in EDK2 today (ARMv7 and later) are
compatible with the documented algorithm.

> Then, the flush all API can be used in ResetSystem() or SEC Phase
> when the boot mode is warm reset.
>

Flushing the entire cache is simply not possible. The set/way
operations are only intended to be used before the core enters the
coherency domain or after it leaves it again (i.e., when cores are
powered up or down). Those set/way operations are not broadcast to
other cores (or other masters such as DMA capable peripherals with
caches), which means that a cache line that is cleaned by set/way may
simply end up in another cache and never make it to main memory. In
other words, set/way operations manage the cache itself rather than
the contents of the caches.

The only way to perform cache maintenance in a way to ensure that the
*content* makes it to main memory is to use cache maintenance by
virtual address. This requires that you know the virtual address to
begin with, and obviously requires that a mapping exists for that
virtual address when executed with the MMU on.

The bottom line is that there is no flush all API, and we will have to
work around that (and believe me, this is not the first time we are
struggling to deal with this limitation).

So to summarize again, we have the following options,
- UpdateCapsule - problematic because it may be called at runtime and
the firmware has no means of translating the physical addresses
- ResetSystem - same as above: it is a runtime service, and so it
cannot rely on a mapping to exist for those physical addresses
- SEC - lacks the information about where the capsule resides
- CapsulePei - already extracts the information about the capsule
address in memory, and can perform the cache maintenance right before
consuming the data.

Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data

[Kinney, Michael D]

Ard,

What about memory init when the memory HOBs are
created.  Could you flush all the ranges of 
initialized memory by HOB as the HOBs are created?

PEI and DXE can not use memory not described by
the HOBs.  More memory can be added in DXE phase
through the GCD services, and additional flush 
could be done as added.

Mike

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Monday, June 11, 2018 3:01 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; edk2-
> devel@lists.01.org; Leif Lindholm
> <leif.lindholm@linaro.org>; Zeng, Star
> <star.zeng@intel.com>
> Subject: Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei:
> clean Dcache before consuming capsule data
> 
> On 11 June 2018 at 23:40, Kinney, Michael D
> <michael.d.kinney@intel.com> wrote:
> > Hi Ard,
> >
> > I would still prefer the cache maintenance be done
> independent
> > of capsules in case there is other memory ranges that
> need to
> > be flushed for other features.
> >
> > The CacheMaintenanceLib for ARM has several services
> that are
> > not implemented and ASSERT().  I found some ARM
> documentation
> > that says that caches can be flushed by looping
> through the
> > indexes.
> >
> >
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.
> doc.ddi0201d/ch03s03s05.html
> >
> 
> Please don't quote severely outdated documentation out
> of context.
> ARM9 is a ~15 year old core that predates SMP on ARM by
> several years.
> 
> > implementations that may not work with that algorithm.
> However
> > that could require a customized CacheMaintenanceLib.
> Can you
> > provide the flush all using the documented algorithm
> and use that
> > instance for systems that are compatible with the
> documented algorithm.
> 
> None of the ARM cores we support in EDK2 today (ARMv7
> and later) are
> compatible with the documented algorithm.
> 
> > Then, the flush all API can be used in ResetSystem()
> or SEC Phase
> > when the boot mode is warm reset.
> >
> 
> Flushing the entire cache is simply not possible. The
> set/way
> operations are only intended to be used before the core
> enters the
> coherency domain or after it leaves it again (i.e., when
> cores are
> powered up or down). Those set/way operations are not
> broadcast to
> other cores (or other masters such as DMA capable
> peripherals with
> caches), which means that a cache line that is cleaned
> by set/way may
> simply end up in another cache and never make it to main
> memory. In
> other words, set/way operations manage the cache itself
> rather than
> the contents of the caches.
> 
> The only way to perform cache maintenance in a way to
> ensure that the
> *content* makes it to main memory is to use cache
> maintenance by
> virtual address. This requires that you know the virtual
> address to
> begin with, and obviously requires that a mapping exists
> for that
> virtual address when executed with the MMU on.
> 
> The bottom line is that there is no flush all API, and
> we will have to
> work around that (and believe me, this is not the first
> time we are
> struggling to deal with this limitation).
> 
> So to summarize again, we have the following options,
> - UpdateCapsule - problematic because it may be called
> at runtime and
> the firmware has no means of translating the physical
> addresses
> - ResetSystem - same as above: it is a runtime service,
> and so it
> cannot rely on a mapping to exist for those physical
> addresses
> - SEC - lacks the information about where the capsule
> resides
> - CapsulePei - already extracts the information about
> the capsule
> address in memory, and can perform the cache maintenance
> right before
> consuming the data.

Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data

[Ard Biesheuvel]

On 12 June 2018 at 02:54, Kinney, Michael D <michael.d.kinney@intel.com> wrote:
> Ard,
>
> What about memory init when the memory HOBs are
> created.  Could you flush all the ranges of
> initialized memory by HOB as the HOBs are created?
>
> PEI and DXE can not use memory not described by
> the HOBs.  More memory can be added in DXE phase
> through the GCD services, and additional flush
> could be done as added.
>

CapsuleCoalesce() goes and finds the capsule data wherever it was left
in memory by the OS. This is independent of what the HOBs may describe
or which memory was added at which point.


>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Monday, June 11, 2018 3:01 PM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>
>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; edk2-
>> devel@lists.01.org; Leif Lindholm
>> <leif.lindholm@linaro.org>; Zeng, Star
>> <star.zeng@intel.com>
>> Subject: Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei:
>> clean Dcache before consuming capsule data
>>
>> On 11 June 2018 at 23:40, Kinney, Michael D
>> <michael.d.kinney@intel.com> wrote:
>> > Hi Ard,
>> >
>> > I would still prefer the cache maintenance be done
>> independent
>> > of capsules in case there is other memory ranges that
>> need to
>> > be flushed for other features.
>> >
>> > The CacheMaintenanceLib for ARM has several services
>> that are
>> > not implemented and ASSERT().  I found some ARM
>> documentation
>> > that says that caches can be flushed by looping
>> through the
>> > indexes.
>> >
>> >
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.
>> doc.ddi0201d/ch03s03s05.html
>> >
>>
>> Please don't quote severely outdated documentation out
>> of context.
>> ARM9 is a ~15 year old core that predates SMP on ARM by
>> several years.
>>
>> > implementations that may not work with that algorithm.
>> However
>> > that could require a customized CacheMaintenanceLib.
>> Can you
>> > provide the flush all using the documented algorithm
>> and use that
>> > instance for systems that are compatible with the
>> documented algorithm.
>>
>> None of the ARM cores we support in EDK2 today (ARMv7
>> and later) are
>> compatible with the documented algorithm.
>>
>> > Then, the flush all API can be used in ResetSystem()
>> or SEC Phase
>> > when the boot mode is warm reset.
>> >
>>
>> Flushing the entire cache is simply not possible. The
>> set/way
>> operations are only intended to be used before the core
>> enters the
>> coherency domain or after it leaves it again (i.e., when
>> cores are
>> powered up or down). Those set/way operations are not
>> broadcast to
>> other cores (or other masters such as DMA capable
>> peripherals with
>> caches), which means that a cache line that is cleaned
>> by set/way may
>> simply end up in another cache and never make it to main
>> memory. In
>> other words, set/way operations manage the cache itself
>> rather than
>> the contents of the caches.
>>
>> The only way to perform cache maintenance in a way to
>> ensure that the
>> *content* makes it to main memory is to use cache
>> maintenance by
>> virtual address. This requires that you know the virtual
>> address to
>> begin with, and obviously requires that a mapping exists
>> for that
>> virtual address when executed with the MMU on.
>>
>> The bottom line is that there is no flush all API, and
>> we will have to
>> work around that (and believe me, this is not the first
>> time we are
>> struggling to deal with this limitation).
>>
>> So to summarize again, we have the following options,
>> - UpdateCapsule - problematic because it may be called
>> at runtime and
>> the firmware has no means of translating the physical
>> addresses
>> - ResetSystem - same as above: it is a runtime service,
>> and so it
>> cannot rely on a mapping to exist for those physical
>> addresses
>> - SEC - lacks the information about where the capsule
>> resides
>> - CapsulePei - already extracts the information about
>> the capsule
>> address in memory, and can perform the cache maintenance
>> right before
>> consuming the data.

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

[Zeng, Star]

I am thinking that could platform BDS parse CAPSULE HOB and call ProcessCapsuleImage by itself in its willing phase with flexibility for the case?


Thanks,
Star
-----Original Message-----
From: Yao, Jiewen 
Sent: Monday, June 11, 2018 11:12 PM
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: 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

The difference between 1st call and 2nd call is that: The system will reset even there are capsules not dispatched in 2nd call.

If a platform just calls once and there is 1 device FMP not dispatched yet, how the core lib knows it should reset the system or it should wait for the second call after connect all?

If you have any thought, please share with us.


Summarize some requirement:
1) Embedded driver must be dispatched after EndOfDxe.
2) System Capsule must be dispatched before system firmware lock. (Ideally it should be EndOfDxe, a platform may choose others)
3) When the system capsule is dispatched, the driver FMP might be ready or might not be ready yet.
4) The core code need make sure the platform code dispatch the capsule in a right time.

Thank you
Yao Jiewen

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Ard Biesheuvel
> Sent: Monday, June 11, 2018 7:07 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: 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
> 
> On 11 June 2018 at 15:55, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> > Ah. Good point.
> >
> > I think the embedded driver dispatch can be controlled by EndOfDxe, 
> > which is
> similar to MdeModulePkg\Universal\SecurityStubDxe.
> >
> > So we can have both EndOfDxe and PcdSystemFmpLockEventGuid.
> > EndOfDxe controls embedded driver dispatch.
> > PcdSystemFmpLockEventGuid controls 1st call and 2nd call.
> >
> 
> But in our case, we don't need an event at all. The BDS just calls
> ProcessCapsules() whenever it wants to.
> 
> I'm rather relucant to complicate the code with an additional event 
> that we don't really need, while retaining some of the EndOfDxe 
> handling as well. Could you think of another way to handle this use 
> case?
> 
> Thanks,
> Ard.
> 
> >> -----Original Message-----
> >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf 
> >> Of
> Ard
> >> Biesheuvel
> >> Sent: Monday, June 11, 2018 5:40 AM
> >> To: Yao, Jiewen <jiewen.yao@intel.com>
> >> Cc: Kinney, Michael D <michael.d.kinney@intel.com>;
> 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
> >>
> >> On 11 June 2018 at 14:37, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> >> > If all fmp can be processed one time，you just need call once. 
> >> > Then system
> >> reset.
> >> >
> >>
> >> I understand that. But if the capsule has embedded drivers, the 
> >> library will only dispatch them on the second call, even if the 
> >> platform does not require making that distinction. It seems to me 
> >> that the phasing between the first and the second call is closely 
> >> tied to the semantics of EndOfDxe and the associated policy on X86 
> >> not to allow third party code before it has been signalled.
> >>
> >> Could we not parameterize this in some way as well?
> >>
> >>
> >> >
> >> >> 在 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
> >> _______________________________________________
> >> 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

Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data

[Ard Biesheuvel]

On 12 June 2018 at 11:01, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 12 June 2018 at 02:54, Kinney, Michael D <michael.d.kinney@intel.com> wrote:
>> Ard,
>>
>> What about memory init when the memory HOBs are
>> created.  Could you flush all the ranges of
>> initialized memory by HOB as the HOBs are created?
>>
>> PEI and DXE can not use memory not described by
>> the HOBs.  More memory can be added in DXE phase
>> through the GCD services, and additional flush
>> could be done as added.
>>
>
> CapsuleCoalesce() goes and finds the capsule data wherever it was left
> in memory by the OS. This is independent of what the HOBs may describe
> or which memory was added at which point.
>

As it turns out, relying on the state of the caches to be preserved
across a reboot and cleaning them afterwards is something that cannot
be architecturally guaranteed either. So I will withdraw this patch,
and propose another approach that cleans the capsule during the call
to UpdateCapsule(). For the time being, that only works at boot time,
but given that both Windows and Linux perform capsule updates before
ExitBootServices(), this is something we should be able to live with.


>
>>> -----Original Message-----
>>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>>> Sent: Monday, June 11, 2018 3:01 PM
>>> To: Kinney, Michael D <michael.d.kinney@intel.com>
>>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; edk2-
>>> devel@lists.01.org; Leif Lindholm
>>> <leif.lindholm@linaro.org>; Zeng, Star
>>> <star.zeng@intel.com>
>>> Subject: Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei:
>>> clean Dcache before consuming capsule data
>>>
>>> On 11 June 2018 at 23:40, Kinney, Michael D
>>> <michael.d.kinney@intel.com> wrote:
>>> > Hi Ard,
>>> >
>>> > I would still prefer the cache maintenance be done
>>> independent
>>> > of capsules in case there is other memory ranges that
>>> need to
>>> > be flushed for other features.
>>> >
>>> > The CacheMaintenanceLib for ARM has several services
>>> that are
>>> > not implemented and ASSERT().  I found some ARM
>>> documentation
>>> > that says that caches can be flushed by looping
>>> through the
>>> > indexes.
>>> >
>>> >
>>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.
>>> doc.ddi0201d/ch03s03s05.html
>>> >
>>>
>>> Please don't quote severely outdated documentation out
>>> of context.
>>> ARM9 is a ~15 year old core that predates SMP on ARM by
>>> several years.
>>>
>>> > implementations that may not work with that algorithm.
>>> However
>>> > that could require a customized CacheMaintenanceLib.
>>> Can you
>>> > provide the flush all using the documented algorithm
>>> and use that
>>> > instance for systems that are compatible with the
>>> documented algorithm.
>>>
>>> None of the ARM cores we support in EDK2 today (ARMv7
>>> and later) are
>>> compatible with the documented algorithm.
>>>
>>> > Then, the flush all API can be used in ResetSystem()
>>> or SEC Phase
>>> > when the boot mode is warm reset.
>>> >
>>>
>>> Flushing the entire cache is simply not possible. The
>>> set/way
>>> operations are only intended to be used before the core
>>> enters the
>>> coherency domain or after it leaves it again (i.e., when
>>> cores are
>>> powered up or down). Those set/way operations are not
>>> broadcast to
>>> other cores (or other masters such as DMA capable
>>> peripherals with
>>> caches), which means that a cache line that is cleaned
>>> by set/way may
>>> simply end up in another cache and never make it to main
>>> memory. In
>>> other words, set/way operations manage the cache itself
>>> rather than
>>> the contents of the caches.
>>>
>>> The only way to perform cache maintenance in a way to
>>> ensure that the
>>> *content* makes it to main memory is to use cache
>>> maintenance by
>>> virtual address. This requires that you know the virtual
>>> address to
>>> begin with, and obviously requires that a mapping exists
>>> for that
>>> virtual address when executed with the MMU on.
>>>
>>> The bottom line is that there is no flush all API, and
>>> we will have to
>>> work around that (and believe me, this is not the first
>>> time we are
>>> struggling to deal with this limitation).
>>>
>>> So to summarize again, we have the following options,
>>> - UpdateCapsule - problematic because it may be called
>>> at runtime and
>>> the firmware has no means of translating the physical
>>> addresses
>>> - ResetSystem - same as above: it is a runtime service,
>>> and so it
>>> cannot rely on a mapping to exist for those physical
>>> addresses
>>> - SEC - lacks the information about where the capsule
>>> resides
>>> - CapsulePei - already extracts the information about
>>> the capsule
>>> address in memory, and can perform the cache maintenance
>>> right before
>>> consuming the data.

Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data

[Kinney, Michael D]

Ard,

The UEFI Spec allows UpdateCapsule() to return EFI_UNSUPPORTED.
So after ExitBootServices() UpdateCapsule() can return EFI_UNSUPPORTED for any capsule that has the CAPSULE_FLAGS_PERSIST_ACROSS_RESET flag set.

Mike

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Tuesday, June 12, 2018 3:32 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; edk2-
> devel@lists.01.org; Leif Lindholm
> <leif.lindholm@linaro.org>; Zeng, Star
> <star.zeng@intel.com>
> Subject: Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei:
> clean Dcache before consuming capsule data
> 
> On 12 June 2018 at 11:01, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> > On 12 June 2018 at 02:54, Kinney, Michael D
> <michael.d.kinney@intel.com> wrote:
> >> Ard,
> >>
> >> What about memory init when the memory HOBs are
> >> created.  Could you flush all the ranges of
> >> initialized memory by HOB as the HOBs are created?
> >>
> >> PEI and DXE can not use memory not described by
> >> the HOBs.  More memory can be added in DXE phase
> >> through the GCD services, and additional flush
> >> could be done as added.
> >>
> >
> > CapsuleCoalesce() goes and finds the capsule data
> wherever it was left
> > in memory by the OS. This is independent of what the
> HOBs may describe
> > or which memory was added at which point.
> >
> 
> As it turns out, relying on the state of the caches to
> be preserved
> across a reboot and cleaning them afterwards is
> something that cannot
> be architecturally guaranteed either. So I will withdraw
> this patch,
> and propose another approach that cleans the capsule
> during the call
> to UpdateCapsule(). For the time being, that only works
> at boot time,
> but given that both Windows and Linux perform capsule
> updates before
> ExitBootServices(), this is something we should be able
> to live with.
> 
> 
> >
> >>> -----Original Message-----
> >>> From: Ard Biesheuvel
> [mailto:ard.biesheuvel@linaro.org]
> >>> Sent: Monday, June 11, 2018 3:01 PM
> >>> To: Kinney, Michael D <michael.d.kinney@intel.com>
> >>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; edk2-
> >>> devel@lists.01.org; Leif Lindholm
> >>> <leif.lindholm@linaro.org>; Zeng, Star
> >>> <star.zeng@intel.com>
> >>> Subject: Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei:
> >>> clean Dcache before consuming capsule data
> >>>
> >>> On 11 June 2018 at 23:40, Kinney, Michael D
> >>> <michael.d.kinney@intel.com> wrote:
> >>> > Hi Ard,
> >>> >
> >>> > I would still prefer the cache maintenance be done
> >>> independent
> >>> > of capsules in case there is other memory ranges
> that
> >>> need to
> >>> > be flushed for other features.
> >>> >
> >>> > The CacheMaintenanceLib for ARM has several
> services
> >>> that are
> >>> > not implemented and ASSERT().  I found some ARM
> >>> documentation
> >>> > that says that caches can be flushed by looping
> >>> through the
> >>> > indexes.
> >>> >
> >>> >
> >>>
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.
> >>> doc.ddi0201d/ch03s03s05.html
> >>> >
> >>>
> >>> Please don't quote severely outdated documentation
> out
> >>> of context.
> >>> ARM9 is a ~15 year old core that predates SMP on ARM
> by
> >>> several years.
> >>>
> >>> > implementations that may not work with that
> algorithm.
> >>> However
> >>> > that could require a customized
> CacheMaintenanceLib.
> >>> Can you
> >>> > provide the flush all using the documented
> algorithm
> >>> and use that
> >>> > instance for systems that are compatible with the
> >>> documented algorithm.
> >>>
> >>> None of the ARM cores we support in EDK2 today
> (ARMv7
> >>> and later) are
> >>> compatible with the documented algorithm.
> >>>
> >>> > Then, the flush all API can be used in
> ResetSystem()
> >>> or SEC Phase
> >>> > when the boot mode is warm reset.
> >>> >
> >>>
> >>> Flushing the entire cache is simply not possible.
> The
> >>> set/way
> >>> operations are only intended to be used before the
> core
> >>> enters the
> >>> coherency domain or after it leaves it again (i.e.,
> when
> >>> cores are
> >>> powered up or down). Those set/way operations are
> not
> >>> broadcast to
> >>> other cores (or other masters such as DMA capable
> >>> peripherals with
> >>> caches), which means that a cache line that is
> cleaned
> >>> by set/way may
> >>> simply end up in another cache and never make it to
> main
> >>> memory. In
> >>> other words, set/way operations manage the cache
> itself
> >>> rather than
> >>> the contents of the caches.
> >>>
> >>> The only way to perform cache maintenance in a way
> to
> >>> ensure that the
> >>> *content* makes it to main memory is to use cache
> >>> maintenance by
> >>> virtual address. This requires that you know the
> virtual
> >>> address to
> >>> begin with, and obviously requires that a mapping
> exists
> >>> for that
> >>> virtual address when executed with the MMU on.
> >>>
> >>> The bottom line is that there is no flush all API,
> and
> >>> we will have to
> >>> work around that (and believe me, this is not the
> first
> >>> time we are
> >>> struggling to deal with this limitation).
> >>>
> >>> So to summarize again, we have the following
> options,
> >>> - UpdateCapsule - problematic because it may be
> called
> >>> at runtime and
> >>> the firmware has no means of translating the
> physical
> >>> addresses
> >>> - ResetSystem - same as above: it is a runtime
> service,
> >>> and so it
> >>> cannot rely on a mapping to exist for those physical
> >>> addresses
> >>> - SEC - lacks the information about where the
> capsule
> >>> resides
> >>> - CapsulePei - already extracts the information
> about
> >>> the capsule
> >>> address in memory, and can perform the cache
> maintenance
> >>> right before
> >>> consuming the data.

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

[Udit Kumar]

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

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

[Yao, Jiewen]

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