[RFC PATCH 0/3] enable IBT/BTI codegen and reporting to the OS

[RFC PATCH 0/3] enable IBT/BTI codegen and reporting to the OS

[Ard Biesheuvel]

Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Michael Kubacki <michael.kubacki@microsoft.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Rebecca Cran <quic_rcran@quicinc.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Taylor Beebe <t@taylorbeebe.com>

Ard Biesheuvel (3):
  MdePkg: Update MemoryAttributesTable to v2.10
  MdeModulePkg: Enable forward edge CFI in mem attributes table
  ArmVirtPkg/ArmVirtQemu: Implement BTI for runtime regions

 ArmVirtPkg/ArmVirtQemu.dsc                         |  8 ++++++++
 MdeModulePkg/Core/Dxe/DxeMain.h                    |  2 ++
 MdeModulePkg/Core/Dxe/DxeMain.inf                  |  1 +
 MdeModulePkg/Core/Dxe/Image/Image.c                | 11 +++++++++++
 MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c |  7 ++++++-
 MdeModulePkg/MdeModulePkg.dec                      |  8 ++++++++
 MdePkg/Include/Guid/MemoryAttributesTable.h        |  8 ++++++--
 7 files changed, 42 insertions(+), 3 deletions(-)

-- 
2.39.1

[RFC PATCH 1/3] MdePkg: Update MemoryAttributesTable to v2.10

[Ard Biesheuvel]

UEFI v2.10 introduces a new flag to the memory attributes table to
inform the OS whether or not runtime services code regions were emitted
by the compiler with guard instructions for forward edge control flow
integrity enforcement.

So update our definition accordingly.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdePkg/Include/Guid/MemoryAttributesTable.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Include/Guid/MemoryAttributesTable.h b/MdePkg/Include/Guid/MemoryAttributesTable.h
index 82f83a67b96d..238c14ff92df 100644
--- a/MdePkg/Include/Guid/MemoryAttributesTable.h
+++ b/MdePkg/Include/Guid/MemoryAttributesTable.h
@@ -17,11 +17,15 @@ typedef struct {
   UINT32    Version;
   UINT32    NumberOfEntries;
   UINT32    DescriptorSize;
-  UINT32    Reserved;
+  UINT32    Flags;
   // EFI_MEMORY_DESCRIPTOR Entry[1];
 } EFI_MEMORY_ATTRIBUTES_TABLE;
 
-#define EFI_MEMORY_ATTRIBUTES_TABLE_VERSION  0x00000001
+#define EFI_MEMORY_ATTRIBUTES_TABLE_VERSION  0x00000002
+
+#define EFI_MEMORY_ATTRIBUTES_FLAGS_RT_FORWARD_CONTROL_FLOW_GUARD  0x1
+// BIT0 implies that Runtime code includes the forward control flow guard
+// instruction, such as X86 CET-IBT or ARM BTI.
 
 extern EFI_GUID  gEfiMemoryAttributesTableGuid;
 
-- 
2.39.1

Re: [edk2-devel] [RFC PATCH 1/3] MdePkg: Update MemoryAttributesTable to v2.10

[Michael D Kinney]

Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
> Sent: Thursday, February 2, 2023 10:04 AM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ardb@kernel.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Yao,
> Jiewen <jiewen.yao@intel.com>; Kubacki, Michael <michael.kubacki@microsoft.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca
> Cran <quic_rcran@quicinc.com>; Leif Lindholm <quic_llindhol@quicinc.com>; Sami Mujawar <sami.mujawar@arm.com>; Taylor Beebe
> <t@taylorbeebe.com>
> Subject: [edk2-devel] [RFC PATCH 1/3] MdePkg: Update MemoryAttributesTable to v2.10
> 
> UEFI v2.10 introduces a new flag to the memory attributes table to
> inform the OS whether or not runtime services code regions were emitted
> by the compiler with guard instructions for forward edge control flow
> integrity enforcement.
> 
> So update our definition accordingly.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  MdePkg/Include/Guid/MemoryAttributesTable.h | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/MdePkg/Include/Guid/MemoryAttributesTable.h b/MdePkg/Include/Guid/MemoryAttributesTable.h
> index 82f83a67b96d..238c14ff92df 100644
> --- a/MdePkg/Include/Guid/MemoryAttributesTable.h
> +++ b/MdePkg/Include/Guid/MemoryAttributesTable.h
> @@ -17,11 +17,15 @@ typedef struct {
>    UINT32    Version;
> 
>    UINT32    NumberOfEntries;
> 
>    UINT32    DescriptorSize;
> 
> -  UINT32    Reserved;
> 
> +  UINT32    Flags;
> 
>    // EFI_MEMORY_DESCRIPTOR Entry[1];
> 
>  } EFI_MEMORY_ATTRIBUTES_TABLE;
> 
> 
> 
> -#define EFI_MEMORY_ATTRIBUTES_TABLE_VERSION  0x00000001
> 
> +#define EFI_MEMORY_ATTRIBUTES_TABLE_VERSION  0x00000002
> 
> +
> 
> +#define EFI_MEMORY_ATTRIBUTES_FLAGS_RT_FORWARD_CONTROL_FLOW_GUARD  0x1
> 
> +// BIT0 implies that Runtime code includes the forward control flow guard
> 
> +// instruction, such as X86 CET-IBT or ARM BTI.
> 
> 
> 
>  extern EFI_GUID  gEfiMemoryAttributesTableGuid;
> 
> 
> 
> --
> 2.39.1
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#99496): https://edk2.groups.io/g/devel/message/99496
> Mute This Topic: https://groups.io/mt/96705497/1643496
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com]
> -=-=-=-=-=-=
> 

Re: [edk2-devel] [RFC PATCH 1/3] MdePkg: Update MemoryAttributesTable to v2.10

[Michael Kubacki]

Acked-by: Michael Kubacki <michael.kubacki@microsoft.com>

Might be convenient to have a link in the commit message to the 
definition in the 2.10 spec:

https://uefi.org/specs/UEFI/2.10/04_EFI_System_Table.html?highlight=memory_attribute#efi-memory-attributes-table

On 2/2/2023 1:03 PM, Ard Biesheuvel wrote:
> UEFI v2.10 introduces a new flag to the memory attributes table to
> inform the OS whether or not runtime services code regions were emitted
> by the compiler with guard instructions for forward edge control flow
> integrity enforcement.
> 
> So update our definition accordingly.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>   MdePkg/Include/Guid/MemoryAttributesTable.h | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/MdePkg/Include/Guid/MemoryAttributesTable.h b/MdePkg/Include/Guid/MemoryAttributesTable.h
> index 82f83a67b96d..238c14ff92df 100644
> --- a/MdePkg/Include/Guid/MemoryAttributesTable.h
> +++ b/MdePkg/Include/Guid/MemoryAttributesTable.h
> @@ -17,11 +17,15 @@ typedef struct {
>     UINT32    Version;
> 
>     UINT32    NumberOfEntries;
> 
>     UINT32    DescriptorSize;
> 
> -  UINT32    Reserved;
> 
> +  UINT32    Flags;
> 
>     // EFI_MEMORY_DESCRIPTOR Entry[1];
> 
>   } EFI_MEMORY_ATTRIBUTES_TABLE;
> 
>   
> 
> -#define EFI_MEMORY_ATTRIBUTES_TABLE_VERSION  0x00000001
> 
> +#define EFI_MEMORY_ATTRIBUTES_TABLE_VERSION  0x00000002
> 
> +
> 
> +#define EFI_MEMORY_ATTRIBUTES_FLAGS_RT_FORWARD_CONTROL_FLOW_GUARD  0x1
> 
> +// BIT0 implies that Runtime code includes the forward control flow guard
> 
> +// instruction, such as X86 CET-IBT or ARM BTI.
> 
>   
> 
>   extern EFI_GUID  gEfiMemoryAttributesTableGuid;
> 
>   
> 

[RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem attributes table

[Ard Biesheuvel]

The memory attributes table has been extended with a flag that indicates
whether or not the OS is permitted to map the EFI runtime code regions
with strict enforcement for IBT/BTI landing pad instructions.

This is generally a property of the firmware build, and so we can permit
a platform to set this property using a PCD, and put the burden on the
platform definition to set the toolchain options accordingly.

There is one snag, however: PE/COFF does not expose whether or not the
code was generated with landing pads, so if any runtime DXE drivers were
loaded from storage other than the firmware volumes, we must assume that
setting the CFI flag in the memory attributes table is unsafe.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdeModulePkg/Core/Dxe/DxeMain.h                    |  2 ++
 MdeModulePkg/Core/Dxe/DxeMain.inf                  |  1 +
 MdeModulePkg/Core/Dxe/Image/Image.c                | 11 +++++++++++
 MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c |  7 ++++++-
 MdeModulePkg/MdeModulePkg.dec                      |  8 ++++++++
 5 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
index 815a6b4bd844..427a5fc78f72 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.h
+++ b/MdeModulePkg/Core/Dxe/DxeMain.h
@@ -280,6 +280,8 @@ extern EFI_MEMORY_TYPE_INFORMATION  gMemoryTypeInformation[EfiMaxMemoryType + 1]
 extern BOOLEAN                    gDispatcherRunning;
 extern EFI_RUNTIME_ARCH_PROTOCOL  gRuntimeTemplate;
 
+extern BOOLEAN                    gMemoryAttributesTableForwardCfi;
+
 extern EFI_LOAD_FIXED_ADDRESS_CONFIGURATION_TABLE  gLoadModuleAtFixAddressConfigurationTable;
 extern BOOLEAN                                     gLoadFixedAddressCodeMemoryReady;
 //
diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
index 35d5bf0dee6f..e6ff67773a69 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.inf
+++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
@@ -187,6 +187,7 @@ [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask                   ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                           ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth           ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi         ## CONSUMES
 
 # [Hob]
 # RESOURCE_DESCRIPTOR   ## CONSUMES
diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
index 06cc6744b8c6..181fefdb6657 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -1398,6 +1398,17 @@ CoreLoadImageCommon (
     CoreNewDebugImageInfoEntry (EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL, &Image->Info, Image->Handle);
   }
 
+  //
+  // If we loaded a runtime DXE driver from something other than a FV, it
+  // was not built as part of the firmware image, and so we cannot assume
+  // that it was built with IBT/BTI landing pads for forward edge control
+  // flow integrity.
+  //
+  if (!ImageIsFromFv &&
+      (Image->ImageContext.ImageCodeMemoryType == EfiRuntimeServicesCode)) {
+    gMemoryAttributesTableForwardCfi = FALSE;
+  }
+
   //
   // Reinstall loaded image protocol to fire any notifications
   //
diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
index e07921371187..cdd35ade0a8a 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
@@ -89,6 +89,7 @@ BOOLEAN                      mMemoryAttributesTableEnable      = TRUE;
 BOOLEAN                      mMemoryAttributesTableEndOfDxe    = FALSE;
 EFI_MEMORY_ATTRIBUTES_TABLE  *mMemoryAttributesTable           = NULL;
 BOOLEAN                      mMemoryAttributesTableReadyToBoot = FALSE;
+BOOLEAN                      gMemoryAttributesTableForwardCfi  = FixedPcdGetBool (PcdMemoryAttributesTableForwardCfi);
 
 /**
   Install MemoryAttributesTable.
@@ -182,7 +183,11 @@ InstallMemoryAttributesTable (
   MemoryAttributesTable->Version         = EFI_MEMORY_ATTRIBUTES_TABLE_VERSION;
   MemoryAttributesTable->NumberOfEntries = RuntimeEntryCount;
   MemoryAttributesTable->DescriptorSize  = (UINT32)DescriptorSize;
-  MemoryAttributesTable->Reserved        = 0;
+  if (gMemoryAttributesTableForwardCfi) {
+    MemoryAttributesTable->Flags         = EFI_MEMORY_ATTRIBUTES_FLAGS_RT_FORWARD_CONTROL_FLOW_GUARD;
+  } else {
+    MemoryAttributesTable->Flags         = 0;
+  }
   DEBUG ((DEBUG_VERBOSE, "MemoryAttributesTable:\n"));
   DEBUG ((DEBUG_VERBOSE, "  Version              - 0x%08x\n", MemoryAttributesTable->Version));
   DEBUG ((DEBUG_VERBOSE, "  NumberOfEntries      - 0x%08x\n", MemoryAttributesTable->NumberOfEntries));
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 9605c617b7a8..d336a38655a3 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1093,6 +1093,14 @@ [PcdsFixedAtBuild]
   # @Prompt Enable UEFI Stack Guard.
   gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0x30001055
 
+  ## Indicates whether the EFI memory attributes table will inform the OS that
+  #  forward edge control flow guards have been inserted into the runtime services
+  #  code regions.
+  #   TRUE  - Runtime code has forward control flow guards.<BR>
+  #   FALSE - Runtime code does not have forward control flow guards.<BR>
+  # @Prompt Enable forward control flow guards in EFI memory attributes table
+  gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi|FALSE|BOOLEAN|0x30001056
+
 [PcdsFixedAtBuild, PcdsPatchableInModule]
   ## Dynamic type PCD can be registered callback function for Pcd setting action.
   #  PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of callback function
-- 
2.39.1

Re: [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem attributes table

[Michael D Kinney]

Hi Ard,

Since the PE/COFF image does not contain this information, is there an option
to add the information to an FFS file. Either as a new bit in a standard header
or as a GUIDed section defined by EDK II?

Since an FV may contain content build from source and additional content 
Integrated as binaries from other vendors, having a PCD that scopes this
attribute to all FVs many only work for FW builds that are 100% from sources.

Mike


> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Thursday, February 2, 2023 10:04 AM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ardb@kernel.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Yao,
> Jiewen <jiewen.yao@intel.com>; Kubacki, Michael <michael.kubacki@microsoft.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca
> Cran <quic_rcran@quicinc.com>; Leif Lindholm <quic_llindhol@quicinc.com>; Sami Mujawar <sami.mujawar@arm.com>; Taylor Beebe
> <t@taylorbeebe.com>
> Subject: [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem attributes table
> 
> The memory attributes table has been extended with a flag that indicates
> whether or not the OS is permitted to map the EFI runtime code regions
> with strict enforcement for IBT/BTI landing pad instructions.
> 
> This is generally a property of the firmware build, and so we can permit
> a platform to set this property using a PCD, and put the burden on the
> platform definition to set the toolchain options accordingly.
> 
> There is one snag, however: PE/COFF does not expose whether or not the
> code was generated with landing pads, so if any runtime DXE drivers were
> loaded from storage other than the firmware volumes, we must assume that
> setting the CFI flag in the memory attributes table is unsafe.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  MdeModulePkg/Core/Dxe/DxeMain.h                    |  2 ++
>  MdeModulePkg/Core/Dxe/DxeMain.inf                  |  1 +
>  MdeModulePkg/Core/Dxe/Image/Image.c                | 11 +++++++++++
>  MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c |  7 ++++++-
>  MdeModulePkg/MdeModulePkg.dec                      |  8 ++++++++
>  5 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
> index 815a6b4bd844..427a5fc78f72 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.h
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
> @@ -280,6 +280,8 @@ extern EFI_MEMORY_TYPE_INFORMATION  gMemoryTypeInformation[EfiMaxMemoryType + 1]
>  extern BOOLEAN                    gDispatcherRunning;
> 
>  extern EFI_RUNTIME_ARCH_PROTOCOL  gRuntimeTemplate;
> 
> 
> 
> +extern BOOLEAN                    gMemoryAttributesTableForwardCfi;
> 
> +
> 
>  extern EFI_LOAD_FIXED_ADDRESS_CONFIGURATION_TABLE  gLoadModuleAtFixAddressConfigurationTable;
> 
>  extern BOOLEAN                                     gLoadFixedAddressCodeMemoryReady;
> 
>  //
> 
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
> index 35d5bf0dee6f..e6ff67773a69 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> @@ -187,6 +187,7 @@ [Pcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask                   ## CONSUMES
> 
>    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                           ## CONSUMES
> 
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth           ## CONSUMES
> 
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi         ## CONSUMES
> 
> 
> 
>  # [Hob]
> 
>  # RESOURCE_DESCRIPTOR   ## CONSUMES
> 
> diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
> index 06cc6744b8c6..181fefdb6657 100644
> --- a/MdeModulePkg/Core/Dxe/Image/Image.c
> +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
> @@ -1398,6 +1398,17 @@ CoreLoadImageCommon (
>      CoreNewDebugImageInfoEntry (EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL, &Image->Info, Image->Handle);
> 
>    }
> 
> 
> 
> +  //
> 
> +  // If we loaded a runtime DXE driver from something other than a FV, it
> 
> +  // was not built as part of the firmware image, and so we cannot assume
> 
> +  // that it was built with IBT/BTI landing pads for forward edge control
> 
> +  // flow integrity.
> 
> +  //
> 
> +  if (!ImageIsFromFv &&
> 
> +      (Image->ImageContext.ImageCodeMemoryType == EfiRuntimeServicesCode)) {
> 
> +    gMemoryAttributesTableForwardCfi = FALSE;
> 
> +  }
> 
> +
> 
>    //
> 
>    // Reinstall loaded image protocol to fire any notifications
> 
>    //
> 
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
> index e07921371187..cdd35ade0a8a 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
> @@ -89,6 +89,7 @@ BOOLEAN                      mMemoryAttributesTableEnable      = TRUE;
>  BOOLEAN                      mMemoryAttributesTableEndOfDxe    = FALSE;
> 
>  EFI_MEMORY_ATTRIBUTES_TABLE  *mMemoryAttributesTable           = NULL;
> 
>  BOOLEAN                      mMemoryAttributesTableReadyToBoot = FALSE;
> 
> +BOOLEAN                      gMemoryAttributesTableForwardCfi  = FixedPcdGetBool (PcdMemoryAttributesTableForwardCfi);
> 
> 
> 
>  /**
> 
>    Install MemoryAttributesTable.
> 
> @@ -182,7 +183,11 @@ InstallMemoryAttributesTable (
>    MemoryAttributesTable->Version         = EFI_MEMORY_ATTRIBUTES_TABLE_VERSION;
> 
>    MemoryAttributesTable->NumberOfEntries = RuntimeEntryCount;
> 
>    MemoryAttributesTable->DescriptorSize  = (UINT32)DescriptorSize;
> 
> -  MemoryAttributesTable->Reserved        = 0;
> 
> +  if (gMemoryAttributesTableForwardCfi) {
> 
> +    MemoryAttributesTable->Flags         = EFI_MEMORY_ATTRIBUTES_FLAGS_RT_FORWARD_CONTROL_FLOW_GUARD;
> 
> +  } else {
> 
> +    MemoryAttributesTable->Flags         = 0;
> 
> +  }
> 
>    DEBUG ((DEBUG_VERBOSE, "MemoryAttributesTable:\n"));
> 
>    DEBUG ((DEBUG_VERBOSE, "  Version              - 0x%08x\n", MemoryAttributesTable->Version));
> 
>    DEBUG ((DEBUG_VERBOSE, "  NumberOfEntries      - 0x%08x\n", MemoryAttributesTable->NumberOfEntries));
> 
> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> index 9605c617b7a8..d336a38655a3 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -1093,6 +1093,14 @@ [PcdsFixedAtBuild]
>    # @Prompt Enable UEFI Stack Guard.
> 
>    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0x30001055
> 
> 
> 
> +  ## Indicates whether the EFI memory attributes table will inform the OS that
> 
> +  #  forward edge control flow guards have been inserted into the runtime services
> 
> +  #  code regions.
> 
> +  #   TRUE  - Runtime code has forward control flow guards.<BR>
> 
> +  #   FALSE - Runtime code does not have forward control flow guards.<BR>
> 
> +  # @Prompt Enable forward control flow guards in EFI memory attributes table
> 
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi|FALSE|BOOLEAN|0x30001056
> 
> +
> 
>  [PcdsFixedAtBuild, PcdsPatchableInModule]
> 
>    ## Dynamic type PCD can be registered callback function for Pcd setting action.
> 
>    #  PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of callback function
> 
> --
> 2.39.1

Re: [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem attributes table

[Ard Biesheuvel]

On Thu, 2 Feb 2023 at 19:49, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
>
> Hi Ard,
>
> Since the PE/COFF image does not contain this information, is there an option
> to add the information to an FFS file. Either as a new bit in a standard header
> or as a GUIDed section defined by EDK II?
>
> Since an FV may contain content build from source and additional content
> Integrated as binaries from other vendors, having a PCD that scopes this
> attribute to all FVs many only work for FW builds that are 100% from sources.
>

I didn't quite consider that scenario tbh.

It would be nice if we could avoid another PI spec dance for this, and
get some kind of IBT/BTI annotation added to the PE/COFF spec. That
way, we cover this issue, as well as clear the way for the use if
IBT/BTI when running under the firmware.

Are TE images a concern here as well? I.e., are those likely to be
used for runtime DXE images in firmware volumes?


>
> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > Sent: Thursday, February 2, 2023 10:04 AM
> > To: devel@edk2.groups.io
> > Cc: Ard Biesheuvel <ardb@kernel.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Yao,
> > Jiewen <jiewen.yao@intel.com>; Kubacki, Michael <michael.kubacki@microsoft.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca
> > Cran <quic_rcran@quicinc.com>; Leif Lindholm <quic_llindhol@quicinc.com>; Sami Mujawar <sami.mujawar@arm.com>; Taylor Beebe
> > <t@taylorbeebe.com>
> > Subject: [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem attributes table
> >
> > The memory attributes table has been extended with a flag that indicates
> > whether or not the OS is permitted to map the EFI runtime code regions
> > with strict enforcement for IBT/BTI landing pad instructions.
> >
> > This is generally a property of the firmware build, and so we can permit
> > a platform to set this property using a PCD, and put the burden on the
> > platform definition to set the toolchain options accordingly.
> >
> > There is one snag, however: PE/COFF does not expose whether or not the
> > code was generated with landing pads, so if any runtime DXE drivers were
> > loaded from storage other than the firmware volumes, we must assume that
> > setting the CFI flag in the memory attributes table is unsafe.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  MdeModulePkg/Core/Dxe/DxeMain.h                    |  2 ++
> >  MdeModulePkg/Core/Dxe/DxeMain.inf                  |  1 +
> >  MdeModulePkg/Core/Dxe/Image/Image.c                | 11 +++++++++++
> >  MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c |  7 ++++++-
> >  MdeModulePkg/MdeModulePkg.dec                      |  8 ++++++++
> >  5 files changed, 28 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
> > index 815a6b4bd844..427a5fc78f72 100644
> > --- a/MdeModulePkg/Core/Dxe/DxeMain.h
> > +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
> > @@ -280,6 +280,8 @@ extern EFI_MEMORY_TYPE_INFORMATION  gMemoryTypeInformation[EfiMaxMemoryType + 1]
> >  extern BOOLEAN                    gDispatcherRunning;
> >
> >  extern EFI_RUNTIME_ARCH_PROTOCOL  gRuntimeTemplate;
> >
> >
> >
> > +extern BOOLEAN                    gMemoryAttributesTableForwardCfi;
> >
> > +
> >
> >  extern EFI_LOAD_FIXED_ADDRESS_CONFIGURATION_TABLE  gLoadModuleAtFixAddressConfigurationTable;
> >
> >  extern BOOLEAN                                     gLoadFixedAddressCodeMemoryReady;
> >
> >  //
> >
> > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > index 35d5bf0dee6f..e6ff67773a69 100644
> > --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> > +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > @@ -187,6 +187,7 @@ [Pcd]
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask                   ## CONSUMES
> >
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                           ## CONSUMES
> >
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth           ## CONSUMES
> >
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi         ## CONSUMES
> >
> >
> >
> >  # [Hob]
> >
> >  # RESOURCE_DESCRIPTOR   ## CONSUMES
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
> > index 06cc6744b8c6..181fefdb6657 100644
> > --- a/MdeModulePkg/Core/Dxe/Image/Image.c
> > +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
> > @@ -1398,6 +1398,17 @@ CoreLoadImageCommon (
> >      CoreNewDebugImageInfoEntry (EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL, &Image->Info, Image->Handle);
> >
> >    }
> >
> >
> >
> > +  //
> >
> > +  // If we loaded a runtime DXE driver from something other than a FV, it
> >
> > +  // was not built as part of the firmware image, and so we cannot assume
> >
> > +  // that it was built with IBT/BTI landing pads for forward edge control
> >
> > +  // flow integrity.
> >
> > +  //
> >
> > +  if (!ImageIsFromFv &&
> >
> > +      (Image->ImageContext.ImageCodeMemoryType == EfiRuntimeServicesCode)) {
> >
> > +    gMemoryAttributesTableForwardCfi = FALSE;
> >
> > +  }
> >
> > +
> >
> >    //
> >
> >    // Reinstall loaded image protocol to fire any notifications
> >
> >    //
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
> > index e07921371187..cdd35ade0a8a 100644
> > --- a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
> > +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
> > @@ -89,6 +89,7 @@ BOOLEAN                      mMemoryAttributesTableEnable      = TRUE;
> >  BOOLEAN                      mMemoryAttributesTableEndOfDxe    = FALSE;
> >
> >  EFI_MEMORY_ATTRIBUTES_TABLE  *mMemoryAttributesTable           = NULL;
> >
> >  BOOLEAN                      mMemoryAttributesTableReadyToBoot = FALSE;
> >
> > +BOOLEAN                      gMemoryAttributesTableForwardCfi  = FixedPcdGetBool (PcdMemoryAttributesTableForwardCfi);
> >
> >
> >
> >  /**
> >
> >    Install MemoryAttributesTable.
> >
> > @@ -182,7 +183,11 @@ InstallMemoryAttributesTable (
> >    MemoryAttributesTable->Version         = EFI_MEMORY_ATTRIBUTES_TABLE_VERSION;
> >
> >    MemoryAttributesTable->NumberOfEntries = RuntimeEntryCount;
> >
> >    MemoryAttributesTable->DescriptorSize  = (UINT32)DescriptorSize;
> >
> > -  MemoryAttributesTable->Reserved        = 0;
> >
> > +  if (gMemoryAttributesTableForwardCfi) {
> >
> > +    MemoryAttributesTable->Flags         = EFI_MEMORY_ATTRIBUTES_FLAGS_RT_FORWARD_CONTROL_FLOW_GUARD;
> >
> > +  } else {
> >
> > +    MemoryAttributesTable->Flags         = 0;
> >
> > +  }
> >
> >    DEBUG ((DEBUG_VERBOSE, "MemoryAttributesTable:\n"));
> >
> >    DEBUG ((DEBUG_VERBOSE, "  Version              - 0x%08x\n", MemoryAttributesTable->Version));
> >
> >    DEBUG ((DEBUG_VERBOSE, "  NumberOfEntries      - 0x%08x\n", MemoryAttributesTable->NumberOfEntries));
> >
> > diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> > index 9605c617b7a8..d336a38655a3 100644
> > --- a/MdeModulePkg/MdeModulePkg.dec
> > +++ b/MdeModulePkg/MdeModulePkg.dec
> > @@ -1093,6 +1093,14 @@ [PcdsFixedAtBuild]
> >    # @Prompt Enable UEFI Stack Guard.
> >
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0x30001055
> >
> >
> >
> > +  ## Indicates whether the EFI memory attributes table will inform the OS that
> >
> > +  #  forward edge control flow guards have been inserted into the runtime services
> >
> > +  #  code regions.
> >
> > +  #   TRUE  - Runtime code has forward control flow guards.<BR>
> >
> > +  #   FALSE - Runtime code does not have forward control flow guards.<BR>
> >
> > +  # @Prompt Enable forward control flow guards in EFI memory attributes table
> >
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi|FALSE|BOOLEAN|0x30001056
> >
> > +
> >
> >  [PcdsFixedAtBuild, PcdsPatchableInModule]
> >
> >    ## Dynamic type PCD can be registered callback function for Pcd setting action.
> >
> >    #  PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of callback function
> >
> > --
> > 2.39.1
>

Re: [edk2-devel] [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem attributes table

[Michael Kubacki]

Ard, I am still actively tracking this for the PE/COFF spec.

Unfortunately, I don't have more firm info right now but I suggest 
holding off on alternatives for the time being and I will reply back as 
soon as the next steps are known.

Thanks,
Michael

On 2/2/2023 2:00 PM, Ard Biesheuvel wrote:
> On Thu, 2 Feb 2023 at 19:49, Kinney, Michael D
> <michael.d.kinney@intel.com> wrote:
>>
>> Hi Ard,
>>
>> Since the PE/COFF image does not contain this information, is there an option
>> to add the information to an FFS file. Either as a new bit in a standard header
>> or as a GUIDed section defined by EDK II?
>>
>> Since an FV may contain content build from source and additional content
>> Integrated as binaries from other vendors, having a PCD that scopes this
>> attribute to all FVs many only work for FW builds that are 100% from sources.
>>
> 
> I didn't quite consider that scenario tbh.
> 
> It would be nice if we could avoid another PI spec dance for this, and
> get some kind of IBT/BTI annotation added to the PE/COFF spec. That
> way, we cover this issue, as well as clear the way for the use if
> IBT/BTI when running under the firmware.
> 
> Are TE images a concern here as well? I.e., are those likely to be
> used for runtime DXE images in firmware volumes?
> 
> 
>>
>>> -----Original Message-----
>>> From: Ard Biesheuvel <ardb@kernel.org>
>>> Sent: Thursday, February 2, 2023 10:04 AM
>>> To: devel@edk2.groups.io
>>> Cc: Ard Biesheuvel <ardb@kernel.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Yao,
>>> Jiewen <jiewen.yao@intel.com>; Kubacki, Michael <michael.kubacki@microsoft.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca
>>> Cran <quic_rcran@quicinc.com>; Leif Lindholm <quic_llindhol@quicinc.com>; Sami Mujawar <sami.mujawar@arm.com>; Taylor Beebe
>>> <t@taylorbeebe.com>
>>> Subject: [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem attributes table
>>>
>>> The memory attributes table has been extended with a flag that indicates
>>> whether or not the OS is permitted to map the EFI runtime code regions
>>> with strict enforcement for IBT/BTI landing pad instructions.
>>>
>>> This is generally a property of the firmware build, and so we can permit
>>> a platform to set this property using a PCD, and put the burden on the
>>> platform definition to set the toolchain options accordingly.
>>>
>>> There is one snag, however: PE/COFF does not expose whether or not the
>>> code was generated with landing pads, so if any runtime DXE drivers were
>>> loaded from storage other than the firmware volumes, we must assume that
>>> setting the CFI flag in the memory attributes table is unsafe.
>>>
>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>> ---
>>>   MdeModulePkg/Core/Dxe/DxeMain.h                    |  2 ++
>>>   MdeModulePkg/Core/Dxe/DxeMain.inf                  |  1 +
>>>   MdeModulePkg/Core/Dxe/Image/Image.c                | 11 +++++++++++
>>>   MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c |  7 ++++++-
>>>   MdeModulePkg/MdeModulePkg.dec                      |  8 ++++++++
>>>   5 files changed, 28 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
>>> index 815a6b4bd844..427a5fc78f72 100644
>>> --- a/MdeModulePkg/Core/Dxe/DxeMain.h
>>> +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
>>> @@ -280,6 +280,8 @@ extern EFI_MEMORY_TYPE_INFORMATION  gMemoryTypeInformation[EfiMaxMemoryType + 1]
>>>   extern BOOLEAN                    gDispatcherRunning;
>>>
>>>   extern EFI_RUNTIME_ARCH_PROTOCOL  gRuntimeTemplate;
>>>
>>>
>>>
>>> +extern BOOLEAN                    gMemoryAttributesTableForwardCfi;
>>>
>>> +
>>>
>>>   extern EFI_LOAD_FIXED_ADDRESS_CONFIGURATION_TABLE  gLoadModuleAtFixAddressConfigurationTable;
>>>
>>>   extern BOOLEAN                                     gLoadFixedAddressCodeMemoryReady;
>>>
>>>   //
>>>
>>> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
>>> index 35d5bf0dee6f..e6ff67773a69 100644
>>> --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
>>> +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
>>> @@ -187,6 +187,7 @@ [Pcd]
>>>     gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask                   ## CONSUMES
>>>
>>>     gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                           ## CONSUMES
>>>
>>>     gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth           ## CONSUMES
>>>
>>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi         ## CONSUMES
>>>
>>>
>>>
>>>   # [Hob]
>>>
>>>   # RESOURCE_DESCRIPTOR   ## CONSUMES
>>>
>>> diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
>>> index 06cc6744b8c6..181fefdb6657 100644
>>> --- a/MdeModulePkg/Core/Dxe/Image/Image.c
>>> +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
>>> @@ -1398,6 +1398,17 @@ CoreLoadImageCommon (
>>>       CoreNewDebugImageInfoEntry (EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL, &Image->Info, Image->Handle);
>>>
>>>     }
>>>
>>>
>>>
>>> +  //
>>>
>>> +  // If we loaded a runtime DXE driver from something other than a FV, it
>>>
>>> +  // was not built as part of the firmware image, and so we cannot assume
>>>
>>> +  // that it was built with IBT/BTI landing pads for forward edge control
>>>
>>> +  // flow integrity.
>>>
>>> +  //
>>>
>>> +  if (!ImageIsFromFv &&
>>>
>>> +      (Image->ImageContext.ImageCodeMemoryType == EfiRuntimeServicesCode)) {
>>>
>>> +    gMemoryAttributesTableForwardCfi = FALSE;
>>>
>>> +  }
>>>
>>> +
>>>
>>>     //
>>>
>>>     // Reinstall loaded image protocol to fire any notifications
>>>
>>>     //
>>>
>>> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
>>> index e07921371187..cdd35ade0a8a 100644
>>> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
>>> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
>>> @@ -89,6 +89,7 @@ BOOLEAN                      mMemoryAttributesTableEnable      = TRUE;
>>>   BOOLEAN                      mMemoryAttributesTableEndOfDxe    = FALSE;
>>>
>>>   EFI_MEMORY_ATTRIBUTES_TABLE  *mMemoryAttributesTable           = NULL;
>>>
>>>   BOOLEAN                      mMemoryAttributesTableReadyToBoot = FALSE;
>>>
>>> +BOOLEAN                      gMemoryAttributesTableForwardCfi  = FixedPcdGetBool (PcdMemoryAttributesTableForwardCfi);
>>>
>>>
>>>
>>>   /**
>>>
>>>     Install MemoryAttributesTable.
>>>
>>> @@ -182,7 +183,11 @@ InstallMemoryAttributesTable (
>>>     MemoryAttributesTable->Version         = EFI_MEMORY_ATTRIBUTES_TABLE_VERSION;
>>>
>>>     MemoryAttributesTable->NumberOfEntries = RuntimeEntryCount;
>>>
>>>     MemoryAttributesTable->DescriptorSize  = (UINT32)DescriptorSize;
>>>
>>> -  MemoryAttributesTable->Reserved        = 0;
>>>
>>> +  if (gMemoryAttributesTableForwardCfi) {
>>>
>>> +    MemoryAttributesTable->Flags         = EFI_MEMORY_ATTRIBUTES_FLAGS_RT_FORWARD_CONTROL_FLOW_GUARD;
>>>
>>> +  } else {
>>>
>>> +    MemoryAttributesTable->Flags         = 0;
>>>
>>> +  }
>>>
>>>     DEBUG ((DEBUG_VERBOSE, "MemoryAttributesTable:\n"));
>>>
>>>     DEBUG ((DEBUG_VERBOSE, "  Version              - 0x%08x\n", MemoryAttributesTable->Version));
>>>
>>>     DEBUG ((DEBUG_VERBOSE, "  NumberOfEntries      - 0x%08x\n", MemoryAttributesTable->NumberOfEntries));
>>>
>>> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
>>> index 9605c617b7a8..d336a38655a3 100644
>>> --- a/MdeModulePkg/MdeModulePkg.dec
>>> +++ b/MdeModulePkg/MdeModulePkg.dec
>>> @@ -1093,6 +1093,14 @@ [PcdsFixedAtBuild]
>>>     # @Prompt Enable UEFI Stack Guard.
>>>
>>>     gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0x30001055
>>>
>>>
>>>
>>> +  ## Indicates whether the EFI memory attributes table will inform the OS that
>>>
>>> +  #  forward edge control flow guards have been inserted into the runtime services
>>>
>>> +  #  code regions.
>>>
>>> +  #   TRUE  - Runtime code has forward control flow guards.<BR>
>>>
>>> +  #   FALSE - Runtime code does not have forward control flow guards.<BR>
>>>
>>> +  # @Prompt Enable forward control flow guards in EFI memory attributes table
>>>
>>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi|FALSE|BOOLEAN|0x30001056
>>>
>>> +
>>>
>>>   [PcdsFixedAtBuild, PcdsPatchableInModule]
>>>
>>>     ## Dynamic type PCD can be registered callback function for Pcd setting action.
>>>
>>>     #  PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of callback function
>>>
>>> --
>>> 2.39.1
>>
> 
> 
> 
> 

Re: [edk2-devel] [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem attributes table

[Yao, Jiewen]

Hello
Can we assume that the entrypoint of PE/COFF image is always ENDBR64, if the PE/COFF image is enlightened to support IBT?

I believe the compiler should do that, because the loader need use indirect call to the PE/COFF entrypoint.

We need more code to detect *all* runtime images. The logic could be:
1) PE/COFF loader (X86, ARM) detects IBT capability for all runtime images, and report it.
2) DXE Core collects such info.
2.1) If ALL runtime image supports IBT, then gMemoryAttributesTableForwardCfi = TRUE
2.2) Else, gMemoryAttributesTableForwardCfi = FALSE.

The benefit is that it is more reliable to support binary PE image use case. Hard to use build time flag for external binary PE ...

Thank you
Yao, Jiewen

> -----Original Message-----
> From: Michael Kubacki <mikuback@linux.microsoft.com>
> Sent: Friday, February 3, 2023 8:24 AM
> To: devel@edk2.groups.io; ardb@kernel.org; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: Gao, Liming <gaoliming@byosoft.com.cn>; Yao, Jiewen
> <jiewen.yao@intel.com>; Kubacki, Michael <michael.kubacki@microsoft.com>;
> Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran
> <quic_rcran@quicinc.com>; Leif Lindholm <quic_llindhol@quicinc.com>; Sami
> Mujawar <sami.mujawar@arm.com>; Taylor Beebe <t@taylorbeebe.com>
> Subject: Re: [edk2-devel] [RFC PATCH 2/3] MdeModulePkg: Enable forward edge
> CFI in mem attributes table
> 
> Ard, I am still actively tracking this for the PE/COFF spec.
> 
> Unfortunately, I don't have more firm info right now but I suggest
> holding off on alternatives for the time being and I will reply back as
> soon as the next steps are known.
> 
> Thanks,
> Michael
> 
> On 2/2/2023 2:00 PM, Ard Biesheuvel wrote:
> > On Thu, 2 Feb 2023 at 19:49, Kinney, Michael D
> > <michael.d.kinney@intel.com> wrote:
> >>
> >> Hi Ard,
> >>
> >> Since the PE/COFF image does not contain this information, is there an
> option
> >> to add the information to an FFS file. Either as a new bit in a standard header
> >> or as a GUIDed section defined by EDK II?
> >>
> >> Since an FV may contain content build from source and additional content
> >> Integrated as binaries from other vendors, having a PCD that scopes this
> >> attribute to all FVs many only work for FW builds that are 100% from sources.
> >>
> >
> > I didn't quite consider that scenario tbh.
> >
> > It would be nice if we could avoid another PI spec dance for this, and
> > get some kind of IBT/BTI annotation added to the PE/COFF spec. That
> > way, we cover this issue, as well as clear the way for the use if
> > IBT/BTI when running under the firmware.
> >
> > Are TE images a concern here as well? I.e., are those likely to be
> > used for runtime DXE images in firmware volumes?
> >
> >
> >>
> >>> -----Original Message-----
> >>> From: Ard Biesheuvel <ardb@kernel.org>
> >>> Sent: Thursday, February 2, 2023 10:04 AM
> >>> To: devel@edk2.groups.io
> >>> Cc: Ard Biesheuvel <ardb@kernel.org>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Yao,
> >>> Jiewen <jiewen.yao@intel.com>; Kubacki, Michael
> <michael.kubacki@microsoft.com>; Sean Brogan
> <sean.brogan@microsoft.com>; Rebecca
> >>> Cran <quic_rcran@quicinc.com>; Leif Lindholm
> <quic_llindhol@quicinc.com>; Sami Mujawar <sami.mujawar@arm.com>;
> Taylor Beebe
> >>> <t@taylorbeebe.com>
> >>> Subject: [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem
> attributes table
> >>>
> >>> The memory attributes table has been extended with a flag that indicates
> >>> whether or not the OS is permitted to map the EFI runtime code regions
> >>> with strict enforcement for IBT/BTI landing pad instructions.
> >>>
> >>> This is generally a property of the firmware build, and so we can permit
> >>> a platform to set this property using a PCD, and put the burden on the
> >>> platform definition to set the toolchain options accordingly.
> >>>
> >>> There is one snag, however: PE/COFF does not expose whether or not the
> >>> code was generated with landing pads, so if any runtime DXE drivers were
> >>> loaded from storage other than the firmware volumes, we must assume
> that
> >>> setting the CFI flag in the memory attributes table is unsafe.
> >>>
> >>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >>> ---
> >>>   MdeModulePkg/Core/Dxe/DxeMain.h                    |  2 ++
> >>>   MdeModulePkg/Core/Dxe/DxeMain.inf                  |  1 +
> >>>   MdeModulePkg/Core/Dxe/Image/Image.c                | 11 +++++++++++
> >>>   MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c |  7 ++++++-
> >>>   MdeModulePkg/MdeModulePkg.dec                      |  8 ++++++++
> >>>   5 files changed, 28 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h
> b/MdeModulePkg/Core/Dxe/DxeMain.h
> >>> index 815a6b4bd844..427a5fc78f72 100644
> >>> --- a/MdeModulePkg/Core/Dxe/DxeMain.h
> >>> +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
> >>> @@ -280,6 +280,8 @@ extern EFI_MEMORY_TYPE_INFORMATION
> gMemoryTypeInformation[EfiMaxMemoryType + 1]
> >>>   extern BOOLEAN                    gDispatcherRunning;
> >>>
> >>>   extern EFI_RUNTIME_ARCH_PROTOCOL  gRuntimeTemplate;
> >>>
> >>>
> >>>
> >>> +extern BOOLEAN                    gMemoryAttributesTableForwardCfi;
> >>>
> >>> +
> >>>
> >>>   extern EFI_LOAD_FIXED_ADDRESS_CONFIGURATION_TABLE
> gLoadModuleAtFixAddressConfigurationTable;
> >>>
> >>>   extern BOOLEAN
> gLoadFixedAddressCodeMemoryReady;
> >>>
> >>>   //
> >>>
> >>> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf
> b/MdeModulePkg/Core/Dxe/DxeMain.inf
> >>> index 35d5bf0dee6f..e6ff67773a69 100644
> >>> --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> >>> +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> >>> @@ -187,6 +187,7 @@ [Pcd]
> >>>     gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask
> ## CONSUMES
> >>>
> >>>     gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                           ##
> CONSUMES
> >>>
> >>>
> gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth
> ## CONSUMES
> >>>
> >>> +
> gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi
> ## CONSUMES
> >>>
> >>>
> >>>
> >>>   # [Hob]
> >>>
> >>>   # RESOURCE_DESCRIPTOR   ## CONSUMES
> >>>
> >>> diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c
> b/MdeModulePkg/Core/Dxe/Image/Image.c
> >>> index 06cc6744b8c6..181fefdb6657 100644
> >>> --- a/MdeModulePkg/Core/Dxe/Image/Image.c
> >>> +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
> >>> @@ -1398,6 +1398,17 @@ CoreLoadImageCommon (
> >>>       CoreNewDebugImageInfoEntry
> (EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL, &Image->Info, Image->Handle);
> >>>
> >>>     }
> >>>
> >>>
> >>>
> >>> +  //
> >>>
> >>> +  // If we loaded a runtime DXE driver from something other than a FV, it
> >>>
> >>> +  // was not built as part of the firmware image, and so we cannot assume
> >>>
> >>> +  // that it was built with IBT/BTI landing pads for forward edge control
> >>>
> >>> +  // flow integrity.
> >>>
> >>> +  //
> >>>
> >>> +  if (!ImageIsFromFv &&
> >>>
> >>> +      (Image->ImageContext.ImageCodeMemoryType ==
> EfiRuntimeServicesCode)) {
> >>>
> >>> +    gMemoryAttributesTableForwardCfi = FALSE;
> >>>
> >>> +  }
> >>>
> >>> +
> >>>
> >>>     //
> >>>
> >>>     // Reinstall loaded image protocol to fire any notifications
> >>>
> >>>     //
> >>>
> >>> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
> b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
> >>> index e07921371187..cdd35ade0a8a 100644
> >>> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
> >>> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
> >>> @@ -89,6 +89,7 @@ BOOLEAN                      mMemoryAttributesTableEnable
> = TRUE;
> >>>   BOOLEAN                      mMemoryAttributesTableEndOfDxe    = FALSE;
> >>>
> >>>   EFI_MEMORY_ATTRIBUTES_TABLE  *mMemoryAttributesTable           =
> NULL;
> >>>
> >>>   BOOLEAN                      mMemoryAttributesTableReadyToBoot = FALSE;
> >>>
> >>> +BOOLEAN                      gMemoryAttributesTableForwardCfi  =
> FixedPcdGetBool (PcdMemoryAttributesTableForwardCfi);
> >>>
> >>>
> >>>
> >>>   /**
> >>>
> >>>     Install MemoryAttributesTable.
> >>>
> >>> @@ -182,7 +183,11 @@ InstallMemoryAttributesTable (
> >>>     MemoryAttributesTable->Version         =
> EFI_MEMORY_ATTRIBUTES_TABLE_VERSION;
> >>>
> >>>     MemoryAttributesTable->NumberOfEntries = RuntimeEntryCount;
> >>>
> >>>     MemoryAttributesTable->DescriptorSize  = (UINT32)DescriptorSize;
> >>>
> >>> -  MemoryAttributesTable->Reserved        = 0;
> >>>
> >>> +  if (gMemoryAttributesTableForwardCfi) {
> >>>
> >>> +    MemoryAttributesTable->Flags         =
> EFI_MEMORY_ATTRIBUTES_FLAGS_RT_FORWARD_CONTROL_FLOW_GUARD;
> >>>
> >>> +  } else {
> >>>
> >>> +    MemoryAttributesTable->Flags         = 0;
> >>>
> >>> +  }
> >>>
> >>>     DEBUG ((DEBUG_VERBOSE, "MemoryAttributesTable:\n"));
> >>>
> >>>     DEBUG ((DEBUG_VERBOSE, "  Version              - 0x%08x\n",
> MemoryAttributesTable->Version));
> >>>
> >>>     DEBUG ((DEBUG_VERBOSE, "  NumberOfEntries      - 0x%08x\n",
> MemoryAttributesTable->NumberOfEntries));
> >>>
> >>> diff --git a/MdeModulePkg/MdeModulePkg.dec
> b/MdeModulePkg/MdeModulePkg.dec
> >>> index 9605c617b7a8..d336a38655a3 100644
> >>> --- a/MdeModulePkg/MdeModulePkg.dec
> >>> +++ b/MdeModulePkg/MdeModulePkg.dec
> >>> @@ -1093,6 +1093,14 @@ [PcdsFixedAtBuild]
> >>>     # @Prompt Enable UEFI Stack Guard.
> >>>
> >>>
> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0x30
> 001055
> >>>
> >>>
> >>>
> >>> +  ## Indicates whether the EFI memory attributes table will inform the OS
> that
> >>>
> >>> +  #  forward edge control flow guards have been inserted into the runtime
> services
> >>>
> >>> +  #  code regions.
> >>>
> >>> +  #   TRUE  - Runtime code has forward control flow guards.<BR>
> >>>
> >>> +  #   FALSE - Runtime code does not have forward control flow guards.<BR>
> >>>
> >>> +  # @Prompt Enable forward control flow guards in EFI memory attributes
> table
> >>>
> >>> +
> gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi|FAL
> SE|BOOLEAN|0x30001056
> >>>
> >>> +
> >>>
> >>>   [PcdsFixedAtBuild, PcdsPatchableInModule]
> >>>
> >>>     ## Dynamic type PCD can be registered callback function for Pcd setting
> action.
> >>>
> >>>     #  PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum
> number of callback function
> >>>
> >>> --
> >>> 2.39.1
> >>
> >
> >
> > 
> >

Re: [edk2-devel] [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem attributes table

[Ard Biesheuvel]

(cc Samer, Jose)

On Fri, 3 Feb 2023 at 02:16, Yao, Jiewen <jiewen.yao@intel.com> wrote:
>
> Hello
> Can we assume that the entrypoint of PE/COFF image is always ENDBR64, if the PE/COFF image is enlightened to support IBT?
>
> I believe the compiler should do that, because the loader need use indirect call to the PE/COFF entrypoint.
>

That is an interesting idea. Even if we have some PE level annotation
for BTI/IBT at some point, it would be a good sanity check on the
image, because if the entry point does not have the guard instruction,
there is obviously something wrong.

However, on arm64, it is a bit more ambiguous, because there are
instructions for pointer authentication (PACIASP) that may be
interpreted as implicit guard instructions too, so the presence of
such an instruction at the entry point does not imply that BTI is
enabled in the entire image.

I'll code up a PoC with this, but us ARM folks should have a think
about how to spec this out and deploy this. I wonder

In the mean time, Michael is trying to round up the MS folks that are
involved in maintaining the PE/COFF spec, and ideally, we'd have some
image level flag that informs the loader whether all code sections in
the image were emitted with guard instructions for BTI/IBT.



> We need more code to detect *all* runtime images. The logic could be:
> 1) PE/COFF loader (X86, ARM) detects IBT capability for all runtime images, and report it.
> 2) DXE Core collects such info.
> 2.1) If ALL runtime image supports IBT, then gMemoryAttributesTableForwardCfi = TRUE
> 2.2) Else, gMemoryAttributesTableForwardCfi = FALSE.
>
> The benefit is that it is more reliable to support binary PE image use case. Hard to use build time flag for external binary PE ...
>

Agreed. This sounds like a good way to implement it.


> > -----Original Message-----
> > From: Michael Kubacki <mikuback@linux.microsoft.com>
> > Sent: Friday, February 3, 2023 8:24 AM
> > To: devel@edk2.groups.io; ardb@kernel.org; Kinney, Michael D
> > <michael.d.kinney@intel.com>
> > Cc: Gao, Liming <gaoliming@byosoft.com.cn>; Yao, Jiewen
> > <jiewen.yao@intel.com>; Kubacki, Michael <michael.kubacki@microsoft.com>;
> > Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran
> > <quic_rcran@quicinc.com>; Leif Lindholm <quic_llindhol@quicinc.com>; Sami
> > Mujawar <sami.mujawar@arm.com>; Taylor Beebe <t@taylorbeebe.com>
> > Subject: Re: [edk2-devel] [RFC PATCH 2/3] MdeModulePkg: Enable forward edge
> > CFI in mem attributes table
> >
> > Ard, I am still actively tracking this for the PE/COFF spec.
> >
> > Unfortunately, I don't have more firm info right now but I suggest
> > holding off on alternatives for the time being and I will reply back as
> > soon as the next steps are known.
> >
> > Thanks,
> > Michael
> >
> > On 2/2/2023 2:00 PM, Ard Biesheuvel wrote:
> > > On Thu, 2 Feb 2023 at 19:49, Kinney, Michael D
> > > <michael.d.kinney@intel.com> wrote:
> > >>
> > >> Hi Ard,
> > >>
> > >> Since the PE/COFF image does not contain this information, is there an
> > option
> > >> to add the information to an FFS file. Either as a new bit in a standard header
> > >> or as a GUIDed section defined by EDK II?
> > >>
> > >> Since an FV may contain content build from source and additional content
> > >> Integrated as binaries from other vendors, having a PCD that scopes this
> > >> attribute to all FVs many only work for FW builds that are 100% from sources.
> > >>
> > >
> > > I didn't quite consider that scenario tbh.
> > >
> > > It would be nice if we could avoid another PI spec dance for this, and
> > > get some kind of IBT/BTI annotation added to the PE/COFF spec. That
> > > way, we cover this issue, as well as clear the way for the use if
> > > IBT/BTI when running under the firmware.
> > >
> > > Are TE images a concern here as well? I.e., are those likely to be
> > > used for runtime DXE images in firmware volumes?
> > >
> > >
> > >>
> > >>> -----Original Message-----
> > >>> From: Ard Biesheuvel <ardb@kernel.org>
> > >>> Sent: Thursday, February 2, 2023 10:04 AM
> > >>> To: devel@edk2.groups.io
> > >>> Cc: Ard Biesheuvel <ardb@kernel.org>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Yao,
> > >>> Jiewen <jiewen.yao@intel.com>; Kubacki, Michael
> > <michael.kubacki@microsoft.com>; Sean Brogan
> > <sean.brogan@microsoft.com>; Rebecca
> > >>> Cran <quic_rcran@quicinc.com>; Leif Lindholm
> > <quic_llindhol@quicinc.com>; Sami Mujawar <sami.mujawar@arm.com>;
> > Taylor Beebe
> > >>> <t@taylorbeebe.com>
> > >>> Subject: [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem
> > attributes table
> > >>>
> > >>> The memory attributes table has been extended with a flag that indicates
> > >>> whether or not the OS is permitted to map the EFI runtime code regions
> > >>> with strict enforcement for IBT/BTI landing pad instructions.
> > >>>
> > >>> This is generally a property of the firmware build, and so we can permit
> > >>> a platform to set this property using a PCD, and put the burden on the
> > >>> platform definition to set the toolchain options accordingly.
> > >>>
> > >>> There is one snag, however: PE/COFF does not expose whether or not the
> > >>> code was generated with landing pads, so if any runtime DXE drivers were
> > >>> loaded from storage other than the firmware volumes, we must assume
> > that
> > >>> setting the CFI flag in the memory attributes table is unsafe.
> > >>>
> > >>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > >>> ---
> > >>>   MdeModulePkg/Core/Dxe/DxeMain.h                    |  2 ++
> > >>>   MdeModulePkg/Core/Dxe/DxeMain.inf                  |  1 +
> > >>>   MdeModulePkg/Core/Dxe/Image/Image.c                | 11 +++++++++++
> > >>>   MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c |  7 ++++++-
> > >>>   MdeModulePkg/MdeModulePkg.dec                      |  8 ++++++++
> > >>>   5 files changed, 28 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h
> > b/MdeModulePkg/Core/Dxe/DxeMain.h
> > >>> index 815a6b4bd844..427a5fc78f72 100644
> > >>> --- a/MdeModulePkg/Core/Dxe/DxeMain.h
> > >>> +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
> > >>> @@ -280,6 +280,8 @@ extern EFI_MEMORY_TYPE_INFORMATION
> > gMemoryTypeInformation[EfiMaxMemoryType + 1]
> > >>>   extern BOOLEAN                    gDispatcherRunning;
> > >>>
> > >>>   extern EFI_RUNTIME_ARCH_PROTOCOL  gRuntimeTemplate;
> > >>>
> > >>>
> > >>>
> > >>> +extern BOOLEAN                    gMemoryAttributesTableForwardCfi;
> > >>>
> > >>> +
> > >>>
> > >>>   extern EFI_LOAD_FIXED_ADDRESS_CONFIGURATION_TABLE
> > gLoadModuleAtFixAddressConfigurationTable;
> > >>>
> > >>>   extern BOOLEAN
> > gLoadFixedAddressCodeMemoryReady;
> > >>>
> > >>>   //
> > >>>
> > >>> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf
> > b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > >>> index 35d5bf0dee6f..e6ff67773a69 100644
> > >>> --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> > >>> +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > >>> @@ -187,6 +187,7 @@ [Pcd]
> > >>>     gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask
> > ## CONSUMES
> > >>>
> > >>>     gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                           ##
> > CONSUMES
> > >>>
> > >>>
> > gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth
> > ## CONSUMES
> > >>>
> > >>> +
> > gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi
> > ## CONSUMES
> > >>>
> > >>>
> > >>>
> > >>>   # [Hob]
> > >>>
> > >>>   # RESOURCE_DESCRIPTOR   ## CONSUMES
> > >>>
> > >>> diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c
> > b/MdeModulePkg/Core/Dxe/Image/Image.c
> > >>> index 06cc6744b8c6..181fefdb6657 100644
> > >>> --- a/MdeModulePkg/Core/Dxe/Image/Image.c
> > >>> +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
> > >>> @@ -1398,6 +1398,17 @@ CoreLoadImageCommon (
> > >>>       CoreNewDebugImageInfoEntry
> > (EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL, &Image->Info, Image->Handle);
> > >>>
> > >>>     }
> > >>>
> > >>>
> > >>>
> > >>> +  //
> > >>>
> > >>> +  // If we loaded a runtime DXE driver from something other than a FV, it
> > >>>
> > >>> +  // was not built as part of the firmware image, and so we cannot assume
> > >>>
> > >>> +  // that it was built with IBT/BTI landing pads for forward edge control
> > >>>
> > >>> +  // flow integrity.
> > >>>
> > >>> +  //
> > >>>
> > >>> +  if (!ImageIsFromFv &&
> > >>>
> > >>> +      (Image->ImageContext.ImageCodeMemoryType ==
> > EfiRuntimeServicesCode)) {
> > >>>
> > >>> +    gMemoryAttributesTableForwardCfi = FALSE;
> > >>>
> > >>> +  }
> > >>>
> > >>> +
> > >>>
> > >>>     //
> > >>>
> > >>>     // Reinstall loaded image protocol to fire any notifications
> > >>>
> > >>>     //
> > >>>
> > >>> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
> > b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
> > >>> index e07921371187..cdd35ade0a8a 100644
> > >>> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
> > >>> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
> > >>> @@ -89,6 +89,7 @@ BOOLEAN                      mMemoryAttributesTableEnable
> > = TRUE;
> > >>>   BOOLEAN                      mMemoryAttributesTableEndOfDxe    = FALSE;
> > >>>
> > >>>   EFI_MEMORY_ATTRIBUTES_TABLE  *mMemoryAttributesTable           =
> > NULL;
> > >>>
> > >>>   BOOLEAN                      mMemoryAttributesTableReadyToBoot = FALSE;
> > >>>
> > >>> +BOOLEAN                      gMemoryAttributesTableForwardCfi  =
> > FixedPcdGetBool (PcdMemoryAttributesTableForwardCfi);
> > >>>
> > >>>
> > >>>
> > >>>   /**
> > >>>
> > >>>     Install MemoryAttributesTable.
> > >>>
> > >>> @@ -182,7 +183,11 @@ InstallMemoryAttributesTable (
> > >>>     MemoryAttributesTable->Version         =
> > EFI_MEMORY_ATTRIBUTES_TABLE_VERSION;
> > >>>
> > >>>     MemoryAttributesTable->NumberOfEntries = RuntimeEntryCount;
> > >>>
> > >>>     MemoryAttributesTable->DescriptorSize  = (UINT32)DescriptorSize;
> > >>>
> > >>> -  MemoryAttributesTable->Reserved        = 0;
> > >>>
> > >>> +  if (gMemoryAttributesTableForwardCfi) {
> > >>>
> > >>> +    MemoryAttributesTable->Flags         =
> > EFI_MEMORY_ATTRIBUTES_FLAGS_RT_FORWARD_CONTROL_FLOW_GUARD;
> > >>>
> > >>> +  } else {
> > >>>
> > >>> +    MemoryAttributesTable->Flags         = 0;
> > >>>
> > >>> +  }
> > >>>
> > >>>     DEBUG ((DEBUG_VERBOSE, "MemoryAttributesTable:\n"));
> > >>>
> > >>>     DEBUG ((DEBUG_VERBOSE, "  Version              - 0x%08x\n",
> > MemoryAttributesTable->Version));
> > >>>
> > >>>     DEBUG ((DEBUG_VERBOSE, "  NumberOfEntries      - 0x%08x\n",
> > MemoryAttributesTable->NumberOfEntries));
> > >>>
> > >>> diff --git a/MdeModulePkg/MdeModulePkg.dec
> > b/MdeModulePkg/MdeModulePkg.dec
> > >>> index 9605c617b7a8..d336a38655a3 100644
> > >>> --- a/MdeModulePkg/MdeModulePkg.dec
> > >>> +++ b/MdeModulePkg/MdeModulePkg.dec
> > >>> @@ -1093,6 +1093,14 @@ [PcdsFixedAtBuild]
> > >>>     # @Prompt Enable UEFI Stack Guard.
> > >>>
> > >>>
> > gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0x30
> > 001055
> > >>>
> > >>>
> > >>>
> > >>> +  ## Indicates whether the EFI memory attributes table will inform the OS
> > that
> > >>>
> > >>> +  #  forward edge control flow guards have been inserted into the runtime
> > services
> > >>>
> > >>> +  #  code regions.
> > >>>
> > >>> +  #   TRUE  - Runtime code has forward control flow guards.<BR>
> > >>>
> > >>> +  #   FALSE - Runtime code does not have forward control flow guards.<BR>
> > >>>
> > >>> +  # @Prompt Enable forward control flow guards in EFI memory attributes
> > table
> > >>>
> > >>> +
> > gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi|FAL
> > SE|BOOLEAN|0x30001056
> > >>>
> > >>> +
> > >>>
> > >>>   [PcdsFixedAtBuild, PcdsPatchableInModule]
> > >>>
> > >>>     ## Dynamic type PCD can be registered callback function for Pcd setting
> > action.
> > >>>
> > >>>     #  PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum
> > number of callback function
> > >>>
> > >>> --
> > >>> 2.39.1
> > >>
> > >
> > >
> > > 
> > >

Re: [edk2-devel] [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem attributes table

[Marvin Häuser]

[-- Attachment #1: Type: text/plain, Size: 14454 bytes --]

Hi Ard and Jiewen,

(I‘m replying from groups.io and cannot figure out how to CC Jiewen. Ugh.)

Personally, I‘d rather have UEFI itself rely solely on the flag in the image file. If there is a way needed to handle images without the tag, in my opinion use some userland preprocessing tool to check and annotate it when generating the firmware image. Even if it‘s trivial for Intel, I hope we won‘t end up with a micro-disassembler in firmware just to tell whether a feature is enabled.

Best regards,
Marvin

On Fri, Feb 3, 2023 at 12:26 AM, Ard Biesheuvel wrote:

> 
> (cc Samer, Jose)
> 
> On Fri, 3 Feb 2023 at 02:16, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> 
>> 
>> Hello
>> Can we assume that the entrypoint of PE/COFF image is always ENDBR64, if
>> the PE/COFF image is enlightened to support IBT?
>> 
>> I believe the compiler should do that, because the loader need use
>> indirect call to the PE/COFF entrypoint.
> 
> That is an interesting idea. Even if we have some PE level annotation
> for BTI/IBT at some point, it would be a good sanity check on the
> image, because if the entry point does not have the guard instruction,
> there is obviously something wrong.
> 
> However, on arm64, it is a bit more ambiguous, because there are
> instructions for pointer authentication (PACIASP) that may be
> interpreted as implicit guard instructions too, so the presence of
> such an instruction at the entry point does not imply that BTI is
> enabled in the entire image.
> 
> I'll code up a PoC with this, but us ARM folks should have a think
> about how to spec this out and deploy this. I wonder
> 
> In the mean time, Michael is trying to round up the MS folks that are
> involved in maintaining the PE/COFF spec, and ideally, we'd have some
> image level flag that informs the loader whether all code sections in
> the image were emitted with guard instructions for BTI/IBT.
> 
> 
> 
> 
>> We need more code to detect *all* runtime images. The logic could be:
>> 1) PE/COFF loader (X86, ARM) detects IBT capability for all runtime
>> images, and report it.
>> 2) DXE Core collects such info.
>> 2.1) If ALL runtime image supports IBT, then
>> gMemoryAttributesTableForwardCfi = TRUE
>> 2.2) Else, gMemoryAttributesTableForwardCfi = FALSE.
>> 
>> The benefit is that it is more reliable to support binary PE image use
>> case. Hard to use build time flag for external binary PE ...
> 
> Agreed. This sounds like a good way to implement it.
> 
> 
> 
>> 
>>> -----Original Message-----
>>> From: Michael Kubacki <mikuback@linux.microsoft.com>
>>> Sent: Friday, February 3, 2023 8:24 AM
>>> To: devel@edk2.groups.io; ardb@kernel.org; Kinney, Michael D
>>> <michael.d.kinney@intel.com>
>>> Cc: Gao, Liming <gaoliming@byosoft.com.cn>; Yao, Jiewen
>>> <jiewen.yao@intel.com>; Kubacki, Michael <michael.kubacki@microsoft.com>;
>>> Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran
>>> <quic_rcran@quicinc.com>; Leif Lindholm <quic_llindhol@quicinc.com>; Sami
>>> Mujawar <sami.mujawar@arm.com>; Taylor Beebe <t@taylorbeebe.com>
>>> Subject: Re: [edk2-devel] [RFC PATCH 2/3] MdeModulePkg: Enable forward
>>> edge
>>> CFI in mem attributes table
>>> 
>>> Ard, I am still actively tracking this for the PE/COFF spec.
>>> 
>>> Unfortunately, I don't have more firm info right now but I suggest
>>> holding off on alternatives for the time being and I will reply back as
>>> soon as the next steps are known.
>>> 
>>> Thanks,
>>> Michael
>>> 
>>> On 2/2/2023 2:00 PM, Ard Biesheuvel wrote:
>>> 
>>>> On Thu, 2 Feb 2023 at 19:49, Kinney, Michael D
>>>> <michael.d.kinney@intel.com> wrote:
>>>> 
>>>>> 
>>>>> Hi Ard,
>>>>> 
>>>>> Since the PE/COFF image does not contain this information, is there an
>>>> 
>>>> 
>>> 
>>> option
>>> 
>>>> 
>>>>> to add the information to an FFS file. Either as a new bit in a standard
>>>>> header
>>>>> or as a GUIDed section defined by EDK II?
>>>>> 
>>>>> Since an FV may contain content build from source and additional content
>>>>> Integrated as binaries from other vendors, having a PCD that scopes this
>>>>> attribute to all FVs many only work for FW builds that are 100% from
>>>>> sources.
>>>> 
>>>> I didn't quite consider that scenario tbh.
>>>> 
>>>> It would be nice if we could avoid another PI spec dance for this, and
>>>> get some kind of IBT/BTI annotation added to the PE/COFF spec. That
>>>> way, we cover this issue, as well as clear the way for the use if
>>>> IBT/BTI when running under the firmware.
>>>> 
>>>> Are TE images a concern here as well? I.e., are those likely to be
>>>> used for runtime DXE images in firmware volumes?
>>>> 
>>>> 
>>>> 
>>>>> 
>>>>> 
>>>>>> -----Original Message-----
>>>>>> From: Ard Biesheuvel <ardb@kernel.org>
>>>>>> Sent: Thursday, February 2, 2023 10:04 AM
>>>>>> To: devel@edk2.groups.io
>>>>>> Cc: Ard Biesheuvel <ardb@kernel.org>; Kinney, Michael D
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Yao,
>>> 
>>> 
>>>> 
>>>>> 
>>>>>> Jiewen <jiewen.yao@intel.com>; Kubacki, Michael
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> <michael.kubacki@microsoft.com>; Sean Brogan
>>> <sean.brogan@microsoft.com>; Rebecca
>>> 
>>>> 
>>>>> 
>>>>>> Cran <quic_rcran@quicinc.com>; Leif Lindholm
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> <quic_llindhol@quicinc.com>; Sami Mujawar <sami.mujawar@arm.com>;
>>> Taylor Beebe
>>> 
>>>> 
>>>>> 
>>>>>> <t@taylorbeebe.com>
>>>>>> Subject: [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> attributes table
>>> 
>>>> 
>>>>> 
>>>>>> 
>>>>>> The memory attributes table has been extended with a flag that indicates
>>>>>> whether or not the OS is permitted to map the EFI runtime code regions
>>>>>> with strict enforcement for IBT/BTI landing pad instructions.
>>>>>> 
>>>>>> This is generally a property of the firmware build, and so we can permit
>>>>>> a platform to set this property using a PCD, and put the burden on the
>>>>>> platform definition to set the toolchain options accordingly.
>>>>>> 
>>>>>> There is one snag, however: PE/COFF does not expose whether or not the
>>>>>> code was generated with landing pads, so if any runtime DXE drivers were
>>>>>> loaded from storage other than the firmware volumes, we must assume
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> that
>>> 
>>>> 
>>>>> 
>>>>>> setting the CFI flag in the memory attributes table is unsafe.
>>>>>> 
>>>>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>>>>> ---
>>>>>> MdeModulePkg/Core/Dxe/DxeMain.h | 2 ++
>>>>>> MdeModulePkg/Core/Dxe/DxeMain.inf | 1 +
>>>>>> MdeModulePkg/Core/Dxe/Image/Image.c | 11 +++++++++++
>>>>>> MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c | 7 ++++++-
>>>>>> MdeModulePkg/MdeModulePkg.dec | 8 ++++++++
>>>>>> 5 files changed, 28 insertions(+), 1 deletion(-)
>>>>>> 
>>>>>> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> b/MdeModulePkg/Core/Dxe/DxeMain.h
>>> 
>>>> 
>>>>> 
>>>>>> index 815a6b4bd844..427a5fc78f72 100644
>>>>>> --- a/MdeModulePkg/Core/Dxe/DxeMain.h
>>>>>> +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
>>>>>> @@ -280,6 +280,8 @@ extern EFI_MEMORY_TYPE_INFORMATION
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> gMemoryTypeInformation[EfiMaxMemoryType + 1]
>>> 
>>>> 
>>>>> 
>>>>>> extern BOOLEAN gDispatcherRunning;
>>>>>> 
>>>>>> extern EFI_RUNTIME_ARCH_PROTOCOL gRuntimeTemplate;
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> +extern BOOLEAN gMemoryAttributesTableForwardCfi;
>>>>>> 
>>>>>> +
>>>>>> 
>>>>>> extern EFI_LOAD_FIXED_ADDRESS_CONFIGURATION_TABLE
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> gLoadModuleAtFixAddressConfigurationTable;
>>> 
>>>> 
>>>>> 
>>>>>> 
>>>>>> extern BOOLEAN
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> gLoadFixedAddressCodeMemoryReady;
>>> 
>>>> 
>>>>> 
>>>>>> 
>>>>>> //
>>>>>> 
>>>>>> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> b/MdeModulePkg/Core/Dxe/DxeMain.inf
>>> 
>>>> 
>>>>> 
>>>>>> index 35d5bf0dee6f..e6ff67773a69 100644
>>>>>> --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
>>>>>> +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
>>>>>> @@ -187,6 +187,7 @@ [Pcd]
>>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> ## CONSUMES
>>> 
>>>> 
>>>>> 
>>>>>> 
>>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ##
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> CONSUMES
>>> 
>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth
>>> ## CONSUMES
>>> 
>>>> 
>>>>> 
>>>>>> 
>>>>>> +
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi
>>> ## CONSUMES
>>> 
>>>> 
>>>>> 
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> # [Hob]
>>>>>> 
>>>>>> # RESOURCE_DESCRIPTOR ## CONSUMES
>>>>>> 
>>>>>> diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> b/MdeModulePkg/Core/Dxe/Image/Image.c
>>> 
>>>> 
>>>>> 
>>>>>> index 06cc6744b8c6..181fefdb6657 100644
>>>>>> --- a/MdeModulePkg/Core/Dxe/Image/Image.c
>>>>>> +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
>>>>>> @@ -1398,6 +1398,17 @@ CoreLoadImageCommon (
>>>>>> CoreNewDebugImageInfoEntry
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> (EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL, &Image->Info, Image->Handle);
>>> 
>>>> 
>>>>> 
>>>>>> 
>>>>>> }
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> + //
>>>>>> 
>>>>>> + // If we loaded a runtime DXE driver from something other than a FV, it
>>>>>> 
>>>>>> + // was not built as part of the firmware image, and so we cannot assume
>>>>>> 
>>>>>> + // that it was built with IBT/BTI landing pads for forward edge control
>>>>>> 
>>>>>> + // flow integrity.
>>>>>> 
>>>>>> + //
>>>>>> 
>>>>>> + if (!ImageIsFromFv &&
>>>>>> 
>>>>>> + (Image->ImageContext.ImageCodeMemoryType ==
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> EfiRuntimeServicesCode)) {
>>> 
>>>> 
>>>>> 
>>>>>> 
>>>>>> + gMemoryAttributesTableForwardCfi = FALSE;
>>>>>> 
>>>>>> + }
>>>>>> 
>>>>>> +
>>>>>> 
>>>>>> //
>>>>>> 
>>>>>> // Reinstall loaded image protocol to fire any notifications
>>>>>> 
>>>>>> //
>>>>>> 
>>>>>> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
>>> 
>>>> 
>>>>> 
>>>>>> index e07921371187..cdd35ade0a8a 100644
>>>>>> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
>>>>>> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
>>>>>> @@ -89,6 +89,7 @@ BOOLEAN mMemoryAttributesTableEnable
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> = TRUE;
>>> 
>>>> 
>>>>> 
>>>>>> BOOLEAN mMemoryAttributesTableEndOfDxe = FALSE;
>>>>>> 
>>>>>> EFI_MEMORY_ATTRIBUTES_TABLE *mMemoryAttributesTable =
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> NULL;
>>> 
>>>> 
>>>>> 
>>>>>> 
>>>>>> BOOLEAN mMemoryAttributesTableReadyToBoot = FALSE;
>>>>>> 
>>>>>> +BOOLEAN gMemoryAttributesTableForwardCfi =
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> FixedPcdGetBool (PcdMemoryAttributesTableForwardCfi);
>>> 
>>>> 
>>>>> 
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> /**
>>>>>> 
>>>>>> Install MemoryAttributesTable.
>>>>>> 
>>>>>> @@ -182,7 +183,11 @@ InstallMemoryAttributesTable (
>>>>>> MemoryAttributesTable->Version =
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> EFI_MEMORY_ATTRIBUTES_TABLE_VERSION;
>>> 
>>>> 
>>>>> 
>>>>>> 
>>>>>> MemoryAttributesTable->NumberOfEntries = RuntimeEntryCount;
>>>>>> 
>>>>>> MemoryAttributesTable->DescriptorSize = (UINT32)DescriptorSize;
>>>>>> 
>>>>>> - MemoryAttributesTable->Reserved = 0;
>>>>>> 
>>>>>> + if (gMemoryAttributesTableForwardCfi) {
>>>>>> 
>>>>>> + MemoryAttributesTable->Flags =
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> EFI_MEMORY_ATTRIBUTES_FLAGS_RT_FORWARD_CONTROL_FLOW_GUARD;
>>> 
>>>> 
>>>>> 
>>>>>> 
>>>>>> + } else {
>>>>>> 
>>>>>> + MemoryAttributesTable->Flags = 0;
>>>>>> 
>>>>>> + }
>>>>>> 
>>>>>> DEBUG ((DEBUG_VERBOSE, "MemoryAttributesTable:\n"));
>>>>>> 
>>>>>> DEBUG ((DEBUG_VERBOSE, " Version - 0x%08x\n",
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> MemoryAttributesTable->Version));
>>> 
>>>> 
>>>>> 
>>>>>> 
>>>>>> DEBUG ((DEBUG_VERBOSE, " NumberOfEntries - 0x%08x\n",
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> MemoryAttributesTable->NumberOfEntries));
>>> 
>>>> 
>>>>> 
>>>>>> 
>>>>>> diff --git a/MdeModulePkg/MdeModulePkg.dec
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> b/MdeModulePkg/MdeModulePkg.dec
>>> 
>>>> 
>>>>> 
>>>>>> index 9605c617b7a8..d336a38655a3 100644
>>>>>> --- a/MdeModulePkg/MdeModulePkg.dec
>>>>>> +++ b/MdeModulePkg/MdeModulePkg.dec
>>>>>> @@ -1093,6 +1093,14 @@ [PcdsFixedAtBuild]
>>>>>> # @Prompt Enable UEFI Stack Guard.
>>>>>> 
>>>>>> 
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0x30
>>> 001055
>>> 
>>>> 
>>>>> 
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> + ## Indicates whether the EFI memory attributes table will inform the OS
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> that
>>> 
>>>> 
>>>>> 
>>>>>> 
>>>>>> + # forward edge control flow guards have been inserted into the runtime
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> services
>>> 
>>>> 
>>>>> 
>>>>>> 
>>>>>> + # code regions.
>>>>>> 
>>>>>> + # TRUE - Runtime code has forward control flow guards.<BR>
>>>>>> 
>>>>>> + # FALSE - Runtime code does not have forward control flow guards.<BR>
>>>>>> 
>>>>>> + # @Prompt Enable forward control flow guards in EFI memory attributes
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> table
>>> 
>>>> 
>>>>> 
>>>>>> 
>>>>>> +
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi|FAL
>>> SE|BOOLEAN|0x30001056
>>> 
>>>> 
>>>>> 
>>>>>> 
>>>>>> +
>>>>>> 
>>>>>> [PcdsFixedAtBuild, PcdsPatchableInModule]
>>>>>> 
>>>>>> ## Dynamic type PCD can be registered callback function for Pcd setting
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> action.
>>> 
>>>> 
>>>>> 
>>>>>> 
>>>>>> # PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> number of callback function
>>> 
>>>> 
>>>>> 
>>>>>> 
>>>>>> --
>>>>>> 2.39.1
>>>>> 
>>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>> 
>>> 
>> 
>> 
> 
>

[-- Attachment #2: Type: text/html, Size: 15051 bytes --]

Re: [edk2-devel] [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem attributes table

[Yao, Jiewen]

[-- Attachment #1: Type: text/plain, Size: 12135 bytes --]

Adding bit in Image header is best way. I totally agree.
The only disadvantage is that it may take time to update PE/COFF specification and take time to update the compiler to generate such bit.
If people want to wait for those spec update, I don't have any concern.


Personally, I don't think adding bit in FFS is a good idea. An image may come from anywhere - Flash, PCI card, Disk, Network. Hard to scale.


Thank you
Yao, Jiewen

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin Häuser
Sent: Friday, February 3, 2023 5:52 PM
To: Ard Biesheuvel <ardb@kernel.org>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem attributes table


Hi Ard and Jiewen,

(I'm replying from groups.io and cannot figure out how to CC Jiewen. Ugh.)

Personally, I'd rather have UEFI itself rely solely on the flag in the image file. If there is a way needed to handle images without the tag, in my opinion use some userland preprocessing tool to check and annotate it when generating the firmware image. Even if it's trivial for Intel, I hope we won't end up with a micro-disassembler in firmware just to tell whether a feature is enabled.

Best regards,
Marvin

On Fri, Feb 3, 2023 at 12:26 AM, Ard Biesheuvel wrote:
(cc Samer, Jose)

On Fri, 3 Feb 2023 at 02:16, Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> wrote:

Hello
Can we assume that the entrypoint of PE/COFF image is always ENDBR64, if the PE/COFF image is enlightened to support IBT?

I believe the compiler should do that, because the loader need use indirect call to the PE/COFF entrypoint.
That is an interesting idea. Even if we have some PE level annotation
for BTI/IBT at some point, it would be a good sanity check on the
image, because if the entry point does not have the guard instruction,
there is obviously something wrong.

However, on arm64, it is a bit more ambiguous, because there are
instructions for pointer authentication (PACIASP) that may be
interpreted as implicit guard instructions too, so the presence of
such an instruction at the entry point does not imply that BTI is
enabled in the entire image.

I'll code up a PoC with this, but us ARM folks should have a think
about how to spec this out and deploy this. I wonder

In the mean time, Michael is trying to round up the MS folks that are
involved in maintaining the PE/COFF spec, and ideally, we'd have some
image level flag that informs the loader whether all code sections in
the image were emitted with guard instructions for BTI/IBT.


We need more code to detect *all* runtime images. The logic could be:
1) PE/COFF loader (X86, ARM) detects IBT capability for all runtime images, and report it.
2) DXE Core collects such info.
2.1) If ALL runtime image supports IBT, then gMemoryAttributesTableForwardCfi = TRUE
2.2) Else, gMemoryAttributesTableForwardCfi = FALSE.

The benefit is that it is more reliable to support binary PE image use case. Hard to use build time flag for external binary PE ...
Agreed. This sounds like a good way to implement it.

-----Original Message-----
From: Michael Kubacki <mikuback@linux.microsoft.com<mailto:mikuback@linux.microsoft.com>>
Sent: Friday, February 3, 2023 8:24 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; ardb@kernel.org<mailto:ardb@kernel.org>; Kinney, Michael D
<michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Cc: Gao, Liming <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>; Yao, Jiewen
<jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Kubacki, Michael <michael.kubacki@microsoft.com<mailto:michael.kubacki@microsoft.com>>;
Sean Brogan <sean.brogan@microsoft.com<mailto:sean.brogan@microsoft.com>>; Rebecca Cran
<quic_rcran@quicinc.com<mailto:quic_rcran@quicinc.com>>; Leif Lindholm <quic_llindhol@quicinc.com<mailto:quic_llindhol@quicinc.com>>; Sami
Mujawar <sami.mujawar@arm.com<mailto:sami.mujawar@arm.com>>; Taylor Beebe <t@taylorbeebe.com<mailto:t@taylorbeebe.com>>
Subject: Re: [edk2-devel] [RFC PATCH 2/3] MdeModulePkg: Enable forward edge
CFI in mem attributes table

Ard, I am still actively tracking this for the PE/COFF spec.

Unfortunately, I don't have more firm info right now but I suggest
holding off on alternatives for the time being and I will reply back as
soon as the next steps are known.

Thanks,
Michael

On 2/2/2023 2:00 PM, Ard Biesheuvel wrote:
On Thu, 2 Feb 2023 at 19:49, Kinney, Michael D
<michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> wrote:

Hi Ard,

Since the PE/COFF image does not contain this information, is there an
option
to add the information to an FFS file. Either as a new bit in a standard header
or as a GUIDed section defined by EDK II?

Since an FV may contain content build from source and additional content
Integrated as binaries from other vendors, having a PCD that scopes this
attribute to all FVs many only work for FW builds that are 100% from sources.
I didn't quite consider that scenario tbh.

It would be nice if we could avoid another PI spec dance for this, and
get some kind of IBT/BTI annotation added to the PE/COFF spec. That
way, we cover this issue, as well as clear the way for the use if
IBT/BTI when running under the firmware.

Are TE images a concern here as well? I.e., are those likely to be
used for runtime DXE images in firmware volumes?


-----Original Message-----
From: Ard Biesheuvel <ardb@kernel.org<mailto:ardb@kernel.org>>
Sent: Thursday, February 2, 2023 10:04 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Ard Biesheuvel <ardb@kernel.org<mailto:ardb@kernel.org>>; Kinney, Michael D
<michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Gao, Liming <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>; Yao,
Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Kubacki, Michael
<michael.kubacki@microsoft.com<mailto:michael.kubacki@microsoft.com>>; Sean Brogan
<sean.brogan@microsoft.com<mailto:sean.brogan@microsoft.com>>; Rebecca
Cran <quic_rcran@quicinc.com<mailto:quic_rcran@quicinc.com>>; Leif Lindholm
<quic_llindhol@quicinc.com<mailto:quic_llindhol@quicinc.com>>; Sami Mujawar <sami.mujawar@arm.com<mailto:sami.mujawar@arm.com>>;
Taylor Beebe
<t@taylorbeebe.com<mailto:t@taylorbeebe.com>>
Subject: [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem
attributes table

The memory attributes table has been extended with a flag that indicates
whether or not the OS is permitted to map the EFI runtime code regions
with strict enforcement for IBT/BTI landing pad instructions.

This is generally a property of the firmware build, and so we can permit
a platform to set this property using a PCD, and put the burden on the
platform definition to set the toolchain options accordingly.

There is one snag, however: PE/COFF does not expose whether or not the
code was generated with landing pads, so if any runtime DXE drivers were
loaded from storage other than the firmware volumes, we must assume
that
setting the CFI flag in the memory attributes table is unsafe.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org<mailto:ardb@kernel.org>>
---
MdeModulePkg/Core/Dxe/DxeMain.h | 2 ++
MdeModulePkg/Core/Dxe/DxeMain.inf | 1 +
MdeModulePkg/Core/Dxe/Image/Image.c | 11 +++++++++++
MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c | 7 ++++++-
MdeModulePkg/MdeModulePkg.dec | 8 ++++++++
5 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h
b/MdeModulePkg/Core/Dxe/DxeMain.h
index 815a6b4bd844..427a5fc78f72 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.h
+++ b/MdeModulePkg/Core/Dxe/DxeMain.h
@@ -280,6 +280,8 @@ extern EFI_MEMORY_TYPE_INFORMATION
gMemoryTypeInformation[EfiMaxMemoryType + 1]
extern BOOLEAN gDispatcherRunning;

extern EFI_RUNTIME_ARCH_PROTOCOL gRuntimeTemplate;



+extern BOOLEAN gMemoryAttributesTableForwardCfi;

+

extern EFI_LOAD_FIXED_ADDRESS_CONFIGURATION_TABLE
gLoadModuleAtFixAddressConfigurationTable;

extern BOOLEAN
gLoadFixedAddressCodeMemoryReady;

//

diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf
b/MdeModulePkg/Core/Dxe/DxeMain.inf
index 35d5bf0dee6f..e6ff67773a69 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.inf
+++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
@@ -187,6 +187,7 @@ [Pcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask
## CONSUMES

gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ##
CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth
## CONSUMES

+
gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi
## CONSUMES



# [Hob]

# RESOURCE_DESCRIPTOR ## CONSUMES

diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c
b/MdeModulePkg/Core/Dxe/Image/Image.c
index 06cc6744b8c6..181fefdb6657 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -1398,6 +1398,17 @@ CoreLoadImageCommon (
CoreNewDebugImageInfoEntry
(EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL, &Image->Info, Image->Handle);

}



+ //

+ // If we loaded a runtime DXE driver from something other than a FV, it

+ // was not built as part of the firmware image, and so we cannot assume

+ // that it was built with IBT/BTI landing pads for forward edge control

+ // flow integrity.

+ //

+ if (!ImageIsFromFv &&

+ (Image->ImageContext.ImageCodeMemoryType ==
EfiRuntimeServicesCode)) {

+ gMemoryAttributesTableForwardCfi = FALSE;

+ }

+

//

// Reinstall loaded image protocol to fire any notifications

//

diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
index e07921371187..cdd35ade0a8a 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
@@ -89,6 +89,7 @@ BOOLEAN mMemoryAttributesTableEnable
= TRUE;
BOOLEAN mMemoryAttributesTableEndOfDxe = FALSE;

EFI_MEMORY_ATTRIBUTES_TABLE *mMemoryAttributesTable =
NULL;

BOOLEAN mMemoryAttributesTableReadyToBoot = FALSE;

+BOOLEAN gMemoryAttributesTableForwardCfi =
FixedPcdGetBool (PcdMemoryAttributesTableForwardCfi);



/**

Install MemoryAttributesTable.

@@ -182,7 +183,11 @@ InstallMemoryAttributesTable (
MemoryAttributesTable->Version =
EFI_MEMORY_ATTRIBUTES_TABLE_VERSION;

MemoryAttributesTable->NumberOfEntries = RuntimeEntryCount;

MemoryAttributesTable->DescriptorSize = (UINT32)DescriptorSize;

- MemoryAttributesTable->Reserved = 0;

+ if (gMemoryAttributesTableForwardCfi) {

+ MemoryAttributesTable->Flags =
EFI_MEMORY_ATTRIBUTES_FLAGS_RT_FORWARD_CONTROL_FLOW_GUARD;

+ } else {

+ MemoryAttributesTable->Flags = 0;

+ }

DEBUG ((DEBUG_VERBOSE, "MemoryAttributesTable:\n"));

DEBUG ((DEBUG_VERBOSE, " Version - 0x%08x\n",
MemoryAttributesTable->Version));

DEBUG ((DEBUG_VERBOSE, " NumberOfEntries - 0x%08x\n",
MemoryAttributesTable->NumberOfEntries));

diff --git a/MdeModulePkg/MdeModulePkg.dec
b/MdeModulePkg/MdeModulePkg.dec
index 9605c617b7a8..d336a38655a3 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1093,6 +1093,14 @@ [PcdsFixedAtBuild]
# @Prompt Enable UEFI Stack Guard.
gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0x30
001055



+ ## Indicates whether the EFI memory attributes table will inform the OS
that

+ # forward edge control flow guards have been inserted into the runtime
services

+ # code regions.

+ # TRUE - Runtime code has forward control flow guards.<BR>

+ # FALSE - Runtime code does not have forward control flow guards.<BR>

+ # @Prompt Enable forward control flow guards in EFI memory attributes
table

+
gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi|FAL
SE|BOOLEAN|0x30001056

+

[PcdsFixedAtBuild, PcdsPatchableInModule]

## Dynamic type PCD can be registered callback function for Pcd setting
action.

# PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum
number of callback function

--
2.39.1



[-- Attachment #2: Type: text/html, Size: 28776 bytes --]

Re: [edk2-devel] [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem attributes table

[Michael Kubacki]

This bit is now available in the PE/COFF spec in the Extended DLL 
Characteristics section.

https://learn.microsoft.com/windows/win32/debug/pe-format#extended-dll-characteristics

Name: "IMAGE_DLLCHARACTERISTICS_EX_FORWARD_CFI_COMPAT"
Value: "0x0040"

On 2/2/2023 7:24 PM, Michael Kubacki wrote:
> Ard, I am still actively tracking this for the PE/COFF spec.
> 
> Unfortunately, I don't have more firm info right now but I suggest 
> holding off on alternatives for the time being and I will reply back as 
> soon as the next steps are known.
> 
> Thanks,
> Michael
> 
> On 2/2/2023 2:00 PM, Ard Biesheuvel wrote:
>> On Thu, 2 Feb 2023 at 19:49, Kinney, Michael D
>> <michael.d.kinney@intel.com> wrote:
>>>
>>> Hi Ard,
>>>
>>> Since the PE/COFF image does not contain this information, is there 
>>> an option
>>> to add the information to an FFS file. Either as a new bit in a 
>>> standard header
>>> or as a GUIDed section defined by EDK II?
>>>
>>> Since an FV may contain content build from source and additional content
>>> Integrated as binaries from other vendors, having a PCD that scopes this
>>> attribute to all FVs many only work for FW builds that are 100% from 
>>> sources.
>>>
>>
>> I didn't quite consider that scenario tbh.
>>
>> It would be nice if we could avoid another PI spec dance for this, and
>> get some kind of IBT/BTI annotation added to the PE/COFF spec. That
>> way, we cover this issue, as well as clear the way for the use if
>> IBT/BTI when running under the firmware.
>>
>> Are TE images a concern here as well? I.e., are those likely to be
>> used for runtime DXE images in firmware volumes?
>>
>>
>>>
>>>> -----Original Message-----
>>>> From: Ard Biesheuvel <ardb@kernel.org>
>>>> Sent: Thursday, February 2, 2023 10:04 AM
>>>> To: devel@edk2.groups.io
>>>> Cc: Ard Biesheuvel <ardb@kernel.org>; Kinney, Michael D 
>>>> <michael.d.kinney@intel.com>; Gao, Liming 
>>>> <gaoliming@byosoft.com.cn>; Yao,
>>>> Jiewen <jiewen.yao@intel.com>; Kubacki, Michael 
>>>> <michael.kubacki@microsoft.com>; Sean Brogan 
>>>> <sean.brogan@microsoft.com>; Rebecca
>>>> Cran <quic_rcran@quicinc.com>; Leif Lindholm 
>>>> <quic_llindhol@quicinc.com>; Sami Mujawar <sami.mujawar@arm.com>; 
>>>> Taylor Beebe
>>>> <t@taylorbeebe.com>
>>>> Subject: [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in 
>>>> mem attributes table
>>>>
>>>> The memory attributes table has been extended with a flag that 
>>>> indicates
>>>> whether or not the OS is permitted to map the EFI runtime code regions
>>>> with strict enforcement for IBT/BTI landing pad instructions.
>>>>
>>>> This is generally a property of the firmware build, and so we can 
>>>> permit
>>>> a platform to set this property using a PCD, and put the burden on the
>>>> platform definition to set the toolchain options accordingly.
>>>>
>>>> There is one snag, however: PE/COFF does not expose whether or not the
>>>> code was generated with landing pads, so if any runtime DXE drivers 
>>>> were
>>>> loaded from storage other than the firmware volumes, we must assume 
>>>> that
>>>> setting the CFI flag in the memory attributes table is unsafe.
>>>>
>>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>>> ---
>>>>   MdeModulePkg/Core/Dxe/DxeMain.h                    |  2 ++
>>>>   MdeModulePkg/Core/Dxe/DxeMain.inf                  |  1 +
>>>>   MdeModulePkg/Core/Dxe/Image/Image.c                | 11 +++++++++++
>>>>   MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c |  7 ++++++-
>>>>   MdeModulePkg/MdeModulePkg.dec                      |  8 ++++++++
>>>>   5 files changed, 28 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h 
>>>> b/MdeModulePkg/Core/Dxe/DxeMain.h
>>>> index 815a6b4bd844..427a5fc78f72 100644
>>>> --- a/MdeModulePkg/Core/Dxe/DxeMain.h
>>>> +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
>>>> @@ -280,6 +280,8 @@ extern EFI_MEMORY_TYPE_INFORMATION  
>>>> gMemoryTypeInformation[EfiMaxMemoryType + 1]
>>>>   extern BOOLEAN                    gDispatcherRunning;
>>>>
>>>>   extern EFI_RUNTIME_ARCH_PROTOCOL  gRuntimeTemplate;
>>>>
>>>>
>>>>
>>>> +extern BOOLEAN                    gMemoryAttributesTableForwardCfi;
>>>>
>>>> +
>>>>
>>>>   extern EFI_LOAD_FIXED_ADDRESS_CONFIGURATION_TABLE  
>>>> gLoadModuleAtFixAddressConfigurationTable;
>>>>
>>>>   extern BOOLEAN                                     
>>>> gLoadFixedAddressCodeMemoryReady;
>>>>
>>>>   //
>>>>
>>>> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf 
>>>> b/MdeModulePkg/Core/Dxe/DxeMain.inf
>>>> index 35d5bf0dee6f..e6ff67773a69 100644
>>>> --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
>>>> +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
>>>> @@ -187,6 +187,7 @@ [Pcd]
>>>>     
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask                   ## CONSUMES
>>>>
>>>>     
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                           ## CONSUMES
>>>>
>>>>     
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth           ## CONSUMES
>>>>
>>>> +  
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi         ## CONSUMES
>>>>
>>>>
>>>>
>>>>   # [Hob]
>>>>
>>>>   # RESOURCE_DESCRIPTOR   ## CONSUMES
>>>>
>>>> diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c 
>>>> b/MdeModulePkg/Core/Dxe/Image/Image.c
>>>> index 06cc6744b8c6..181fefdb6657 100644
>>>> --- a/MdeModulePkg/Core/Dxe/Image/Image.c
>>>> +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
>>>> @@ -1398,6 +1398,17 @@ CoreLoadImageCommon (
>>>>       CoreNewDebugImageInfoEntry (EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL, 
>>>> &Image->Info, Image->Handle);
>>>>
>>>>     }
>>>>
>>>>
>>>>
>>>> +  //
>>>>
>>>> +  // If we loaded a runtime DXE driver from something other than a 
>>>> FV, it
>>>>
>>>> +  // was not built as part of the firmware image, and so we cannot 
>>>> assume
>>>>
>>>> +  // that it was built with IBT/BTI landing pads for forward edge 
>>>> control
>>>>
>>>> +  // flow integrity.
>>>>
>>>> +  //
>>>>
>>>> +  if (!ImageIsFromFv &&
>>>>
>>>> +      (Image->ImageContext.ImageCodeMemoryType == 
>>>> EfiRuntimeServicesCode)) {
>>>>
>>>> +    gMemoryAttributesTableForwardCfi = FALSE;
>>>>
>>>> +  }
>>>>
>>>> +
>>>>
>>>>     //
>>>>
>>>>     // Reinstall loaded image protocol to fire any notifications
>>>>
>>>>     //
>>>>
>>>> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c 
>>>> b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
>>>> index e07921371187..cdd35ade0a8a 100644
>>>> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
>>>> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
>>>> @@ -89,6 +89,7 @@ BOOLEAN                      
>>>> mMemoryAttributesTableEnable      = TRUE;
>>>>   BOOLEAN                      mMemoryAttributesTableEndOfDxe    = 
>>>> FALSE;
>>>>
>>>>   EFI_MEMORY_ATTRIBUTES_TABLE  *mMemoryAttributesTable           = 
>>>> NULL;
>>>>
>>>>   BOOLEAN                      mMemoryAttributesTableReadyToBoot = 
>>>> FALSE;
>>>>
>>>> +BOOLEAN                      gMemoryAttributesTableForwardCfi  = 
>>>> FixedPcdGetBool (PcdMemoryAttributesTableForwardCfi);
>>>>
>>>>
>>>>
>>>>   /**
>>>>
>>>>     Install MemoryAttributesTable.
>>>>
>>>> @@ -182,7 +183,11 @@ InstallMemoryAttributesTable (
>>>>     MemoryAttributesTable->Version         = 
>>>> EFI_MEMORY_ATTRIBUTES_TABLE_VERSION;
>>>>
>>>>     MemoryAttributesTable->NumberOfEntries = RuntimeEntryCount;
>>>>
>>>>     MemoryAttributesTable->DescriptorSize  = (UINT32)DescriptorSize;
>>>>
>>>> -  MemoryAttributesTable->Reserved        = 0;
>>>>
>>>> +  if (gMemoryAttributesTableForwardCfi) {
>>>>
>>>> +    MemoryAttributesTable->Flags         = 
>>>> EFI_MEMORY_ATTRIBUTES_FLAGS_RT_FORWARD_CONTROL_FLOW_GUARD;
>>>>
>>>> +  } else {
>>>>
>>>> +    MemoryAttributesTable->Flags         = 0;
>>>>
>>>> +  }
>>>>
>>>>     DEBUG ((DEBUG_VERBOSE, "MemoryAttributesTable:\n"));
>>>>
>>>>     DEBUG ((DEBUG_VERBOSE, "  Version              - 0x%08x\n", 
>>>> MemoryAttributesTable->Version));
>>>>
>>>>     DEBUG ((DEBUG_VERBOSE, "  NumberOfEntries      - 0x%08x\n", 
>>>> MemoryAttributesTable->NumberOfEntries));
>>>>
>>>> diff --git a/MdeModulePkg/MdeModulePkg.dec 
>>>> b/MdeModulePkg/MdeModulePkg.dec
>>>> index 9605c617b7a8..d336a38655a3 100644
>>>> --- a/MdeModulePkg/MdeModulePkg.dec
>>>> +++ b/MdeModulePkg/MdeModulePkg.dec
>>>> @@ -1093,6 +1093,14 @@ [PcdsFixedAtBuild]
>>>>     # @Prompt Enable UEFI Stack Guard.
>>>>
>>>>     
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0x30001055
>>>>
>>>>
>>>>
>>>> +  ## Indicates whether the EFI memory attributes table will inform 
>>>> the OS that
>>>>
>>>> +  #  forward edge control flow guards have been inserted into the 
>>>> runtime services
>>>>
>>>> +  #  code regions.
>>>>
>>>> +  #   TRUE  - Runtime code has forward control flow guards.<BR>
>>>>
>>>> +  #   FALSE - Runtime code does not have forward control flow 
>>>> guards.<BR>
>>>>
>>>> +  # @Prompt Enable forward control flow guards in EFI memory 
>>>> attributes table
>>>>
>>>> +  
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi|FALSE|BOOLEAN|0x30001056
>>>>
>>>> +
>>>>
>>>>   [PcdsFixedAtBuild, PcdsPatchableInModule]
>>>>
>>>>     ## Dynamic type PCD can be registered callback function for Pcd 
>>>> setting action.
>>>>
>>>>     #  PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum 
>>>> number of callback function
>>>>
>>>> -- 
>>>> 2.39.1
>>>
>>
>>
>> 
>>

Re: [edk2-devel] [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem attributes table

[Marvin Häuser]

[-- Attachment #1: Type: text/plain, Size: 629 bytes --]

Hi Ard,

Short story: No, TE is not a concern for RT drivers, at least not upstream [1].

Long story: Even if a downstream added support for RT relocation itself, TE images are loaded misaligned by DxeCore, because it lacks a (terrible) workaround as done in PeiCore [2]. It doesn’t look like anyone tried to use them in DXE at all.

Best regards,
Marvin

[1] https://github.com/tianocore/edk2/blob/11f0014c0e3046e3762eac420b760091d0cdc063/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1759

[2] https://github.com/tianocore/edk2/blob/11f0014c0e3046e3762eac420b760091d0cdc063/MdeModulePkg/Core/Pei/Image/Image.c#L412

[-- Attachment #2: Type: text/html, Size: 1109 bytes --]

Re: [edk2-devel] [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem attributes table

[Ard Biesheuvel]

On Fri, 3 Feb 2023 at 09:25, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
> Hi Ard,
>
> Short story: No, TE is not a concern for RT drivers, at least not upstream [1].
>
> Long story: Even if a downstream added support for RT relocation itself, TE images are loaded misaligned by DxeCore, because it lacks a (terrible) workaround as done in PeiCore [2]. It doesn’t look like anyone tried to use them in DXE at all.
>
> Best regards,
> Marvin
>
> [1] https://github.com/tianocore/edk2/blob/11f0014c0e3046e3762eac420b760091d0cdc063/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1759
>
> [2] https://github.com/tianocore/edk2/blob/11f0014c0e3046e3762eac420b760091d0cdc063/MdeModulePkg/Core/Pei/Image/Image.c#L412

Ah thanks for digging that up.

So all the more reason to add this to the PE/COFF spec rather than
anywhere else.

Re: [edk2-devel] [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem attributes table

[Marvin Häuser]

Sorry, forgot to ask (and it’s probably obvious), but there is a sane tag for ELFs that could be translated to the potential new PE tag, right? For both GCC and Clang builds?

> On 3. Feb 2023, at 09:28, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> On Fri, 3 Feb 2023 at 09:25, Marvin Häuser <mhaeuser@posteo.de> wrote:
>> 
>> Hi Ard,
>> 
>> Short story: No, TE is not a concern for RT drivers, at least not upstream [1].
>> 
>> Long story: Even if a downstream added support for RT relocation itself, TE images are loaded misaligned by DxeCore, because it lacks a (terrible) workaround as done in PeiCore [2]. It doesn’t look like anyone tried to use them in DXE at all.
>> 
>> Best regards,
>> Marvin
>> 
>> [1] https://github.com/tianocore/edk2/blob/11f0014c0e3046e3762eac420b760091d0cdc063/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1759
>> 
>> [2] https://github.com/tianocore/edk2/blob/11f0014c0e3046e3762eac420b760091d0cdc063/MdeModulePkg/Core/Pei/Image/Image.c#L412
> 
> Ah thanks for digging that up.
> 
> So all the more reason to add this to the PE/COFF spec rather than
> anywhere else.

[RFC PATCH 3/3] ArmVirtPkg/ArmVirtQemu: Implement BTI for runtime regions

[Ard Biesheuvel]

Add a build option RUNTIM_BTI_ENABLE, and wire it up to the newly added
PCD that controls the value of the BTI flag in the memory attributes
table, as well as the command line options passed to the compiler to get
it to emit BTI landing pads in BASE and DXE_RUNTIME_DRIVER modules.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmVirtPkg/ArmVirtQemu.dsc | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 0f1c6395488a..0a67fe250d86 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -32,6 +32,7 @@ [Defines]
   DEFINE TPM2_ENABLE             = FALSE
   DEFINE TPM2_CONFIG_ENABLE      = FALSE
   DEFINE CAVIUM_ERRATUM_27456    = FALSE
+  DEFINE RUNTIME_BTI_ENABLE      = TRUE
 
   #
   # Network definition
@@ -124,6 +125,11 @@ [BuildOptions]
   GCC:*_*_AARCH64_CC_XIPFLAGS ==
 !endif
 
+!if $(RUNTIME_BTI_ENABLE) == TRUE
+[BuildOptions.common.BASE,BuildOptions.common.DXE_RUNTIME_DRIVER]
+  GCC:*_*_AARCH64_CC_FLAGS = -mbranch-protection=bti
+!endif
+
 !include NetworkPkg/NetworkBuildOptions.dsc.inc
 
 ################################################################################
@@ -148,6 +154,8 @@ [PcdsFeatureFlag.common]
 [PcdsFixedAtBuild.common]
 !if $(ARCH) == AARCH64
   gArmTokenSpaceGuid.PcdVFPEnabled|1
+
+  gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi|$(RUNTIME_BTI_ENABLE)
 !endif
 
   gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x4007c000
-- 
2.39.1