[PATCH v2 00/17] Enable BTI support in memory attributes table

[PATCH v2 00/17] Enable BTI support in memory attributes table

[Ard Biesheuvel]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 5961 bytes --]

Implement version 2 of the memory attributes table, which now contains a
flag informing the OS whether or not code regions may be mapped with CFI
mitigations such as IBT or BTI enabled.

This series covers roughly the following parts:

- (AARCH64) Annotate ELF objects generated from asm as BTI compatible
  when BTI codegen is enabled
- Update the BaseTools to emit the appropriate PE/COFF annotation when a
  BTI/IBT compatible ELF executable is converted to PE/COFF
- Take this PE/COFF annotation into account when populating the memory
  attributes table in the DXE core

TODO:
- X64 changes to make the code IBT compatible and emit the ELF note
- Figure out how to generate such executables with native PE toolchains
- Implement BTI/IBT enforcement at boot time - this is something I
  intend to look into next.

Can be tested with the CLANG38 toolchain (both Clang compiler and LLD
linker, version 3.8 or newer) with the following build options.

[BuildOptions]
  GCC:*_*_AARCH64_PP_FLAGS = -mbranch-protection=bti
  GCC:*_*_AARCH64_CC_FLAGS = -mbranch-protection=bti
  GCC:*_*_AARCH64_DLINK_FLAGS = -fuse-ld=lld -Wl,--no-relax,--no-pie,-z,bti-report=error

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>
Cc: Marvin Häuser <mhaeuser@posteo.de>
Cc: Bob Feng <bob.c.feng@intel.com>

Ard Biesheuvel (17):
  MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note
  MdePkg/BaseCpuLib AARCH64: Make asm files BTI compatible
  MdePkg/BaseIoLibIntrinsic AARCH64: Make asm files BTI compatible
  MdePkg/BaseLib AARCH64: Make LongJump() BTI compatible
  MdePkg/BaseLib AARCH64: Make asm files BTI compatible
  MdePkg/BaseMemoryLibOptDxe AARCH64: Make asm files BTI compatible
  MdePkg/BaseSynchronizationLib AARCH64: Make asm files BTI compatible
  MdePkg/BaseRngLib AARCH64: Make asm files BTI compatible
  ArmPkg: Emit BTI opcodes when BTI codegen is enabled
  ArmPkg/GccLto AARCH64: Add BTI note to LTO helper library
  ArmPkg, BaseTools AARCH64: Add BTI ELF note to .hii objects
  ArmPlatformPkg/PrePeiCore: Make vector table object BTI compatible
  BaseTools/GenFw: Parse IBT/BTI support status from ELF note
  BaseTools/GenFw: Add DllCharacteristicsEx field to debug data
  MdePkg: Update MemoryAttributesTable to v2.10
  MdePkg/PeCoffLib: Capture DLL characteristics fieldis in image context
  MdeModulePkg: Enable forward edge CFI in mem attributes table

 ArmPkg/Include/AsmMacroIoLibV8.h                                |   3 +-
 ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S       |   3 +-
 ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S                       |   4 +-
 ArmPkg/Library/GccLto/liblto-aarch64.a                          | Bin 1016 -> 1128 bytes
 ArmPkg/Library/GnuNoteBti.bin                                   | Bin 0 -> 32 bytes
 ArmPlatformPkg/PrePeiCore/AArch64/Exception.S                   |   2 +
 ArmVirtPkg/Library/ArmPlatformLibQemu/IdMap.S                   |   2 +
 BaseTools/Conf/tools_def.template                               |   4 +-
 BaseTools/Source/C/GenFw/Elf64Convert.c                         | 104 +++++++++++++++++---
 BaseTools/Source/C/GenFw/GenFw.c                                |   3 +-
 BaseTools/Source/C/GenFw/elf_common.h                           |   9 ++
 BaseTools/Source/C/Include/IndustryStandard/PeImage.h           |  13 ++-
 MdeModulePkg/Core/Dxe/DxeMain.h                                 |   2 +
 MdeModulePkg/Core/Dxe/Image/Image.c                             |  10 ++
 MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c              |   8 +-
 MdePkg/Include/AArch64/ProcessorBind.h                          |  31 ++++++
 MdePkg/Include/Guid/MemoryAttributesTable.h                     |   8 +-
 MdePkg/Include/IndustryStandard/PeImage.h                       |  13 ++-
 MdePkg/Include/Library/PeCoffLib.h                              |   6 ++
 MdePkg/Library/BaseCpuLib/AArch64/CpuFlushTlb.S                 |   1 +
 MdePkg/Library/BaseCpuLib/AArch64/CpuSleep.S                    |   1 +
 MdePkg/Library/BaseIoLibIntrinsic/AArch64/ArmVirtMmio.S         |   8 ++
 MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.S                  |   1 +
 MdePkg/Library/BaseLib/AArch64/DisableInterrupts.S              |   1 +
 MdePkg/Library/BaseLib/AArch64/EnableInterrupts.S               |   1 +
 MdePkg/Library/BaseLib/AArch64/GetInterruptsState.S             |   1 +
 MdePkg/Library/BaseLib/AArch64/MemoryFence.S                    |   1 +
 MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S                |   5 +-
 MdePkg/Library/BaseLib/AArch64/SpeculationBarrier.S             |   1 +
 MdePkg/Library/BaseLib/AArch64/SwitchStack.S                    |   2 +
 MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CompareGuid.S        |   1 +
 MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CompareMem.S         |   1 +
 MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CopyMem.S            |   1 +
 MdePkg/Library/BaseMemoryLibOptDxe/AArch64/ScanMem.S            |   1 +
 MdePkg/Library/BaseMemoryLibOptDxe/AArch64/SetMem.S             |   5 +
 MdePkg/Library/BasePeCoffLib/BasePeCoff.c                       |  46 ++++++---
 MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.S              |   3 +-
 MdePkg/Library/BaseRngLib/AArch64/ArmRng.S                      |   1 +
 MdePkg/Library/BaseSynchronizationLib/AArch64/Synchronization.S |   5 +
 39 files changed, 270 insertions(+), 42 deletions(-)
 create mode 100644 ArmPkg/Library/GnuNoteBti.bin

-- 
2.39.2

[PATCH v2 01/17] MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note

[Ard Biesheuvel]

Implement a CPP macro that can be called from .S files to emit the .note
section carrying the annotation that informs the linker that the object
file is compatible with BTI control flow integrity checks.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdePkg/Include/AArch64/ProcessorBind.h | 31 ++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
index abe2571245c665f3..11814f1ffaef698a 100644
--- a/MdePkg/Include/AArch64/ProcessorBind.h
+++ b/MdePkg/Include/AArch64/ProcessorBind.h
@@ -186,6 +186,37 @@ typedef INT64 INTN;
 #define GCC_ASM_IMPORT(func__)  \
          .extern  _CONCATENATE (__USER_LABEL_PREFIX__, func__)
 
+#if defined(__ARM_FEATURE_BTI_DEFAULT) && __ARM_FEATURE_BTI_DEFAULT == 1
+#define AARCH64_BTI(__type)                                        \
+    .ifnc         __type,                                         ;\
+    bti           __type                                          ;\
+    .endif                                                        ;\
+    .ifndef       .Lgnu_bti_notesize                              ;\
+    .pushsection  .note.gnu.property, "a"                         ;\
+    .set          NT_GNU_PROPERTY_TYPE_0, 0x5                     ;\
+    .set          GNU_PROPERTY_AARCH64_FEATURE_1_AND, 0xc0000000  ;\
+    .set          GNU_PROPERTY_AARCH64_FEATURE_1_BTI, 0x1         ;\
+    .align        3                                               ;\
+    .long         .Lnamesize                                      ;\
+    .long         .Lgnu_bti_notesize                              ;\
+    .long         NT_GNU_PROPERTY_TYPE_0                          ;\
+0:  .asciz        "GNU"                                           ;\
+    .set          .Lnamesize, . - 0b                              ;\
+    .align        3                                               ;\
+1:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_AND              ;\
+    .long         .Lvalsize                                       ;\
+2:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_BTI              ;\
+    .set          .Lvalsize, . - 2b                               ;\
+    .align        3                                               ;\
+    .set          .Lgnu_bti_notesize, . - 1b                      ;\
+    .popsection                                                   ;\
+    .endif
+#endif
+
+#endif
+
+#ifndef AARCH64_BTI
+#define AARCH64_BTI(__type)
 #endif
 
 /**
-- 
2.39.2

[PATCH v2 02/17] MdePkg/BaseCpuLib AARCH64: Make asm files BTI compatible

[Ard Biesheuvel]

Add the BTI instructions and the associated note to make the AArch64 asm
objects compatible with BTI enforcement.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdePkg/Library/BaseCpuLib/AArch64/CpuFlushTlb.S | 1 +
 MdePkg/Library/BaseCpuLib/AArch64/CpuSleep.S    | 1 +
 2 files changed, 2 insertions(+)

diff --git a/MdePkg/Library/BaseCpuLib/AArch64/CpuFlushTlb.S b/MdePkg/Library/BaseCpuLib/AArch64/CpuFlushTlb.S
index 82a7232268f59809..12c2421b6d327a7b 100644
--- a/MdePkg/Library/BaseCpuLib/AArch64/CpuFlushTlb.S
+++ b/MdePkg/Library/BaseCpuLib/AArch64/CpuFlushTlb.S
@@ -26,6 +26,7 @@ GCC_ASM_EXPORT(CpuFlushTlb)
 #  )#
 #
 ASM_PFX(CpuFlushTlb):
+  AARCH64_BTI(c)
   tlbi  vmalle1                 // Invalidate Inst TLB and Data TLB
   dsb   sy
   isb
diff --git a/MdePkg/Library/BaseCpuLib/AArch64/CpuSleep.S b/MdePkg/Library/BaseCpuLib/AArch64/CpuSleep.S
index 410a271565edfb68..6853e0c56e0bb135 100644
--- a/MdePkg/Library/BaseCpuLib/AArch64/CpuSleep.S
+++ b/MdePkg/Library/BaseCpuLib/AArch64/CpuSleep.S
@@ -29,5 +29,6 @@ GCC_ASM_EXPORT(CpuSleep)
 #
 
 ASM_PFX(CpuSleep):
+    AARCH64_BTI(c)
     wfi
     ret
-- 
2.39.2

[PATCH v2 03/17] MdePkg/BaseIoLibIntrinsic AARCH64: Make asm files BTI compatible

[Ard Biesheuvel]

Add the BTI instructions and the associated note to make the AArch64 asm
objects compatible with BTI enforcement.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmVirtPkg/Library/ArmPlatformLibQemu/IdMap.S           | 2 ++
 MdePkg/Library/BaseIoLibIntrinsic/AArch64/ArmVirtMmio.S | 8 ++++++++
 2 files changed, 10 insertions(+)

diff --git a/ArmVirtPkg/Library/ArmPlatformLibQemu/IdMap.S b/ArmVirtPkg/Library/ArmPlatformLibQemu/IdMap.S
index 584ffcb3ebe2ef34..b67f09ab61f2474e 100644
--- a/ArmVirtPkg/Library/ArmPlatformLibQemu/IdMap.S
+++ b/ArmVirtPkg/Library/ArmPlatformLibQemu/IdMap.S
@@ -57,3 +57,5 @@ idmap:      /* level 0 */
   .quad     PAGE_XIP | (idx << 12)        // 2044 KiB of R-X flash mappings
   .set      idx, idx + 1
   .endr
+
+AARCH64_BTI()
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/AArch64/ArmVirtMmio.S b/MdePkg/Library/BaseIoLibIntrinsic/AArch64/ArmVirtMmio.S
index 00f1abec15f0ef08..77f562697ef971b5 100644
--- a/MdePkg/Library/BaseIoLibIntrinsic/AArch64/ArmVirtMmio.S
+++ b/MdePkg/Library/BaseIoLibIntrinsic/AArch64/ArmVirtMmio.S
@@ -29,6 +29,7 @@ GCC_ASM_EXPORT(MmioWrite64Internal)
 //  @return The value read.
 //
 ASM_PFX(MmioRead8Internal):
+  AARCH64_BTI(c)
   ldrb    w0, [x0]
   dmb     ld
   ret
@@ -44,6 +45,7 @@ ASM_PFX(MmioRead8Internal):
 //  @param  Value   The value to write to the MMIO register.
 //
 ASM_PFX(MmioWrite8Internal):
+  AARCH64_BTI(c)
   dmb     st
   strb    w1, [x0]
   ret
@@ -60,6 +62,7 @@ ASM_PFX(MmioWrite8Internal):
 //  @return The value read.
 //
 ASM_PFX(MmioRead16Internal):
+  AARCH64_BTI(c)
   ldrh    w0, [x0]
   dmb     ld
   ret
@@ -75,6 +78,7 @@ ASM_PFX(MmioRead16Internal):
 //  @param  Value   The value to write to the MMIO register.
 //
 ASM_PFX(MmioWrite16Internal):
+  AARCH64_BTI(c)
   dmb     st
   strh    w1, [x0]
   ret
@@ -91,6 +95,7 @@ ASM_PFX(MmioWrite16Internal):
 //  @return The value read.
 //
 ASM_PFX(MmioRead32Internal):
+  AARCH64_BTI(c)
   ldr     w0, [x0]
   dmb     ld
   ret
@@ -106,6 +111,7 @@ ASM_PFX(MmioRead32Internal):
 //  @param  Value   The value to write to the MMIO register.
 //
 ASM_PFX(MmioWrite32Internal):
+  AARCH64_BTI(c)
   dmb     st
   str     w1, [x0]
   ret
@@ -122,6 +128,7 @@ ASM_PFX(MmioWrite32Internal):
 //  @return The value read.
 //
 ASM_PFX(MmioRead64Internal):
+  AARCH64_BTI(c)
   ldr     x0, [x0]
   dmb     ld
   ret
@@ -137,6 +144,7 @@ ASM_PFX(MmioRead64Internal):
 //  @param  Value   The value to write to the MMIO register.
 //
 ASM_PFX(MmioWrite64Internal):
+  AARCH64_BTI(c)
   dmb     st
   str     x1, [x0]
   ret
-- 
2.39.2

[PATCH v2 04/17] MdePkg/BaseLib AARCH64: Make LongJump() BTI compatible

[Ard Biesheuvel]

Currently, the AArch64 implementation of LongJump() avoids using the RET
instruction to perform the jump, even though the target address is held
in the link register X30, as the nature of a long jump implies that the
ordinary return address prediction machinery will not be able to make a
correct prediction.

However, LongJump() is rarely used, and the return stack will be out of
sync in any case, so this optimization has little value in practice, and
given that indirect calls other than function returns require a BTI
landing pad at the call site, this optimization is not compatible with
BTI. So let's just use RET instead.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
index deefdf526b95ca93..1d5cfbf64470452f 100644
--- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
+++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
@@ -85,7 +85,6 @@ ASM_PFX(InternalLongJump):
         cmp     w1, #0
         mov     w0, #1
         csel    w0, w1, w0, ne
-        // use br not ret, as ret is guaranteed to mispredict
-        br      x30
+        ret
 
 ASM_FUNCTION_REMOVE_IF_UNREFERENCED
-- 
2.39.2

[PATCH v2 05/17] MdePkg/BaseLib AARCH64: Make asm files BTI compatible

[Ard Biesheuvel]

Add the BTI instructions and the associated note to make the AArch64 asm
objects compatible with BTI enforcement.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.S      | 1 +
 MdePkg/Library/BaseLib/AArch64/DisableInterrupts.S  | 1 +
 MdePkg/Library/BaseLib/AArch64/EnableInterrupts.S   | 1 +
 MdePkg/Library/BaseLib/AArch64/GetInterruptsState.S | 1 +
 MdePkg/Library/BaseLib/AArch64/MemoryFence.S        | 1 +
 MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S    | 2 ++
 MdePkg/Library/BaseLib/AArch64/SpeculationBarrier.S | 1 +
 MdePkg/Library/BaseLib/AArch64/SwitchStack.S        | 2 ++
 8 files changed, 10 insertions(+)

diff --git a/MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.S b/MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.S
index 7524fb18820c2fa3..24a1ac371884bb1d 100644
--- a/MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.S
+++ b/MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.S
@@ -27,5 +27,6 @@ GCC_ASM_EXPORT(CpuBreakpoint)
 #  );
 #
 ASM_PFX(CpuBreakpoint):
+    AARCH64_BTI(c)
     svc   0xdbdb    // Superviser exception. Takes 16bit arg -> Armv7 had 'swi' here.
     ret
diff --git a/MdePkg/Library/BaseLib/AArch64/DisableInterrupts.S b/MdePkg/Library/BaseLib/AArch64/DisableInterrupts.S
index f0faf16b06a3fcae..3f562461614ad294 100644
--- a/MdePkg/Library/BaseLib/AArch64/DisableInterrupts.S
+++ b/MdePkg/Library/BaseLib/AArch64/DisableInterrupts.S
@@ -26,5 +26,6 @@ GCC_ASM_EXPORT(DisableInterrupts)
 #  );
 #
 ASM_PFX(DisableInterrupts):
+   AARCH64_BTI(c)
    msr  daifset, #DAIF_WR_IRQ_BIT
    ret
diff --git a/MdePkg/Library/BaseLib/AArch64/EnableInterrupts.S b/MdePkg/Library/BaseLib/AArch64/EnableInterrupts.S
index 97eeb13fbe5d2145..0f1377f51c7e88f7 100644
--- a/MdePkg/Library/BaseLib/AArch64/EnableInterrupts.S
+++ b/MdePkg/Library/BaseLib/AArch64/EnableInterrupts.S
@@ -26,5 +26,6 @@ GCC_ASM_EXPORT(EnableInterrupts)
 #  );
 #
 ASM_PFX(EnableInterrupts):
+    AARCH64_BTI(c)
     msr  daifclr, #DAIF_WR_IRQ_BIT
     ret
diff --git a/MdePkg/Library/BaseLib/AArch64/GetInterruptsState.S b/MdePkg/Library/BaseLib/AArch64/GetInterruptsState.S
index bf8b829bb2b1749d..26787a5b9bddcd7e 100644
--- a/MdePkg/Library/BaseLib/AArch64/GetInterruptsState.S
+++ b/MdePkg/Library/BaseLib/AArch64/GetInterruptsState.S
@@ -33,6 +33,7 @@ GCC_ASM_EXPORT(GetInterruptState)
 # );
 #
 ASM_PFX(GetInterruptState):
+    AARCH64_BTI(c)
     mrs    x0, daif
     tst    x0, #DAIF_RD_IRQ_BIT   // Check IRQ mask; set Z=1 if clear/unmasked
     cset   w0, eq                 // if Z=1 (eq) return 1, else 0
diff --git a/MdePkg/Library/BaseLib/AArch64/MemoryFence.S b/MdePkg/Library/BaseLib/AArch64/MemoryFence.S
index e553bd2dc9fcf63f..ad5b92a9a72bd65c 100644
--- a/MdePkg/Library/BaseLib/AArch64/MemoryFence.S
+++ b/MdePkg/Library/BaseLib/AArch64/MemoryFence.S
@@ -28,6 +28,7 @@ GCC_ASM_EXPORT(MemoryFence)
 #  );
 #
 ASM_PFX(MemoryFence):
+    AARCH64_BTI(c)
     // System wide Data Memory Barrier.
     dmb sy
     ret
diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
index 1d5cfbf64470452f..0d902d94d31c4a35 100644
--- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
+++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
@@ -46,6 +46,7 @@ GCC_ASM_EXPORT(InternalLongJump)
 #  );
 #
 ASM_PFX(SetJump):
+        AARCH64_BTI(c)
         mov     x16, sp // use IP0 so save SP
 #define REG_PAIR(REG1, REG2, OFFS)      stp REG1, REG2, [x0, OFFS]
 #define REG_ONE(REG1, OFFS)             str REG1, [x0, OFFS]
@@ -75,6 +76,7 @@ ASM_PFX(SetJump):
 #  );
 #
 ASM_PFX(InternalLongJump):
+        AARCH64_BTI(c)
 #define REG_PAIR(REG1, REG2, OFFS)      ldp REG1, REG2, [x0, OFFS]
 #define REG_ONE(REG1, OFFS)             ldr REG1, [x0, OFFS]
         GPR_LAYOUT
diff --git a/MdePkg/Library/BaseLib/AArch64/SpeculationBarrier.S b/MdePkg/Library/BaseLib/AArch64/SpeculationBarrier.S
index a20d6aed0cdd5284..248ee01e52c27367 100644
--- a/MdePkg/Library/BaseLib/AArch64/SpeculationBarrier.S
+++ b/MdePkg/Library/BaseLib/AArch64/SpeculationBarrier.S
@@ -28,6 +28,7 @@ GCC_ASM_EXPORT(SpeculationBarrier)
 #  );
 #
 ASM_PFX(SpeculationBarrier):
+    AARCH64_BTI(c)
     dsb  sy
     isb
     ret
diff --git a/MdePkg/Library/BaseLib/AArch64/SwitchStack.S b/MdePkg/Library/BaseLib/AArch64/SwitchStack.S
index f3bce6a09bc2d555..837c65b45e73024e 100644
--- a/MdePkg/Library/BaseLib/AArch64/SwitchStack.S
+++ b/MdePkg/Library/BaseLib/AArch64/SwitchStack.S
@@ -35,6 +35,7 @@ GCC_ASM_EXPORT(CpuPause)
 #  );
 #
 ASM_PFX(InternalSwitchStackAsm):
+    AARCH64_BTI(c)
     mov   x29, #0
     mov   x30, x0
     mov   sp, x3
@@ -57,6 +58,7 @@ ASM_PFX(InternalSwitchStackAsm):
 #  )
 #
 ASM_PFX(CpuPause):
+    AARCH64_BTI(c)
     nop
     nop
     nop
-- 
2.39.2

[PATCH v2 06/17] MdePkg/BaseMemoryLibOptDxe AARCH64: Make asm files BTI compatible

[Ard Biesheuvel]

Add the BTI instructions and the associated note to make the AArch64 asm
objects compatible with BTI enforcement.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CompareGuid.S | 1 +
 MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CompareMem.S  | 1 +
 MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CopyMem.S     | 1 +
 MdePkg/Library/BaseMemoryLibOptDxe/AArch64/ScanMem.S     | 1 +
 MdePkg/Library/BaseMemoryLibOptDxe/AArch64/SetMem.S      | 5 +++++
 5 files changed, 9 insertions(+)

diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CompareGuid.S b/MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CompareGuid.S
index b7a566fdafacc8a6..7f058e94b3b7023a 100644
--- a/MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CompareGuid.S
+++ b/MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CompareGuid.S
@@ -8,6 +8,7 @@
     .align  5
 ASM_GLOBAL ASM_PFX(InternalMemCompareGuid)
 ASM_PFX(InternalMemCompareGuid):
+    AARCH64_BTI(c)
     mov     x2, xzr
     ldp     x3, x4, [x0]
     cbz     x1, 0f
diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CompareMem.S b/MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CompareMem.S
index ffe4b7a0b058cc85..707e06b0505a19cd 100644
--- a/MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CompareMem.S
+++ b/MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CompareMem.S
@@ -32,6 +32,7 @@
     .p2align 6
 ASM_GLOBAL ASM_PFX(InternalMemCompareMem)
 ASM_PFX(InternalMemCompareMem):
+    AARCH64_BTI(c)
     eor     tmp1, src1, src2
     tst     tmp1, #7
     b.ne    .Lmisaligned8
diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CopyMem.S b/MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CopyMem.S
index 9fad6d1f267c1bcf..59a6593d96cae907 100644
--- a/MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CopyMem.S
+++ b/MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CopyMem.S
@@ -178,6 +178,7 @@ L(copy_long):
 
 ASM_GLOBAL ASM_PFX(InternalMemCopyMem)
 ASM_PFX(InternalMemCopyMem):
+    AARCH64_BTI(c)
     sub     tmp2, dstin, src
     cmp     count, 96
     ccmp    tmp2, count, 2, hi
diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/AArch64/ScanMem.S b/MdePkg/Library/BaseMemoryLibOptDxe/AArch64/ScanMem.S
index 8673b76eca857b8e..55aaf89f56b43fad 100644
--- a/MdePkg/Library/BaseMemoryLibOptDxe/AArch64/ScanMem.S
+++ b/MdePkg/Library/BaseMemoryLibOptDxe/AArch64/ScanMem.S
@@ -45,6 +45,7 @@
 
 ASM_GLOBAL ASM_PFX(InternalMemScanMem8)
 ASM_PFX(InternalMemScanMem8):
+    AARCH64_BTI(c)
     // Do not dereference srcin if no bytes to compare.
     cbz  cntin, .Lzero_length
     //
diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/AArch64/SetMem.S b/MdePkg/Library/BaseMemoryLibOptDxe/AArch64/SetMem.S
index f9748405592057f0..b5618bf09d8bae2f 100644
--- a/MdePkg/Library/BaseMemoryLibOptDxe/AArch64/SetMem.S
+++ b/MdePkg/Library/BaseMemoryLibOptDxe/AArch64/SetMem.S
@@ -29,29 +29,34 @@
 
 ASM_GLOBAL ASM_PFX(InternalMemSetMem16)
 ASM_PFX(InternalMemSetMem16):
+    AARCH64_BTI(c)
     dup     v0.8H, valw
     lsl     count, count, #1
     b       0f
 
 ASM_GLOBAL ASM_PFX(InternalMemSetMem32)
 ASM_PFX(InternalMemSetMem32):
+    AARCH64_BTI(c)
     dup     v0.4S, valw
     lsl     count, count, #2
     b       0f
 
 ASM_GLOBAL ASM_PFX(InternalMemSetMem64)
 ASM_PFX(InternalMemSetMem64):
+    AARCH64_BTI(c)
     dup     v0.2D, val
     lsl     count, count, #3
     b       0f
 
 ASM_GLOBAL ASM_PFX(InternalMemZeroMem)
 ASM_PFX(InternalMemZeroMem):
+    AARCH64_BTI(c)
     movi    v0.16B, #0
     b       0f
 
 ASM_GLOBAL ASM_PFX(InternalMemSetMem)
 ASM_PFX(InternalMemSetMem):
+    AARCH64_BTI(c)
     dup     v0.16B, valw
 0:  add     dstend, dstin, count
     mov     val, v0.D[0]
-- 
2.39.2

[PATCH v2 07/17] MdePkg/BaseSynchronizationLib AARCH64: Make asm files BTI compatible

[Ard Biesheuvel]

Add the BTI instructions and the associated note to make the AArch64 asm
objects compatible with BTI enforcement.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdePkg/Library/BaseSynchronizationLib/AArch64/Synchronization.S | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/MdePkg/Library/BaseSynchronizationLib/AArch64/Synchronization.S b/MdePkg/Library/BaseSynchronizationLib/AArch64/Synchronization.S
index 1a0eb971a6c92d4c..dfcfc80d0cbeff5e 100644
--- a/MdePkg/Library/BaseSynchronizationLib/AArch64/Synchronization.S
+++ b/MdePkg/Library/BaseSynchronizationLib/AArch64/Synchronization.S
@@ -41,6 +41,7 @@ GCC_ASM_EXPORT(InternalSyncDecrement)
 //  IN      UINT16                    ExchangeValue
 //  )
 ASM_PFX(InternalSyncCompareExchange16):
+  AARCH64_BTI(c)
   uxth    w1, w1
   uxth    w2, w2
   dmb     sy
@@ -84,6 +85,7 @@ InternalSyncCompareExchange16Fail:
 //  IN      UINT32                    ExchangeValue
 //  )
 ASM_PFX(InternalSyncCompareExchange32):
+  AARCH64_BTI(c)
   dmb     sy
 
 InternalSyncCompareExchange32Again:
@@ -124,6 +126,7 @@ InternalSyncCompareExchange32Fail:
 //  IN      UINT64                    ExchangeValue
 //  )
 ASM_PFX(InternalSyncCompareExchange64):
+  AARCH64_BTI(c)
   dmb     sy
 
 InternalSyncCompareExchange64Again:
@@ -159,6 +162,7 @@ InternalSyncCompareExchange64Fail:
 //  IN      volatile UINT32           *Value
 //  )
 ASM_PFX(InternalSyncIncrement):
+  AARCH64_BTI(c)
   dmb     sy
 TryInternalSyncIncrement:
   ldxr    w1, [x0]
@@ -188,6 +192,7 @@ TryInternalSyncIncrement:
 //  IN      volatile UINT32           *Value
 //  )
 ASM_PFX(InternalSyncDecrement):
+  AARCH64_BTI(c)
   dmb     sy
 TryInternalSyncDecrement:
   ldxr    w1, [x0]
-- 
2.39.2

[PATCH v2 08/17] MdePkg/BaseRngLib AARCH64: Make asm files BTI compatible

[Ard Biesheuvel]

Add the BTI instructions and the associated note to make the AArch64 asm
objects compatible with BTI enforcement.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.S | 3 +--
 MdePkg/Library/BaseRngLib/AArch64/ArmRng.S         | 1 +
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.S b/MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.S
index 82a00d36221278e0..d30b63fe5c68c565 100644
--- a/MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.S
+++ b/MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.S
@@ -25,7 +25,6 @@ GCC_ASM_EXPORT(ArmReadIdIsar0)
 #  );
 #
 ASM_PFX(ArmReadIdIsar0):
+  AARCH64_BTI(c)
   mrs  x0, id_aa64isar0_el1 // Read ID_AA64ISAR0 Register
   ret
-
-
diff --git a/MdePkg/Library/BaseRngLib/AArch64/ArmRng.S b/MdePkg/Library/BaseRngLib/AArch64/ArmRng.S
index 5159f467e3a6cd6e..27a847b996fd1d2a 100644
--- a/MdePkg/Library/BaseRngLib/AArch64/ArmRng.S
+++ b/MdePkg/Library/BaseRngLib/AArch64/ArmRng.S
@@ -31,6 +31,7 @@ GCC_ASM_EXPORT(ArmRndr)
 #  );
 #
 ASM_PFX(ArmRndr):
+  AARCH64_BTI(c)
   mrs  x1, RNDR
   str  x1, [x0]
   cset x0, ne    // RNDR sets NZCV to 0b0100 on failure
-- 
2.39.2

[PATCH v2 09/17] ArmPkg: Emit BTI opcodes when BTI codegen is enabled

[Ard Biesheuvel]

When building with -mbranch-protection=bti, which affects the compiler
codegen only, ensure that the assembler based codegen is aligned with
this, by emitting the BTI C opcode at the start of each exported
function. While most exported functions are not in fact ever called
indirectly, whether or not this is the case is a property of the caller
so annotating every exported function is a reasonable default.

While at it, fix two occurrences in ArmPkg of exported functions that
did not use the ASM_FUNC() macro.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/Include/AsmMacroIoLibV8.h                          | 3 ++-
 ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S | 3 +--
 ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S                 | 4 +---
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/ArmPkg/Include/AsmMacroIoLibV8.h b/ArmPkg/Include/AsmMacroIoLibV8.h
index 2c2b1cabd0537671..135aaeca5d0b986d 100644
--- a/ArmPkg/Include/AsmMacroIoLibV8.h
+++ b/ArmPkg/Include/AsmMacroIoLibV8.h
@@ -38,7 +38,8 @@
   .global   Name                  ; \
   .section  #Section, "ax"        ; \
   .type     Name, %function       ; \
-  Name:
+  Name:                           ; \
+  AARCH64_BTI(c)
 
 #define ASM_FUNC(Name)  _ASM_FUNC(ASM_PFX(Name), .text. ## Name)
 
diff --git a/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S b/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S
index 9202952ee9c0d4e5..cd9437b6aab82889 100644
--- a/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S
+++ b/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S
@@ -94,7 +94,6 @@
 
 GCC_ASM_EXPORT(ExceptionHandlersEnd)
 GCC_ASM_EXPORT(CommonCExceptionHandler)
-GCC_ASM_EXPORT(RegisterEl0Stack)
 
 .text
 
@@ -387,6 +386,6 @@ ASM_PFX(CommonExceptionEntry):
 
   eret
 
-ASM_PFX(RegisterEl0Stack):
+ASM_FUNC(RegisterEl0Stack)
   msr     sp_el0, x0
   ret
diff --git a/ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S b/ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S
index 1a7c10cb793183e7..ab13914fd3e7a359 100644
--- a/ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S
+++ b/ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S
@@ -8,9 +8,7 @@
 .text
 .align 3
 
-GCC_ASM_EXPORT(ArmCallSvc)
-
-ASM_PFX(ArmCallSvc):
+ASM_FUNC(ArmCallSvc)
   // Push frame pointer and return address on the stack
   stp   x29, x30, [sp, #-32]!
   mov   x29, sp
-- 
2.39.2

[PATCH v2 10/17] ArmPkg/GccLto AARCH64: Add BTI note to LTO helper library

[Ard Biesheuvel]

The GccLto helper library does not contain any code, as its only purpose
is to pull in other libraries that implement intrinsics to which the
linker's codegen pass may emit calls.

So mark it as BTI compatible, so that the linker does not complain about
unannotated objects.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/Library/GccLto/liblto-aarch64.a | Bin 1016 -> 1128 bytes
 1 file changed, 0 insertions(+), 0 deletions(-)

diff --git a/ArmPkg/Library/GccLto/liblto-aarch64.a b/ArmPkg/Library/GccLto/liblto-aarch64.a
index 2ab00238f0dad882abf08a1fb9623c9cdea9f17b..6ca3932f1cf30e389ab4c9bdbb77c7db729f1f14 100644
GIT binary patch
delta 168
zcmeyt{(@tIhNA`(0~|PjSq==G3><JV7=s1K5CCFUAa?f)1qmMji7`%eRNk1=$|$6l
zmtT^qm!4OuS5TB+kXlqyIr$@_>tqY22*!rV6PcEC8Uj_b12NO&gG}O#3KJKKPR?Ui
hVl0?Ek=dTnWAa62^~n~@5}Xehfog!j1+0LH3joADAhrMi

delta 90
zcmaFC@q>MWhWHOg1~_m4vm6*W7+5D8GU;yYX=R*j#AG(vfk|+322%uM!sLTY%O`8F
gNN^s2>N1>oP;~M<W~E8Y9FtElTQF)&W@J$Z0B}JRI{*Lx

-- 
2.39.2

[PATCH v2 11/17] ArmPkg, BaseTools AARCH64: Add BTI ELF note to .hii objects

[Ard Biesheuvel]

The ELF based toolchains use objcopy to create HII object files, which
contain only a single .hii section. This means no GNU note is inserted
that describes the object as compatible with BTI, even though the lack
of executable code in such an object makes the distinction irrelevant.
However, the linker will not add the note globally to the resulting ELF
executable, and this breaks BTI compatibility.

So let's insert a GNU BTI-compatible ELF note by hand when generating
such object files.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/Library/GnuNoteBti.bin     | Bin 0 -> 32 bytes
 BaseTools/Conf/tools_def.template |   4 ++--
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ArmPkg/Library/GnuNoteBti.bin b/ArmPkg/Library/GnuNoteBti.bin
new file mode 100644
index 0000000000000000000000000000000000000000..339567b4e89943c610b44767ddad5f631229ed3b
GIT binary patch
literal 32
dcmZQ!U|<jcVpbq__X`D*3<p?%1S5zA1OOf&0m%RW

literal 0
HcmV?d00001

diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
index 471eb67c0c839730..ed6050aa96157cb9 100755
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -2400,7 +2400,7 @@ RELEASE_GCC5_ARM_DLINK_FLAGS     = DEF(GCC5_ARM_DLINK_FLAGS) -flto -Os -L$(WORKS
 *_GCC5_AARCH64_DTCPP_FLAGS       = DEF(GCC_DTCPP_FLAGS)
 *_GCC5_AARCH64_PLATFORM_FLAGS    =
 *_GCC5_AARCH64_PP_FLAGS          = $(PLATFORM_FLAGS) DEF(GCC_PP_FLAGS)
-*_GCC5_AARCH64_RC_FLAGS          = DEF(GCC_AARCH64_RC_FLAGS)
+*_GCC5_AARCH64_RC_FLAGS          = DEF(GCC_AARCH64_RC_FLAGS) --add-section .note.gnu.property=$(WORKSPACE)/ArmPkg/Library/GnuNoteBti.bin --set-section-flags .note.gnu.property=alloc,readonly
 *_GCC5_AARCH64_VFRPP_FLAGS       = $(PLATFORM_FLAGS) DEF(GCC_VFRPP_FLAGS)
 *_GCC5_AARCH64_CC_XIPFLAGS       = DEF(GCC5_AARCH64_CC_XIPFLAGS)
 
@@ -2735,7 +2735,7 @@ DEFINE CLANG38_AARCH64_DLINK_FLAGS  = DEF(CLANG38_AARCH64_TARGET) DEF(GCC_AARCH6
 *_CLANG38_AARCH64_DLINK2_FLAGS   = DEF(GCC_DLINK2_FLAGS_COMMON) -Wl,--defsym=PECOFF_HEADER_SIZE=0x228
 *_CLANG38_AARCH64_PLATFORM_FLAGS =
 *_CLANG38_AARCH64_PP_FLAGS       = DEF(GCC_PP_FLAGS) DEF(CLANG38_AARCH64_TARGET) $(PLATFORM_FLAGS)
-*_CLANG38_AARCH64_RC_FLAGS       = DEF(GCC_AARCH64_RC_FLAGS)
+*_CLANG38_AARCH64_RC_FLAGS       = DEF(GCC_AARCH64_RC_FLAGS) --add-section .note.gnu.property=$(WORKSPACE)/ArmPkg/Library/GnuNoteBti.bin --set-section-flags .note.gnu.property=alloc,readonly
 *_CLANG38_AARCH64_VFRPP_FLAGS    = DEF(GCC_VFRPP_FLAGS) DEF(CLANG38_AARCH64_TARGET) $(PLATFORM_FLAGS)
 *_CLANG38_AARCH64_ASLPP_FLAGS    = DEF(GCC_ASLPP_FLAGS) DEF(CLANG38_AARCH64_TARGET)
 *_CLANG38_AARCH64_CC_XIPFLAGS    = DEF(GCC_AARCH64_CC_XIPFLAGS)
-- 
2.39.2

[PATCH v2 12/17] ArmPlatformPkg/PrePeiCore: Make vector table object BTI compatible

[Ard Biesheuvel]

The object file containing the vector table does not contain any
callable functions, so it will not be implicitly annotated as BTI
compatible on BTI builds. So add the annotation by hand, and use the
'empty' type so we get the GNU ELF note but not the actual BTI opcode.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPlatformPkg/PrePeiCore/AArch64/Exception.S | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ArmPlatformPkg/PrePeiCore/AArch64/Exception.S b/ArmPlatformPkg/PrePeiCore/AArch64/Exception.S
index 43e40f97c3eed5ff..5a84fefe3fb55216 100644
--- a/ArmPlatformPkg/PrePeiCore/AArch64/Exception.S
+++ b/ArmPlatformPkg/PrePeiCore/AArch64/Exception.S
@@ -112,3 +112,5 @@ _DefaultSError_LowerA32:
   TO_HANDLER
 
 VECTOR_END(PeiVectorTable)
+
+AARCH64_BTI()
-- 
2.39.2

[PATCH v2 13/17] BaseTools/GenFw: Parse IBT/BTI support status from ELF note

[Ard Biesheuvel]

When performing ELF to PE/COFF conversion, parse any notes sections to
decide whether the image supports forward CFI landing pads. This will be
used to set the associated DllCharacteristicsEx flag in a subsequent
patch.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 BaseTools/Source/C/GenFw/Elf64Convert.c | 50 ++++++++++++++++++++
 BaseTools/Source/C/GenFw/elf_common.h   |  9 ++++
 2 files changed, 59 insertions(+)

diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c b/BaseTools/Source/C/GenFw/Elf64Convert.c
index 8b50774beb1eebda..2a810e835d4a4a66 100644
--- a/BaseTools/Source/C/GenFw/Elf64Convert.c
+++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
@@ -770,6 +770,49 @@ WriteSectionRiscV64 (
   }
 }
 
+STATIC UINT16 mDllCharacteristicsEx;
+
+STATIC
+VOID
+ParseNoteSection (
+  CONST Elf_Shdr  *Shdr
+  )
+{
+  CONST Elf_Note *Note;
+  CONST UINT32   *Prop;
+  UINT32         Prop0;
+  UINT32         Prop2;
+
+  Note = (Elf_Note *)((UINT8 *)mEhdr + Shdr->sh_offset);
+
+  if ((Note->n_type == NT_GNU_PROPERTY_TYPE_0) &&
+      (Note->n_namesz == sizeof ("GNU")) &&
+      (strcmp ((CHAR8 *)(Note + 1), "GNU") == 0) &&
+      (Note->n_descsz > sizeof (UINT32[2]))) {
+    Prop = (UINT32 *)((UINT8 *)(Note + 1) + sizeof("GNU"));
+
+    switch (mEhdr->e_machine) {
+    case EM_AARCH64:
+      Prop0 = GNU_PROPERTY_AARCH64_FEATURE_1_AND;
+      Prop2 = GNU_PROPERTY_AARCH64_FEATURE_1_BTI;
+      break;
+
+    case EM_X86_64:
+      Prop0 = GNU_PROPERTY_X86_FEATURE_1_AND;
+      Prop2 = GNU_PROPERTY_X86_FEATURE_1_IBT;
+      break;
+
+    default:
+      return;
+    }
+    if ((Prop[0] == Prop0) &&
+        (Prop[1] >= sizeof (UINT32)) &&
+        ((Prop[2] & Prop2) != 0)) {
+      mDllCharacteristicsEx |= EFI_IMAGE_DLLCHARACTERISTICS_EX_FORWARD_CFI_COMPAT;
+    }
+  }
+}
+
 //
 // Elf functions interface implementation
 //
@@ -826,6 +869,13 @@ ScanSections64 (
     }
   }
 
+  for (i = 0; i < mEhdr->e_shnum; i++) {
+    Elf_Shdr *shdr = GetShdrByIndex(i);
+    if (shdr->sh_type == SHT_NOTE) {
+      ParseNoteSection (shdr);
+    }
+  }
+
   //
   // Check if mCoffAlignment is larger than MAX_COFF_ALIGNMENT
   //
diff --git a/BaseTools/Source/C/GenFw/elf_common.h b/BaseTools/Source/C/GenFw/elf_common.h
index 7b7fdeb3290dfa88..ccd32804b090a226 100644
--- a/BaseTools/Source/C/GenFw/elf_common.h
+++ b/BaseTools/Source/C/GenFw/elf_common.h
@@ -59,6 +59,15 @@ typedef struct {
   UINT32  n_type;    /* Type of this note. */
 } Elf_Note;
 
+#define NT_GNU_PROPERTY_TYPE_0  5
+
+#define GNU_PROPERTY_X86_FEATURE_1_AND  0xc0000002
+#define GNU_PROPERTY_X86_FEATURE_1_IBT  0x1
+
+#define GNU_PROPERTY_AARCH64_FEATURE_1_AND  0xc0000000
+#define GNU_PROPERTY_AARCH64_FEATURE_1_BTI  0x1
+#define GNU_PROPERTY_AARCH64_FEATURE_1_PAC  0x2
+
 /* Indexes into the e_ident array.  Keep synced with
    http://www.sco.com/developers/gabi/latest/ch4.eheader.html */
 #define EI_MAG0    0  /* Magic number, byte 0. */
-- 
2.39.2

[PATCH v2 14/17] BaseTools/GenFw: Add DllCharacteristicsEx field to debug data

[Ard Biesheuvel]

The PE/COFF spec describes an additional DllCharacteristics field
implemented as a debug directory entry, which carries flags related to
which control flow integrity (CFI) features are supported by the binary.

So let's add this entry when doing ELF to PE/COFF conversion - we will
add support for setting the flags in a subsequent patch.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 BaseTools/Source/C/GenFw/Elf64Convert.c               | 54 +++++++++++++++-----
 BaseTools/Source/C/GenFw/GenFw.c                      |  3 +-
 BaseTools/Source/C/Include/IndustryStandard/PeImage.h | 13 ++++-
 3 files changed, 55 insertions(+), 15 deletions(-)

diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c b/BaseTools/Source/C/GenFw/Elf64Convert.c
index 2a810e835d4a4a66..9c17c90b16602421 100644
--- a/BaseTools/Source/C/GenFw/Elf64Convert.c
+++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
@@ -992,6 +992,16 @@ ScanSections64 (
                 sizeof(EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY) +
                 strlen(mInImageName) + 1;
 
+  //
+  // Add more space in the .debug data region for the DllCharacteristicsEx
+  // field.
+  //
+  if (mDllCharacteristicsEx != 0) {
+    mCoffOffset = DebugRvaAlign(mCoffOffset) +
+                  sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY) +
+                  sizeof (EFI_IMAGE_DEBUG_EX_DLLCHARACTERISTICS_ENTRY);
+  }
+
   mCoffOffset = CoffAlign(mCoffOffset);
   if (SectionCount == 0) {
     mDataOffset = mCoffOffset;
@@ -2244,29 +2254,47 @@ WriteDebug64 (
   VOID
   )
 {
-  UINT32                              Len;
-  EFI_IMAGE_OPTIONAL_HEADER_UNION     *NtHdr;
-  EFI_IMAGE_DATA_DIRECTORY            *DataDir;
-  EFI_IMAGE_DEBUG_DIRECTORY_ENTRY     *Dir;
-  EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY *Nb10;
+  UINT32                                      Len;
+  EFI_IMAGE_OPTIONAL_HEADER_UNION             *NtHdr;
+  EFI_IMAGE_DATA_DIRECTORY                    *DataDir;
+  EFI_IMAGE_DEBUG_DIRECTORY_ENTRY             *Dir;
+  EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY         *Nb10;
+  EFI_IMAGE_DEBUG_EX_DLLCHARACTERISTICS_ENTRY *DllEntry;
 
   Len = strlen(mInImageName) + 1;
 
+  NtHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)(mCoffFile + mNtHdrOffset);
+  DataDir = &NtHdr->Pe32Plus.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG];
+  DataDir->VirtualAddress = mDebugOffset;
+  DataDir->Size = sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
+
   Dir = (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY*)(mCoffFile + mDebugOffset);
+
+  if (mDllCharacteristicsEx != 0) {
+    DataDir->Size  += sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
+
+    Dir->Type       = EFI_IMAGE_DEBUG_TYPE_EX_DLLCHARACTERISTICS;
+    Dir->SizeOfData = sizeof (EFI_IMAGE_DEBUG_EX_DLLCHARACTERISTICS_ENTRY);
+    Dir->FileOffset = mDebugOffset + DataDir->Size +
+                      sizeof (EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY) +
+                      DebugRvaAlign(Len);
+    Dir->RVA        = Dir->FileOffset;
+
+    DllEntry = (VOID *)(mCoffFile + Dir->FileOffset);
+
+    DllEntry->DllCharacteristicsEx = mDllCharacteristicsEx;
+
+    Dir++;
+  }
+
   Dir->Type = EFI_IMAGE_DEBUG_TYPE_CODEVIEW;
   Dir->SizeOfData = sizeof(EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY) + Len;
-  Dir->RVA = mDebugOffset + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
-  Dir->FileOffset = mDebugOffset + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
+  Dir->RVA = mDebugOffset + DataDir->Size;
+  Dir->FileOffset = mDebugOffset + DataDir->Size;
 
   Nb10 = (EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY*)(Dir + 1);
   Nb10->Signature = CODEVIEW_SIGNATURE_NB10;
   strcpy ((char *)(Nb10 + 1), mInImageName);
-
-
-  NtHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)(mCoffFile + mNtHdrOffset);
-  DataDir = &NtHdr->Pe32Plus.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG];
-  DataDir->VirtualAddress = mDebugOffset;
-  DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
 }
 
 STATIC
diff --git a/BaseTools/Source/C/GenFw/GenFw.c b/BaseTools/Source/C/GenFw/GenFw.c
index 6f61f16788cd2a0a..d0e52ccc26431380 100644
--- a/BaseTools/Source/C/GenFw/GenFw.c
+++ b/BaseTools/Source/C/GenFw/GenFw.c
@@ -2932,7 +2932,8 @@ Routine Description:
       if (mIsConvertXip) {
         DebugEntry->FileOffset = DebugEntry->RVA;
       }
-      if (ZeroDebugFlag || DebugEntry->Type != EFI_IMAGE_DEBUG_TYPE_CODEVIEW) {
+      if ((ZeroDebugFlag || DebugEntry->Type != EFI_IMAGE_DEBUG_TYPE_CODEVIEW) &&
+          (DebugEntry->Type != EFI_IMAGE_DEBUG_TYPE_EX_DLLCHARACTERISTICS)) {
         memset (FileBuffer + DebugEntry->FileOffset, 0, DebugEntry->SizeOfData);
         memset (DebugEntry, 0, sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY));
       }
diff --git a/BaseTools/Source/C/Include/IndustryStandard/PeImage.h b/BaseTools/Source/C/Include/IndustryStandard/PeImage.h
index 77ded3f611398278..5e9428ab98c7f68a 100644
--- a/BaseTools/Source/C/Include/IndustryStandard/PeImage.h
+++ b/BaseTools/Source/C/Include/IndustryStandard/PeImage.h
@@ -615,7 +615,8 @@ typedef struct {
 ///
 /// Debug Format
 ///
-#define EFI_IMAGE_DEBUG_TYPE_CODEVIEW 2
+#define EFI_IMAGE_DEBUG_TYPE_CODEVIEW               2
+#define EFI_IMAGE_DEBUG_TYPE_EX_DLLCHARACTERISTICS  20
 
 typedef struct {
   UINT32  Characteristics;
@@ -664,6 +665,16 @@ typedef struct {
   //
 } EFI_IMAGE_DEBUG_CODEVIEW_MTOC_ENTRY;
 
+///
+/// Extended DLL Characteristics
+///
+#define EFI_IMAGE_DLLCHARACTERISTICS_EX_CET_COMPAT          0x0001
+#define EFI_IMAGE_DLLCHARACTERISTICS_EX_FORWARD_CFI_COMPAT  0x0040
+
+typedef struct {
+  UINT16  DllCharacteristicsEx;
+} EFI_IMAGE_DEBUG_EX_DLLCHARACTERISTICS_ENTRY;
+
 //
 // .pdata entries for X64
 //
-- 
2.39.2

[PATCH v2 15/17] 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.

Link: https://uefi.org/specs/UEFI/2.10/04_EFI_System_Table.html#efi-memory-attributes-table
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
Acked-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
 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 82f83a67b96d38c5..238c14ff92dfed31 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.2

[PATCH v2 16/17] MdePkg/PeCoffLib: Capture DLL characteristics fieldis in image context

[Ard Biesheuvel]

When loading a PE/COFF image, capture the DLL characteristics fields of
the header into our image context structure so we can refer to them when
mapping the image.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdePkg/Include/IndustryStandard/PeImage.h | 13 +++++-
 MdePkg/Include/Library/PeCoffLib.h        |  6 +++
 MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 46 +++++++++++++++-----
 3 files changed, 53 insertions(+), 12 deletions(-)

diff --git a/MdePkg/Include/IndustryStandard/PeImage.h b/MdePkg/Include/IndustryStandard/PeImage.h
index dd4cc25483bc4bcf..a5b9b848ed391f98 100644
--- a/MdePkg/Include/IndustryStandard/PeImage.h
+++ b/MdePkg/Include/IndustryStandard/PeImage.h
@@ -625,7 +625,8 @@ typedef struct {
   UINT32    FileOffset;  ///< The file pointer to the debug data.
 } EFI_IMAGE_DEBUG_DIRECTORY_ENTRY;
 
-#define EFI_IMAGE_DEBUG_TYPE_CODEVIEW  2    ///< The Visual C++ debug information.
+#define EFI_IMAGE_DEBUG_TYPE_CODEVIEW               2    ///< The Visual C++ debug information.
+#define EFI_IMAGE_DEBUG_TYPE_EX_DLLCHARACTERISTICS  20
 
 ///
 /// Debug Data Structure defined in Microsoft C++.
@@ -669,6 +670,16 @@ typedef struct {
   //
 } EFI_IMAGE_DEBUG_CODEVIEW_MTOC_ENTRY;
 
+///
+/// Extended DLL Characteristics
+///
+#define EFI_IMAGE_DLLCHARACTERISTICS_EX_CET_COMPAT          0x0001
+#define EFI_IMAGE_DLLCHARACTERISTICS_EX_FORWARD_CFI_COMPAT  0x0040
+
+typedef struct {
+  UINT16    DllCharacteristicsEx;
+} EFI_IMAGE_DEBUG_EX_DLLCHARACTERISTICS_ENTRY;
+
 ///
 /// Resource format.
 ///
diff --git a/MdePkg/Include/Library/PeCoffLib.h b/MdePkg/Include/Library/PeCoffLib.h
index b45879453785c77d..d21c932076c072f6 100644
--- a/MdePkg/Include/Library/PeCoffLib.h
+++ b/MdePkg/Include/Library/PeCoffLib.h
@@ -171,6 +171,12 @@ typedef struct {
   ///
   UINT16                      ImageType;
   ///
+  /// Set by PeCoffLoaderGetImageInfo() to the DLL flags stored in the PE/COFF header and
+  /// in the DllCharacteristicsEx debug table.
+  ///
+  UINT16                      DllCharacteristics;
+  UINT16                      DllCharacteristicsEx;
+  ///
   /// Set by PeCoffLoaderGetImageInfo() to TRUE if the PE/COFF image does not contain
   /// relocation information.
   ///
diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
index 97a8aaf8c73d3e3c..4b71176a0c7c2ed0 100644
--- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
+++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
@@ -308,10 +308,11 @@ PeCoffLoaderGetPeHeader (
       //
       // Use PE32 offset
       //
-      ImageContext->ImageType        = Hdr.Pe32->OptionalHeader.Subsystem;
-      ImageContext->ImageSize        = (UINT64)Hdr.Pe32->OptionalHeader.SizeOfImage;
-      ImageContext->SectionAlignment = Hdr.Pe32->OptionalHeader.SectionAlignment;
-      ImageContext->SizeOfHeaders    = Hdr.Pe32->OptionalHeader.SizeOfHeaders;
+      ImageContext->ImageType          = Hdr.Pe32->OptionalHeader.Subsystem;
+      ImageContext->ImageSize          = (UINT64)Hdr.Pe32->OptionalHeader.SizeOfImage;
+      ImageContext->SectionAlignment   = Hdr.Pe32->OptionalHeader.SectionAlignment;
+      ImageContext->SizeOfHeaders      = Hdr.Pe32->OptionalHeader.SizeOfHeaders;
+      ImageContext->DllCharacteristics = Hdr.Pe32->OptionalHeader.DllCharacteristics;
     } else if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
       //
       // 1. Check FileHeader.NumberOfRvaAndSizes filed.
@@ -429,10 +430,11 @@ PeCoffLoaderGetPeHeader (
       //
       // Use PE32+ offset
       //
-      ImageContext->ImageType        = Hdr.Pe32Plus->OptionalHeader.Subsystem;
-      ImageContext->ImageSize        = (UINT64)Hdr.Pe32Plus->OptionalHeader.SizeOfImage;
-      ImageContext->SectionAlignment = Hdr.Pe32Plus->OptionalHeader.SectionAlignment;
-      ImageContext->SizeOfHeaders    = Hdr.Pe32Plus->OptionalHeader.SizeOfHeaders;
+      ImageContext->ImageType          = Hdr.Pe32Plus->OptionalHeader.Subsystem;
+      ImageContext->ImageSize          = (UINT64)Hdr.Pe32Plus->OptionalHeader.SizeOfImage;
+      ImageContext->SectionAlignment   = Hdr.Pe32Plus->OptionalHeader.SectionAlignment;
+      ImageContext->SizeOfHeaders      = Hdr.Pe32Plus->OptionalHeader.SizeOfHeaders;
+      ImageContext->DllCharacteristics = Hdr.Pe32Plus->OptionalHeader.DllCharacteristics;
     } else {
       ImageContext->ImageError = IMAGE_ERROR_INVALID_MACHINE_TYPE;
       return RETURN_UNSUPPORTED;
@@ -545,8 +547,9 @@ PeCoffLoaderGetPeHeader (
   Retrieves information about a PE/COFF image.
 
   Computes the PeCoffHeaderOffset, IsTeImage, ImageType, ImageAddress, ImageSize,
-  DestinationAddress, RelocationsStripped, SectionAlignment, SizeOfHeaders, and
-  DebugDirectoryEntryRva fields of the ImageContext structure.
+  DestinationAddress, RelocationsStripped, SectionAlignment, SizeOfHeaders,
+  DllCharacteristics, DllCharacteristicsEx and DebugDirectoryEntryRva fields of
+  the ImageContext structure.
   If ImageContext is NULL, then return RETURN_INVALID_PARAMETER.
   If the PE/COFF image accessed through the ImageRead service in the ImageContext
   structure is not a supported PE/COFF image type, then return RETURN_UNSUPPORTED.
@@ -752,7 +755,28 @@ PeCoffLoaderGetImageInfo (
               ImageContext->ImageSize += DebugEntry.SizeOfData;
             }
 
-            return RETURN_SUCCESS;
+            continue;
+          }
+
+          if (DebugEntry.Type == EFI_IMAGE_DEBUG_TYPE_EX_DLLCHARACTERISTICS) {
+            Size     = sizeof (EFI_IMAGE_DEBUG_EX_DLLCHARACTERISTICS_ENTRY);
+            ReadSize = sizeof (EFI_IMAGE_DEBUG_EX_DLLCHARACTERISTICS_ENTRY);
+            Status   = ImageContext->ImageRead (
+                                       ImageContext->Handle,
+                                       DebugEntry.FileOffset,
+                                       &Size,
+                                       &ImageContext->DllCharacteristicsEx
+                                       );
+            if (RETURN_ERROR (Status) || (Size != ReadSize)) {
+              ImageContext->ImageError = IMAGE_ERROR_IMAGE_READ;
+              if (Size != ReadSize) {
+                Status = RETURN_UNSUPPORTED;
+              }
+
+              return Status;
+            }
+
+            continue;
           }
         }
       }
-- 
2.39.2

[PATCH v2 17/17] 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.

Given that the PE/COFF spec now defines a DllCharacteristicsEx flag that
indicates whether or not a loaded image is compatible with this, we can
wire this up to the flag in the memory attributes table, and set it if
all loaded runtime image are compatible with it.

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

diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
index 815a6b4bd844a452..43daa037be441150 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/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
index 8704ebea9a7c88c0..9dbfb2a1fad22ced 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -1399,6 +1399,16 @@ CoreLoadImageCommon (
     CoreNewDebugImageInfoEntry (EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL, &Image->Info, Image->Handle);
   }
 
+  //
+  // Check whether we are loading a runtime image that lacks support for
+  // IBT/BTI landing pads.
+  //
+  if ((Image->ImageContext.ImageCodeMemoryType == EfiRuntimeServicesCode) &&
+      ((Image->ImageContext.DllCharacteristicsEx & EFI_IMAGE_DLLCHARACTERISTICS_EX_FORWARD_CFI_COMPAT) == 0))
+  {
+    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 e079213711875f89..fd127ee167e1ac9a 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  = TRUE;
 
 /**
   Install MemoryAttributesTable.
@@ -182,7 +183,12 @@ 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));
-- 
2.39.2

Re: [PATCH v2 00/17] Enable BTI support in memory attributes table

[Leif Lindholm]

On Mon, Mar 27, 2023 at 13:00:55 +0200, Ard Biesheuvel wrote:
> Implement version 2 of the memory attributes table, which now contains a
> flag informing the OS whether or not code regions may be mapped with CFI
> mitigations such as IBT or BTI enabled.
> 
> This series covers roughly the following parts:
> 
> - (AARCH64) Annotate ELF objects generated from asm as BTI compatible
>   when BTI codegen is enabled
> - Update the BaseTools to emit the appropriate PE/COFF annotation when a
>   BTI/IBT compatible ELF executable is converted to PE/COFF
> - Take this PE/COFF annotation into account when populating the memory
>   attributes table in the DXE core
> 
> TODO:
> - X64 changes to make the code IBT compatible and emit the ELF note
> - Figure out how to generate such executables with native PE toolchains
> - Implement BTI/IBT enforcement at boot time - this is something I
>   intend to look into next.
> 
> Can be tested with the CLANG38 toolchain (both Clang compiler and LLD
> linker, version 3.8 or newer) with the following build options.
> 
> [BuildOptions]
>   GCC:*_*_AARCH64_PP_FLAGS = -mbranch-protection=bti
>   GCC:*_*_AARCH64_CC_FLAGS = -mbranch-protection=bti

I know you point out to use CLANG38, but the set is also tagged PATCH
rather than RFC.
I believe this option was added to GCC in version 9, meaning this is a
breaking change for GCC8. Now, GCC8 is ancient, but I expect it's
still what's available in RHEL8 for example. So it's worth mentioning.

/
    Leif

>   GCC:*_*_AARCH64_DLINK_FLAGS = -fuse-ld=lld -Wl,--no-relax,--no-pie,-z,bti-report=error
>
> 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>
> Cc: Marvin H??user <mhaeuser@posteo.de>
> Cc: Bob Feng <bob.c.feng@intel.com>
> 
> Ard Biesheuvel (17):
>   MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note
>   MdePkg/BaseCpuLib AARCH64: Make asm files BTI compatible
>   MdePkg/BaseIoLibIntrinsic AARCH64: Make asm files BTI compatible
>   MdePkg/BaseLib AARCH64: Make LongJump() BTI compatible
>   MdePkg/BaseLib AARCH64: Make asm files BTI compatible
>   MdePkg/BaseMemoryLibOptDxe AARCH64: Make asm files BTI compatible
>   MdePkg/BaseSynchronizationLib AARCH64: Make asm files BTI compatible
>   MdePkg/BaseRngLib AARCH64: Make asm files BTI compatible
>   ArmPkg: Emit BTI opcodes when BTI codegen is enabled
>   ArmPkg/GccLto AARCH64: Add BTI note to LTO helper library
>   ArmPkg, BaseTools AARCH64: Add BTI ELF note to .hii objects
>   ArmPlatformPkg/PrePeiCore: Make vector table object BTI compatible
>   BaseTools/GenFw: Parse IBT/BTI support status from ELF note
>   BaseTools/GenFw: Add DllCharacteristicsEx field to debug data
>   MdePkg: Update MemoryAttributesTable to v2.10
>   MdePkg/PeCoffLib: Capture DLL characteristics fieldis in image context
>   MdeModulePkg: Enable forward edge CFI in mem attributes table
> 
>  ArmPkg/Include/AsmMacroIoLibV8.h                                |   3 +-
>  ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S       |   3 +-
>  ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S                       |   4 +-
>  ArmPkg/Library/GccLto/liblto-aarch64.a                          | Bin 1016 -> 1128 bytes
>  ArmPkg/Library/GnuNoteBti.bin                                   | Bin 0 -> 32 bytes
>  ArmPlatformPkg/PrePeiCore/AArch64/Exception.S                   |   2 +
>  ArmVirtPkg/Library/ArmPlatformLibQemu/IdMap.S                   |   2 +
>  BaseTools/Conf/tools_def.template                               |   4 +-
>  BaseTools/Source/C/GenFw/Elf64Convert.c                         | 104 +++++++++++++++++---
>  BaseTools/Source/C/GenFw/GenFw.c                                |   3 +-
>  BaseTools/Source/C/GenFw/elf_common.h                           |   9 ++
>  BaseTools/Source/C/Include/IndustryStandard/PeImage.h           |  13 ++-
>  MdeModulePkg/Core/Dxe/DxeMain.h                                 |   2 +
>  MdeModulePkg/Core/Dxe/Image/Image.c                             |  10 ++
>  MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c              |   8 +-
>  MdePkg/Include/AArch64/ProcessorBind.h                          |  31 ++++++
>  MdePkg/Include/Guid/MemoryAttributesTable.h                     |   8 +-
>  MdePkg/Include/IndustryStandard/PeImage.h                       |  13 ++-
>  MdePkg/Include/Library/PeCoffLib.h                              |   6 ++
>  MdePkg/Library/BaseCpuLib/AArch64/CpuFlushTlb.S                 |   1 +
>  MdePkg/Library/BaseCpuLib/AArch64/CpuSleep.S                    |   1 +
>  MdePkg/Library/BaseIoLibIntrinsic/AArch64/ArmVirtMmio.S         |   8 ++
>  MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.S                  |   1 +
>  MdePkg/Library/BaseLib/AArch64/DisableInterrupts.S              |   1 +
>  MdePkg/Library/BaseLib/AArch64/EnableInterrupts.S               |   1 +
>  MdePkg/Library/BaseLib/AArch64/GetInterruptsState.S             |   1 +
>  MdePkg/Library/BaseLib/AArch64/MemoryFence.S                    |   1 +
>  MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S                |   5 +-
>  MdePkg/Library/BaseLib/AArch64/SpeculationBarrier.S             |   1 +
>  MdePkg/Library/BaseLib/AArch64/SwitchStack.S                    |   2 +
>  MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CompareGuid.S        |   1 +
>  MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CompareMem.S         |   1 +
>  MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CopyMem.S            |   1 +
>  MdePkg/Library/BaseMemoryLibOptDxe/AArch64/ScanMem.S            |   1 +
>  MdePkg/Library/BaseMemoryLibOptDxe/AArch64/SetMem.S             |   5 +
>  MdePkg/Library/BasePeCoffLib/BasePeCoff.c                       |  46 ++++++---
>  MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.S              |   3 +-
>  MdePkg/Library/BaseRngLib/AArch64/ArmRng.S                      |   1 +
>  MdePkg/Library/BaseSynchronizationLib/AArch64/Synchronization.S |   5 +
>  39 files changed, 270 insertions(+), 42 deletions(-)
>  create mode 100644 ArmPkg/Library/GnuNoteBti.bin
> 
> -- 
> 2.39.2
> 

Re: [PATCH v2 01/17] MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note

[Leif Lindholm]

On Mon, Mar 27, 2023 at 13:00:56 +0200, Ard Biesheuvel wrote:
> Implement a CPP macro that can be called from .S files to emit the .note
> section carrying the annotation that informs the linker that the object
> file is compatible with BTI control flow integrity checks.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  MdePkg/Include/AArch64/ProcessorBind.h | 31 ++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
> index abe2571245c665f3..11814f1ffaef698a 100644
> --- a/MdePkg/Include/AArch64/ProcessorBind.h
> +++ b/MdePkg/Include/AArch64/ProcessorBind.h
> @@ -186,6 +186,37 @@ typedef INT64 INTN;
>  #define GCC_ASM_IMPORT(func__)  \
>           .extern  _CONCATENATE (__USER_LABEL_PREFIX__, func__)
>  
> +#if defined(__ARM_FEATURE_BTI_DEFAULT) && __ARM_FEATURE_BTI_DEFAULT == 1
> +#define AARCH64_BTI(__type)                                        \
> +    .ifnc         __type,                                         ;\
> +    bti           __type                                          ;\
> +    .endif                                                        ;\
> +    .ifndef       .Lgnu_bti_notesize                              ;\
> +    .pushsection  .note.gnu.property, "a"                         ;\
> +    .set          NT_GNU_PROPERTY_TYPE_0, 0x5                     ;\
> +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_AND, 0xc0000000  ;\
> +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_BTI, 0x1         ;\
> +    .align        3                                               ;\
> +    .long         .Lnamesize                                      ;\
> +    .long         .Lgnu_bti_notesize                              ;\
> +    .long         NT_GNU_PROPERTY_TYPE_0                          ;\
> +0:  .asciz        "GNU"                                           ;\
> +    .set          .Lnamesize, . - 0b                              ;\
> +    .align        3                                               ;\
> +1:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_AND              ;\
> +    .long         .Lvalsize                                       ;\
> +2:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_BTI              ;\
> +    .set          .Lvalsize, . - 2b                               ;\
> +    .align        3                                               ;\
> +    .set          .Lgnu_bti_notesize, . - 1b                      ;\
> +    .popsection                                                   ;\
> +    .endif
> +#endif
> +
> +#endif

Is that second endif superflouous?

/
    Leif

> +
> +#ifndef AARCH64_BTI
> +#define AARCH64_BTI(__type)
>  #endif
>  
>  /**
> -- 
> 2.39.2
> 

Re: [PATCH v2 01/17] MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note

[Ard Biesheuvel]

On Mon, 27 Mar 2023 at 13:53, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> On Mon, Mar 27, 2023 at 13:00:56 +0200, Ard Biesheuvel wrote:
> > Implement a CPP macro that can be called from .S files to emit the .note
> > section carrying the annotation that informs the linker that the object
> > file is compatible with BTI control flow integrity checks.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  MdePkg/Include/AArch64/ProcessorBind.h | 31 ++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
> > index abe2571245c665f3..11814f1ffaef698a 100644
> > --- a/MdePkg/Include/AArch64/ProcessorBind.h
> > +++ b/MdePkg/Include/AArch64/ProcessorBind.h
> > @@ -186,6 +186,37 @@ typedef INT64 INTN;
> >  #define GCC_ASM_IMPORT(func__)  \
> >           .extern  _CONCATENATE (__USER_LABEL_PREFIX__, func__)
> >
> > +#if defined(__ARM_FEATURE_BTI_DEFAULT) && __ARM_FEATURE_BTI_DEFAULT == 1
> > +#define AARCH64_BTI(__type)                                        \
> > +    .ifnc         __type,                                         ;\
> > +    bti           __type                                          ;\
> > +    .endif                                                        ;\
> > +    .ifndef       .Lgnu_bti_notesize                              ;\
> > +    .pushsection  .note.gnu.property, "a"                         ;\
> > +    .set          NT_GNU_PROPERTY_TYPE_0, 0x5                     ;\
> > +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_AND, 0xc0000000  ;\
> > +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_BTI, 0x1         ;\
> > +    .align        3                                               ;\
> > +    .long         .Lnamesize                                      ;\
> > +    .long         .Lgnu_bti_notesize                              ;\
> > +    .long         NT_GNU_PROPERTY_TYPE_0                          ;\
> > +0:  .asciz        "GNU"                                           ;\
> > +    .set          .Lnamesize, . - 0b                              ;\
> > +    .align        3                                               ;\
> > +1:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_AND              ;\
> > +    .long         .Lvalsize                                       ;\
> > +2:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_BTI              ;\
> > +    .set          .Lvalsize, . - 2b                               ;\
> > +    .align        3                                               ;\
> > +    .set          .Lgnu_bti_notesize, . - 1b                      ;\
> > +    .popsection                                                   ;\
> > +    .endif
> > +#endif
> > +
> > +#endif
>
> Is that second endif superflouous?
>

No the diff just turned out a bit odd.

Re: [PATCH v2 01/17] MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note

[Leif Lindholm]

On Mon, Mar 27, 2023 at 13:00:56 +0200, Ard Biesheuvel wrote:
> Implement a CPP macro that can be called from .S files to emit the .note
> section carrying the annotation that informs the linker that the object
> file is compatible with BTI control flow integrity checks.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  MdePkg/Include/AArch64/ProcessorBind.h | 31 ++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
> index abe2571245c665f3..11814f1ffaef698a 100644
> --- a/MdePkg/Include/AArch64/ProcessorBind.h
> +++ b/MdePkg/Include/AArch64/ProcessorBind.h
> @@ -186,6 +186,37 @@ typedef INT64 INTN;
>  #define GCC_ASM_IMPORT(func__)  \
>           .extern  _CONCATENATE (__USER_LABEL_PREFIX__, func__)
>  
> +#if defined(__ARM_FEATURE_BTI_DEFAULT) && __ARM_FEATURE_BTI_DEFAULT == 1
> +#define AARCH64_BTI(__type)                                        \
> +    .ifnc         __type,                                         ;\
> +    bti           __type                                          ;\
> +    .endif                                                        ;\

This didn't jump out at me until looking at the consumer side.
This overlays two different sets of functionality depending on whether
an option is given to the macro or not, which feels semantically
suboptimal to me (i.e. it makes my head hurt).

Could we split this into two macros - one that inserts the instruction
and one that inserts the note, and expand the latter in the former?

/
    Leif

> +    .ifndef       .Lgnu_bti_notesize                              ;\
> +    .pushsection  .note.gnu.property, "a"                         ;\
> +    .set          NT_GNU_PROPERTY_TYPE_0, 0x5                     ;\
> +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_AND, 0xc0000000  ;\
> +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_BTI, 0x1         ;\
> +    .align        3                                               ;\
> +    .long         .Lnamesize                                      ;\
> +    .long         .Lgnu_bti_notesize                              ;\
> +    .long         NT_GNU_PROPERTY_TYPE_0                          ;\
> +0:  .asciz        "GNU"                                           ;\
> +    .set          .Lnamesize, . - 0b                              ;\
> +    .align        3                                               ;\
> +1:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_AND              ;\
> +    .long         .Lvalsize                                       ;\
> +2:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_BTI              ;\
> +    .set          .Lvalsize, . - 2b                               ;\
> +    .align        3                                               ;\
> +    .set          .Lgnu_bti_notesize, . - 1b                      ;\
> +    .popsection                                                   ;\
> +    .endif
> +#endif
> +
> +#endif
> +
> +#ifndef AARCH64_BTI
> +#define AARCH64_BTI(__type)
>  #endif
>  
>  /**
> -- 
> 2.39.2
> 

Re: [edk2-devel] [PATCH v2 01/17] MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note

[Ard Biesheuvel]

On Mon, 27 Mar 2023 at 14:45, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> On Mon, Mar 27, 2023 at 13:00:56 +0200, Ard Biesheuvel wrote:
> > Implement a CPP macro that can be called from .S files to emit the .note
> > section carrying the annotation that informs the linker that the object
> > file is compatible with BTI control flow integrity checks.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  MdePkg/Include/AArch64/ProcessorBind.h | 31 ++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
> > index abe2571245c665f3..11814f1ffaef698a 100644
> > --- a/MdePkg/Include/AArch64/ProcessorBind.h
> > +++ b/MdePkg/Include/AArch64/ProcessorBind.h
> > @@ -186,6 +186,37 @@ typedef INT64 INTN;
> >  #define GCC_ASM_IMPORT(func__)  \
> >           .extern  _CONCATENATE (__USER_LABEL_PREFIX__, func__)
> >
> > +#if defined(__ARM_FEATURE_BTI_DEFAULT) && __ARM_FEATURE_BTI_DEFAULT == 1
> > +#define AARCH64_BTI(__type)                                        \
> > +    .ifnc         __type,                                         ;\
> > +    bti           __type                                          ;\
> > +    .endif                                                        ;\
>
> This didn't jump out at me until looking at the consumer side.
> This overlays two different sets of functionality depending on whether
> an option is given to the macro or not, which feels semantically
> suboptimal to me (i.e. it makes my head hurt).
>
> Could we split this into two macros - one that inserts the instruction
> and one that inserts the note, and expand the latter in the former?
>

Yeah, fair point.

So AARCH64_BTI_NOTE() to emit the note, and AARCH64_BTI() to emit the
instruction as well as the note.

Re: [edk2-devel] [PATCH v2 01/17] MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note

[Leif Lindholm]

On Mon, Mar 27, 2023 at 14:46:49 +0200, Ard Biesheuvel wrote:
> On Mon, 27 Mar 2023 at 14:45, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
> >
> > On Mon, Mar 27, 2023 at 13:00:56 +0200, Ard Biesheuvel wrote:
> > > Implement a CPP macro that can be called from .S files to emit the .note
> > > section carrying the annotation that informs the linker that the object
> > > file is compatible with BTI control flow integrity checks.
> > >
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > >  MdePkg/Include/AArch64/ProcessorBind.h | 31 ++++++++++++++++++++
> > >  1 file changed, 31 insertions(+)
> > >
> > > diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
> > > index abe2571245c665f3..11814f1ffaef698a 100644
> > > --- a/MdePkg/Include/AArch64/ProcessorBind.h
> > > +++ b/MdePkg/Include/AArch64/ProcessorBind.h
> > > @@ -186,6 +186,37 @@ typedef INT64 INTN;
> > >  #define GCC_ASM_IMPORT(func__)  \
> > >           .extern  _CONCATENATE (__USER_LABEL_PREFIX__, func__)
> > >
> > > +#if defined(__ARM_FEATURE_BTI_DEFAULT) && __ARM_FEATURE_BTI_DEFAULT == 1
> > > +#define AARCH64_BTI(__type)                                        \
> > > +    .ifnc         __type,                                         ;\
> > > +    bti           __type                                          ;\
> > > +    .endif                                                        ;\
> >
> > This didn't jump out at me until looking at the consumer side.
> > This overlays two different sets of functionality depending on whether
> > an option is given to the macro or not, which feels semantically
> > suboptimal to me (i.e. it makes my head hurt).
> >
> > Could we split this into two macros - one that inserts the instruction
> > and one that inserts the note, and expand the latter in the former?
> >
> 
> Yeah, fair point.
> 
> So AARCH64_BTI_NOTE() to emit the note, and AARCH64_BTI() to emit the
> instruction as well as the note.

Yeah, exacty.

> 
> 
> 

Re: [edk2-devel] [PATCH v2 00/17] Enable BTI support in memory attributes table

[Ard Biesheuvel]

(update Rebecca's email)

On Mon, 27 Mar 2023 at 13:43, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> On Mon, Mar 27, 2023 at 13:00:55 +0200, Ard Biesheuvel wrote:
> > Implement version 2 of the memory attributes table, which now contains a
> > flag informing the OS whether or not code regions may be mapped with CFI
> > mitigations such as IBT or BTI enabled.
> >
> > This series covers roughly the following parts:
> >
> > - (AARCH64) Annotate ELF objects generated from asm as BTI compatible
> >   when BTI codegen is enabled
> > - Update the BaseTools to emit the appropriate PE/COFF annotation when a
> >   BTI/IBT compatible ELF executable is converted to PE/COFF
> > - Take this PE/COFF annotation into account when populating the memory
> >   attributes table in the DXE core
> >
> > TODO:
> > - X64 changes to make the code IBT compatible and emit the ELF note
> > - Figure out how to generate such executables with native PE toolchains
> > - Implement BTI/IBT enforcement at boot time - this is something I
> >   intend to look into next.
> >
> > Can be tested with the CLANG38 toolchain (both Clang compiler and LLD
> > linker, version 3.8 or newer) with the following build options.
> >
> > [BuildOptions]
> >   GCC:*_*_AARCH64_PP_FLAGS = -mbranch-protection=bti
> >   GCC:*_*_AARCH64_CC_FLAGS = -mbranch-protection=bti
>
> I know you point out to use CLANG38, but the set is also tagged PATCH
> rather than RFC.
> I believe this option was added to GCC in version 9, meaning this is a
> breaking change for GCC8. Now, GCC8 is ancient, but I expect it's
> still what's available in RHEL8 for example. So it's worth mentioning.
>

Indeed.

So when and where to enable this (by default or not) is an open question.

I thought we might enable this by default for CLANGDWARF once we add
AArch64 support to it (which Rebecca is working on), and retire the
CLANG3x toolchains entirely.

Then, it is up to individual platforms to decide what they want to
enable or disable - it also depends on any asm code the platforms are
carrying. (This does not really matter for this series which only
covers runtime DXE executables running under the OS, but i'd like to
find ways to enable this at boot time as well)

Re: [PATCH v2 01/17] MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note

[Leif Lindholm]

On Mon, Mar 27, 2023 at 14:15:09 +0200, Ard Biesheuvel wrote:
> On Mon, 27 Mar 2023 at 13:53, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
> >
> > On Mon, Mar 27, 2023 at 13:00:56 +0200, Ard Biesheuvel wrote:
> > > Implement a CPP macro that can be called from .S files to emit the .note
> > > section carrying the annotation that informs the linker that the object
> > > file is compatible with BTI control flow integrity checks.
> > >
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > >  MdePkg/Include/AArch64/ProcessorBind.h | 31 ++++++++++++++++++++
> > >  1 file changed, 31 insertions(+)
> > >
> > > diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
> > > index abe2571245c665f3..11814f1ffaef698a 100644
> > > --- a/MdePkg/Include/AArch64/ProcessorBind.h
> > > +++ b/MdePkg/Include/AArch64/ProcessorBind.h
> > > @@ -186,6 +186,37 @@ typedef INT64 INTN;
> > >  #define GCC_ASM_IMPORT(func__)  \
> > >           .extern  _CONCATENATE (__USER_LABEL_PREFIX__, func__)
> > >
> > > +#if defined(__ARM_FEATURE_BTI_DEFAULT) && __ARM_FEATURE_BTI_DEFAULT == 1
> > > +#define AARCH64_BTI(__type)                                        \
> > > +    .ifnc         __type,                                         ;\
> > > +    bti           __type                                          ;\
> > > +    .endif                                                        ;\
> > > +    .ifndef       .Lgnu_bti_notesize                              ;\
> > > +    .pushsection  .note.gnu.property, "a"                         ;\
> > > +    .set          NT_GNU_PROPERTY_TYPE_0, 0x5                     ;\
> > > +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_AND, 0xc0000000  ;\
> > > +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_BTI, 0x1         ;\
> > > +    .align        3                                               ;\
> > > +    .long         .Lnamesize                                      ;\
> > > +    .long         .Lgnu_bti_notesize                              ;\
> > > +    .long         NT_GNU_PROPERTY_TYPE_0                          ;\
> > > +0:  .asciz        "GNU"                                           ;\
> > > +    .set          .Lnamesize, . - 0b                              ;\
> > > +    .align        3                                               ;\
> > > +1:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_AND              ;\
> > > +    .long         .Lvalsize                                       ;\
> > > +2:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_BTI              ;\
> > > +    .set          .Lvalsize, . - 2b                               ;\
> > > +    .align        3                                               ;\
> > > +    .set          .Lgnu_bti_notesize, . - 1b                      ;\
> > > +    .popsection                                                   ;\
> > > +    .endif
> > > +#endif
> > > +
> > > +#endif
> >
> > Is that second endif superflouous?
> >
> 
> No the diff just turned out a bit odd.

Oh, I see - it's because you add this inside the if __GNUC__ || __clang__
and add the #ifndef AARCH64_BTI outside it. Which is necessary. Moving
on.

/
    Leif

Re: [PATCH v2 11/17] ArmPkg, BaseTools AARCH64: Add BTI ELF note to .hii objects

[Leif Lindholm]

On Mon, Mar 27, 2023 at 13:01:06 +0200, Ard Biesheuvel wrote:
> The ELF based toolchains use objcopy to create HII object files, which
> contain only a single .hii section. This means no GNU note is inserted
> that describes the object as compatible with BTI, even though the lack
> of executable code in such an object makes the distinction irrelevant.
> However, the linker will not add the note globally to the resulting ELF
> executable, and this breaks BTI compatibility.
> 
> So let's insert a GNU BTI-compatible ELF note by hand when generating
> such object files.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  ArmPkg/Library/GnuNoteBti.bin     | Bin 0 -> 32 bytes
>  BaseTools/Conf/tools_def.template |   4 ++--
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/ArmPkg/Library/GnuNoteBti.bin b/ArmPkg/Library/GnuNoteBti.bin
> new file mode 100644
> index 0000000000000000000000000000000000000000..339567b4e89943c610b44767ddad5f631229ed3b
> GIT binary patch
> literal 32
> dcmZQ!U|<jcVpbq__X`D*3<p?%1S5zA1OOf&0m%RW
> 
> literal 0
> HcmV?d00001
> 
> diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
> index 471eb67c0c839730..ed6050aa96157cb9 100755
> --- a/BaseTools/Conf/tools_def.template
> +++ b/BaseTools/Conf/tools_def.template
> @@ -2400,7 +2400,7 @@ RELEASE_GCC5_ARM_DLINK_FLAGS     = DEF(GCC5_ARM_DLINK_FLAGS) -flto -Os -L$(WORKS
>  *_GCC5_AARCH64_DTCPP_FLAGS       = DEF(GCC_DTCPP_FLAGS)
>  *_GCC5_AARCH64_PLATFORM_FLAGS    =
>  *_GCC5_AARCH64_PP_FLAGS          = $(PLATFORM_FLAGS) DEF(GCC_PP_FLAGS)
> -*_GCC5_AARCH64_RC_FLAGS          = DEF(GCC_AARCH64_RC_FLAGS)
> +*_GCC5_AARCH64_RC_FLAGS          = DEF(GCC_AARCH64_RC_FLAGS) --add-section .note.gnu.property=$(WORKSPACE)/ArmPkg/Library/GnuNoteBti.bin --set-section-flags .note.gnu.property=alloc,readonly
>  *_GCC5_AARCH64_VFRPP_FLAGS       = $(PLATFORM_FLAGS) DEF(GCC_VFRPP_FLAGS)
>  *_GCC5_AARCH64_CC_XIPFLAGS       = DEF(GCC5_AARCH64_CC_XIPFLAGS)
>  
> @@ -2735,7 +2735,7 @@ DEFINE CLANG38_AARCH64_DLINK_FLAGS  = DEF(CLANG38_AARCH64_TARGET) DEF(GCC_AARCH6
>  *_CLANG38_AARCH64_DLINK2_FLAGS   = DEF(GCC_DLINK2_FLAGS_COMMON) -Wl,--defsym=PECOFF_HEADER_SIZE=0x228
>  *_CLANG38_AARCH64_PLATFORM_FLAGS =
>  *_CLANG38_AARCH64_PP_FLAGS       = DEF(GCC_PP_FLAGS) DEF(CLANG38_AARCH64_TARGET) $(PLATFORM_FLAGS)
> -*_CLANG38_AARCH64_RC_FLAGS       = DEF(GCC_AARCH64_RC_FLAGS)
> +*_CLANG38_AARCH64_RC_FLAGS       = DEF(GCC_AARCH64_RC_FLAGS) --add-section .note.gnu.property=$(WORKSPACE)/ArmPkg/Library/GnuNoteBti.bin --set-section-flags .note.gnu.property=alloc,readonly

Bikeshedding, but could we have an AARCH64_BTI_RC_FLAGS or something
set, which is expanded for each toolchain profile? I think this is
esoteric enough that it's helpful to group just the
bti-note-incantations together in a single place.

/
    Leif

>  *_CLANG38_AARCH64_VFRPP_FLAGS    = DEF(GCC_VFRPP_FLAGS) DEF(CLANG38_AARCH64_TARGET) $(PLATFORM_FLAGS)
>  *_CLANG38_AARCH64_ASLPP_FLAGS    = DEF(GCC_ASLPP_FLAGS) DEF(CLANG38_AARCH64_TARGET)
>  *_CLANG38_AARCH64_CC_XIPFLAGS    = DEF(GCC_AARCH64_CC_XIPFLAGS)
> -- 
> 2.39.2
> 

Re: [edk2-devel] [PATCH v2 11/17] ArmPkg, BaseTools AARCH64: Add BTI ELF note to .hii objects

[Ard Biesheuvel]

On Mon, 27 Mar 2023 at 15:10, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> On Mon, Mar 27, 2023 at 13:01:06 +0200, Ard Biesheuvel wrote:
> > The ELF based toolchains use objcopy to create HII object files, which
> > contain only a single .hii section. This means no GNU note is inserted
> > that describes the object as compatible with BTI, even though the lack
> > of executable code in such an object makes the distinction irrelevant.
> > However, the linker will not add the note globally to the resulting ELF
> > executable, and this breaks BTI compatibility.
> >
> > So let's insert a GNU BTI-compatible ELF note by hand when generating
> > such object files.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  ArmPkg/Library/GnuNoteBti.bin     | Bin 0 -> 32 bytes
> >  BaseTools/Conf/tools_def.template |   4 ++--
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/ArmPkg/Library/GnuNoteBti.bin b/ArmPkg/Library/GnuNoteBti.bin
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..339567b4e89943c610b44767ddad5f631229ed3b
> > GIT binary patch
> > literal 32
> > dcmZQ!U|<jcVpbq__X`D*3<p?%1S5zA1OOf&0m%RW
> >
> > literal 0
> > HcmV?d00001
> >
> > diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
> > index 471eb67c0c839730..ed6050aa96157cb9 100755
> > --- a/BaseTools/Conf/tools_def.template
> > +++ b/BaseTools/Conf/tools_def.template
> > @@ -2400,7 +2400,7 @@ RELEASE_GCC5_ARM_DLINK_FLAGS     = DEF(GCC5_ARM_DLINK_FLAGS) -flto -Os -L$(WORKS
> >  *_GCC5_AARCH64_DTCPP_FLAGS       = DEF(GCC_DTCPP_FLAGS)
> >  *_GCC5_AARCH64_PLATFORM_FLAGS    =
> >  *_GCC5_AARCH64_PP_FLAGS          = $(PLATFORM_FLAGS) DEF(GCC_PP_FLAGS)
> > -*_GCC5_AARCH64_RC_FLAGS          = DEF(GCC_AARCH64_RC_FLAGS)
> > +*_GCC5_AARCH64_RC_FLAGS          = DEF(GCC_AARCH64_RC_FLAGS) --add-section .note.gnu.property=$(WORKSPACE)/ArmPkg/Library/GnuNoteBti.bin --set-section-flags .note.gnu.property=alloc,readonly
> >  *_GCC5_AARCH64_VFRPP_FLAGS       = $(PLATFORM_FLAGS) DEF(GCC_VFRPP_FLAGS)
> >  *_GCC5_AARCH64_CC_XIPFLAGS       = DEF(GCC5_AARCH64_CC_XIPFLAGS)
> >
> > @@ -2735,7 +2735,7 @@ DEFINE CLANG38_AARCH64_DLINK_FLAGS  = DEF(CLANG38_AARCH64_TARGET) DEF(GCC_AARCH6
> >  *_CLANG38_AARCH64_DLINK2_FLAGS   = DEF(GCC_DLINK2_FLAGS_COMMON) -Wl,--defsym=PECOFF_HEADER_SIZE=0x228
> >  *_CLANG38_AARCH64_PLATFORM_FLAGS =
> >  *_CLANG38_AARCH64_PP_FLAGS       = DEF(GCC_PP_FLAGS) DEF(CLANG38_AARCH64_TARGET) $(PLATFORM_FLAGS)
> > -*_CLANG38_AARCH64_RC_FLAGS       = DEF(GCC_AARCH64_RC_FLAGS)
> > +*_CLANG38_AARCH64_RC_FLAGS       = DEF(GCC_AARCH64_RC_FLAGS) --add-section .note.gnu.property=$(WORKSPACE)/ArmPkg/Library/GnuNoteBti.bin --set-section-flags .note.gnu.property=alloc,readonly
>
> Bikeshedding, but could we have an AARCH64_BTI_RC_FLAGS or something
> set, which is expanded for each toolchain profile? I think this is
> esoteric enough that it's helpful to group just the
> bti-note-incantations together in a single place.
>

Sure.

It's a bit disappointing that we even need this - the linker should be
able to infer that for objects without any executable sections,
whether the note exists or not is irrelevant.

Re: [PATCH v2 15/17] MdePkg: Update MemoryAttributesTable to v2.10

[Leif Lindholm]

On Mon, Mar 27, 2023 at 13:01:10 +0200, 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.
> 
> Link: https://uefi.org/specs/UEFI/2.10/04_EFI_System_Table.html#efi-memory-attributes-table
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
> Acked-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
>  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 82f83a67b96d38c5..238c14ff92dfed31 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;

Does this not cause a bisect breakage vs patch 17?

/
    Leif

>    // 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.2
> 

Re: [edk2-devel] [PATCH v2 00/17] Enable BTI support in memory attributes table

[Gerd Hoffmann]

  Hi,

> > I know you point out to use CLANG38, but the set is also tagged PATCH
> > rather than RFC.
> > I believe this option was added to GCC in version 9, meaning this is a
> > breaking change for GCC8. Now, GCC8 is ancient, but I expect it's
> > still what's available in RHEL8 for example. So it's worth mentioning.
> 
> Indeed.
> 
> So when and where to enable this (by default or not) is an open question.

No objections to requiring a newer compiler from my side.

Even with the default system compiler not changing through the whole
live cycle RHEL typically offers newer gcc versions as an option
(packaged as gcc-toolset-${version}-gcc, latest version available for
RHEL-8 is gcc-12).

take care,
  Gerd

Re: [edk2-devel] [PATCH v2 01/17] MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note

[Pedro Falcato]

On Mon, Mar 27, 2023 at 12:01 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> Implement a CPP macro that can be called from .S files to emit the .note
> section carrying the annotation that informs the linker that the object
> file is compatible with BTI control flow integrity checks.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  MdePkg/Include/AArch64/ProcessorBind.h | 31 ++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
> index abe2571245c665f3..11814f1ffaef698a 100644
> --- a/MdePkg/Include/AArch64/ProcessorBind.h
> +++ b/MdePkg/Include/AArch64/ProcessorBind.h
> @@ -186,6 +186,37 @@ typedef INT64 INTN;
>  #define GCC_ASM_IMPORT(func__)  \
>           .extern  _CONCATENATE (__USER_LABEL_PREFIX__, func__)
>
> +#if defined(__ARM_FEATURE_BTI_DEFAULT) && __ARM_FEATURE_BTI_DEFAULT == 1
> +#define AARCH64_BTI(__type)                                        \
> +    .ifnc         __type,                                         ;\
> +    bti           __type                                          ;\
> +    .endif                                                        ;\
> +    .ifndef       .Lgnu_bti_notesize                              ;\
> +    .pushsection  .note.gnu.property, "a"                         ;\
> +    .set          NT_GNU_PROPERTY_TYPE_0, 0x5                     ;\
> +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_AND, 0xc0000000  ;\
> +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_BTI, 0x1         ;\
> +    .align        3                                               ;\
> +    .long         .Lnamesize                                      ;\
> +    .long         .Lgnu_bti_notesize                              ;\
> +    .long         NT_GNU_PROPERTY_TYPE_0                          ;\
> +0:  .asciz        "GNU"                                           ;\
> +    .set          .Lnamesize, . - 0b                              ;\
> +    .align        3                                               ;\
> +1:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_AND              ;\
> +    .long         .Lvalsize                                       ;\
> +2:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_BTI              ;\
> +    .set          .Lvalsize, . - 2b                               ;\
> +    .align        3                                               ;\
> +    .set          .Lgnu_bti_notesize, . - 1b                      ;\
> +    .popsection                                                   ;\
> +    .endif
> +#endif
> +
> +#endif
> +
> +#ifndef AARCH64_BTI
> +#define AARCH64_BTI(__type)
>  #endif
>
>  /**
> --
> 2.39.2

Patch-set wide comment: is there any chance we could take this
opportunity to introduce a global ASM_FUNC (or a more Linux-named
ENTRY(FuncName))?
It seems to be that the current way is a bit error prone and you end
up repeating yourself quite a bit with:

ASM_PFX(Foo):
AARCH64_BTI(c)
<code>

having a:
ASM_FUNC(Foo)
<code>

that does proper PFX and BTI expansion internally seems better to me.

-- 
Pedro

Re: [edk2-devel] [PATCH v2 01/17] MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note

[Leif Lindholm]

On Mon, Mar 27, 2023 at 15:12:29 +0100, Pedro Falcato wrote:
> On Mon, Mar 27, 2023 at 12:01 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > Implement a CPP macro that can be called from .S files to emit the .note
> > section carrying the annotation that informs the linker that the object
> > file is compatible with BTI control flow integrity checks.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  MdePkg/Include/AArch64/ProcessorBind.h | 31 ++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
> > index abe2571245c665f3..11814f1ffaef698a 100644
> > --- a/MdePkg/Include/AArch64/ProcessorBind.h
> > +++ b/MdePkg/Include/AArch64/ProcessorBind.h
> > @@ -186,6 +186,37 @@ typedef INT64 INTN;
> >  #define GCC_ASM_IMPORT(func__)  \
> >           .extern  _CONCATENATE (__USER_LABEL_PREFIX__, func__)
> >
> > +#if defined(__ARM_FEATURE_BTI_DEFAULT) && __ARM_FEATURE_BTI_DEFAULT == 1
> > +#define AARCH64_BTI(__type)                                        \
> > +    .ifnc         __type,                                         ;\
> > +    bti           __type                                          ;\
> > +    .endif                                                        ;\
> > +    .ifndef       .Lgnu_bti_notesize                              ;\
> > +    .pushsection  .note.gnu.property, "a"                         ;\
> > +    .set          NT_GNU_PROPERTY_TYPE_0, 0x5                     ;\
> > +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_AND, 0xc0000000  ;\
> > +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_BTI, 0x1         ;\
> > +    .align        3                                               ;\
> > +    .long         .Lnamesize                                      ;\
> > +    .long         .Lgnu_bti_notesize                              ;\
> > +    .long         NT_GNU_PROPERTY_TYPE_0                          ;\
> > +0:  .asciz        "GNU"                                           ;\
> > +    .set          .Lnamesize, . - 0b                              ;\
> > +    .align        3                                               ;\
> > +1:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_AND              ;\
> > +    .long         .Lvalsize                                       ;\
> > +2:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_BTI              ;\
> > +    .set          .Lvalsize, . - 2b                               ;\
> > +    .align        3                                               ;\
> > +    .set          .Lgnu_bti_notesize, . - 1b                      ;\
> > +    .popsection                                                   ;\
> > +    .endif
> > +#endif
> > +
> > +#endif
> > +
> > +#ifndef AARCH64_BTI
> > +#define AARCH64_BTI(__type)
> >  #endif
> >
> >  /**
> > --
> > 2.39.2
> 
> Patch-set wide comment: is there any chance we could take this
> opportunity to introduce a global ASM_FUNC (or a more Linux-named
> ENTRY(FuncName))?
> It seems to be that the current way is a bit error prone and you end
> up repeating yourself quite a bit with:
> 
> ASM_PFX(Foo):
> AARCH64_BTI(c)
> <code>
> 
> having a:
> ASM_FUNC(Foo)
> <code>
> 
> that does proper PFX and BTI expansion internally seems better to me.

I was thinking while looking at this patch that ASM_FUNC could
probably do with moving over to this file from AsmMacroIoLibV8.h.
I didn't take the thought far enough to consider including the BTI
bits in that, but I guess that could make sense.

/
    Leif

> 
> -- 
> Pedro
> 
> 
> 
> 
> 

Re: [PATCH v2 14/17] BaseTools/GenFw: Add DllCharacteristicsEx field to debug data

[Marvin Häuser]

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

Hi Ard,

> On 27. Mar 2023, at 13:01, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> The PE/COFF spec describes an additional DllCharacteristics field
> implemented as a debug directory entry, which carries flags related to
> which control flow integrity (CFI) features are supported by the binary.

Out of mere personal interest, is there any reference for this yet? The "PE format" page [1] doesn't seem to have this yet.

[1] https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#the-debug-section

> 
> So let's add this entry when doing ELF to PE/COFF conversion - we will
> add support for setting the flags in a subsequent patch.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> BaseTools/Source/C/GenFw/Elf64Convert.c               | 54 +++++++++++++++-----
> BaseTools/Source/C/GenFw/GenFw.c                      |  3 +-
> BaseTools/Source/C/Include/IndustryStandard/PeImage.h | 13 ++++-
> 3 files changed, 55 insertions(+), 15 deletions(-)
> 
> diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c b/BaseTools/Source/C/GenFw/Elf64Convert.c
> index 2a810e835d4a4a66..9c17c90b16602421 100644
> --- a/BaseTools/Source/C/GenFw/Elf64Convert.c
> +++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
> @@ -992,6 +992,16 @@ ScanSections64 (
>                 sizeof(EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY) +
>                 strlen(mInImageName) + 1;
> 
> +  //
> +  // Add more space in the .debug data region for the DllCharacteristicsEx
> +  // field.
> +  //
> +  if (mDllCharacteristicsEx != 0) {
> +    mCoffOffset = DebugRvaAlign(mCoffOffset) +
> +                  sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY) +
> +                  sizeof (EFI_IMAGE_DEBUG_EX_DLLCHARACTERISTICS_ENTRY);
> +  }
> +
>   mCoffOffset = CoffAlign(mCoffOffset);
>   if (SectionCount == 0) {
>     mDataOffset = mCoffOffset;
> @@ -2244,29 +2254,47 @@ WriteDebug64 (
>   VOID
>   )
> {
> -  UINT32                              Len;
> -  EFI_IMAGE_OPTIONAL_HEADER_UNION     *NtHdr;
> -  EFI_IMAGE_DATA_DIRECTORY            *DataDir;
> -  EFI_IMAGE_DEBUG_DIRECTORY_ENTRY     *Dir;
> -  EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY *Nb10;
> +  UINT32                                      Len;
> +  EFI_IMAGE_OPTIONAL_HEADER_UNION             *NtHdr;
> +  EFI_IMAGE_DATA_DIRECTORY                    *DataDir;
> +  EFI_IMAGE_DEBUG_DIRECTORY_ENTRY             *Dir;
> +  EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY         *Nb10;
> +  EFI_IMAGE_DEBUG_EX_DLLCHARACTERISTICS_ENTRY *DllEntry;
> 
>   Len = strlen(mInImageName) + 1;
> 
> +  NtHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)(mCoffFile + mNtHdrOffset);
> +  DataDir = &NtHdr->Pe32Plus.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG];
> +  DataDir->VirtualAddress = mDebugOffset;
> +  DataDir->Size = sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
> +
>   Dir = (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY*)(mCoffFile + mDebugOffset);
> +
> +  if (mDllCharacteristicsEx != 0) {
> +    DataDir->Size  += sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
> +
> +    Dir->Type       = EFI_IMAGE_DEBUG_TYPE_EX_DLLCHARACTERISTICS;
> +    Dir->SizeOfData = sizeof (EFI_IMAGE_DEBUG_EX_DLLCHARACTERISTICS_ENTRY);
> +    Dir->FileOffset = mDebugOffset + DataDir->Size +
> +                      sizeof (EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY) +
> +                      DebugRvaAlign(Len);
> +    Dir->RVA        = Dir->FileOffset;
> +
> +    DllEntry = (VOID *)(mCoffFile + Dir->FileOffset);
> +
> +    DllEntry->DllCharacteristicsEx = mDllCharacteristicsEx;
> +
> +    Dir++;
> +  }
> +
>   Dir->Type = EFI_IMAGE_DEBUG_TYPE_CODEVIEW;
>   Dir->SizeOfData = sizeof(EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY) + Len;
> -  Dir->RVA = mDebugOffset + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
> -  Dir->FileOffset = mDebugOffset + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
> +  Dir->RVA = mDebugOffset + DataDir->Size;
> +  Dir->FileOffset = mDebugOffset + DataDir->Size;
> 
>   Nb10 = (EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY*)(Dir + 1);
>   Nb10->Signature = CODEVIEW_SIGNATURE_NB10;
>   strcpy ((char *)(Nb10 + 1), mInImageName);
> -
> -
> -  NtHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)(mCoffFile + mNtHdrOffset);
> -  DataDir = &NtHdr->Pe32Plus.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG];
> -  DataDir->VirtualAddress = mDebugOffset;
> -  DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
> }
> 
> STATIC
> diff --git a/BaseTools/Source/C/GenFw/GenFw.c b/BaseTools/Source/C/GenFw/GenFw.c
> index 6f61f16788cd2a0a..d0e52ccc26431380 100644
> --- a/BaseTools/Source/C/GenFw/GenFw.c
> +++ b/BaseTools/Source/C/GenFw/GenFw.c
> @@ -2932,7 +2932,8 @@ Routine Description:
>       if (mIsConvertXip) {
>         DebugEntry->FileOffset = DebugEntry->RVA;
>       }
> -      if (ZeroDebugFlag || DebugEntry->Type != EFI_IMAGE_DEBUG_TYPE_CODEVIEW) {
> +      if ((ZeroDebugFlag || DebugEntry->Type != EFI_IMAGE_DEBUG_TYPE_CODEVIEW) &&
> +          (DebugEntry->Type != EFI_IMAGE_DEBUG_TYPE_EX_DLLCHARACTERISTICS)) {
>         memset (FileBuffer + DebugEntry->FileOffset, 0, DebugEntry->SizeOfData);
>         memset (DebugEntry, 0, sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY));
>       }
> diff --git a/BaseTools/Source/C/Include/IndustryStandard/PeImage.h b/BaseTools/Source/C/Include/IndustryStandard/PeImage.h
> index 77ded3f611398278..5e9428ab98c7f68a 100644
> --- a/BaseTools/Source/C/Include/IndustryStandard/PeImage.h
> +++ b/BaseTools/Source/C/Include/IndustryStandard/PeImage.h
> @@ -615,7 +615,8 @@ typedef struct {
> ///
> /// Debug Format
> ///
> -#define EFI_IMAGE_DEBUG_TYPE_CODEVIEW 2
> +#define EFI_IMAGE_DEBUG_TYPE_CODEVIEW               2
> +#define EFI_IMAGE_DEBUG_TYPE_EX_DLLCHARACTERISTICS  20
> 
> typedef struct {
>   UINT32  Characteristics;
> @@ -664,6 +665,16 @@ typedef struct {
>   //
> } EFI_IMAGE_DEBUG_CODEVIEW_MTOC_ENTRY;
> 
> +///
> +/// Extended DLL Characteristics
> +///
> +#define EFI_IMAGE_DLLCHARACTERISTICS_EX_CET_COMPAT          0x0001
> +#define EFI_IMAGE_DLLCHARACTERISTICS_EX_FORWARD_CFI_COMPAT  0x0040
> +
> +typedef struct {
> +  UINT16  DllCharacteristicsEx;
> +} EFI_IMAGE_DEBUG_EX_DLLCHARACTERISTICS_ENTRY;
> +
> //
> // .pdata entries for X64
> //
> -- 
> 2.39.2
> 


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

Re: [PATCH v2 14/17] BaseTools/GenFw: Add DllCharacteristicsEx field to debug data

[Ard Biesheuvel]

On Mon, 27 Mar 2023 at 17:46, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
> Hi Ard,
>
> On 27. Mar 2023, at 13:01, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> The PE/COFF spec describes an additional DllCharacteristics field
> implemented as a debug directory entry, which carries flags related to
> which control flow integrity (CFI) features are supported by the binary.
>
>
> Out of mere personal interest, is there any reference for this yet? The "PE format" page [1] doesn't seem to have this yet.
>
> [1] https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#the-debug-section
>
>

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

Re: [edk2-devel] [PATCH v2 17/17] MdeModulePkg: Enable forward edge CFI in mem attributes table

[Oliver Smith-Denny]

On 3/27/2023 4:01 AM, Ard Biesheuvel wrote:
> 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.
> 
> Given that the PE/COFF spec now defines a DllCharacteristicsEx flag that
> indicates whether or not a loaded image is compatible with this, we can
> wire this up to the flag in the memory attributes table, and set it if
> all loaded runtime image are compatible with it.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>   MdeModulePkg/Core/Dxe/DxeMain.h                    |  2 ++
>   MdeModulePkg/Core/Dxe/Image/Image.c                | 10 ++++++++++
>   MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c |  8 +++++++-
>   3 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
> index 815a6b4bd844a452..43daa037be441150 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/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
> index 8704ebea9a7c88c0..9dbfb2a1fad22ced 100644
> --- a/MdeModulePkg/Core/Dxe/Image/Image.c
> +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
> @@ -1399,6 +1399,16 @@ CoreLoadImageCommon (
>       CoreNewDebugImageInfoEntry (EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL, &Image->Info, Image->Handle);
> 
>     }
> 
>   
> 
> +  //
> 
> +  // Check whether we are loading a runtime image that lacks support for
> 
> +  // IBT/BTI landing pads.
> 
> +  //
> 
> +  if ((Image->ImageContext.ImageCodeMemoryType == EfiRuntimeServicesCode) &&
> 
> +      ((Image->ImageContext.DllCharacteristicsEx & EFI_IMAGE_DLLCHARACTERISTICS_EX_FORWARD_CFI_COMPAT) == 0))
> 
> +  {
> 
> +    gMemoryAttributesTableForwardCfi = FALSE;
> 
> +  }
If I understand this correctly, we are disabling Forward CFI if we attempt
to load any runtime images that don't support it. Would it make
sense to have a PCD to determine whether we strictly enforce
Forward CFI (i.e. don't load this incompatible image) in such a case?

Thanks,
Oliver

> 
> +
> 
>     //
> 
>     // 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 e079213711875f89..fd127ee167e1ac9a 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  = TRUE;
> 
>   
> 
>   /**
> 
>     Install MemoryAttributesTable.
> 
> @@ -182,7 +183,12 @@ 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));
> 

Re: [edk2-devel] [PATCH v2 00/17] Enable BTI support in memory attributes table

[Oliver Smith-Denny]

For the patchset:

Reviewed-by: Oliver Smith-Denny <osd@smith-denny.com>

Thanks!

On 3/27/2023 4:00 AM, Ard Biesheuvel wrote:
> Implement version 2 of the memory attributes table, which now contains a
> 
> flag informing the OS whether or not code regions may be mapped with CFI
> 
> mitigations such as IBT or BTI enabled.
> 
> 
> 
> This series covers roughly the following parts:
> 
> 
> 
> - (AARCH64) Annotate ELF objects generated from asm as BTI compatible
> 
>    when BTI codegen is enabled
> 
> - Update the BaseTools to emit the appropriate PE/COFF annotation when a
> 
>    BTI/IBT compatible ELF executable is converted to PE/COFF
> 
> - Take this PE/COFF annotation into account when populating the memory
> 
>    attributes table in the DXE core
> 
> 
> 
> TODO:
> 
> - X64 changes to make the code IBT compatible and emit the ELF note
> 
> - Figure out how to generate such executables with native PE toolchains
> 
> - Implement BTI/IBT enforcement at boot time - this is something I
> 
>    intend to look into next.
> 
> 
> 
> Can be tested with the CLANG38 toolchain (both Clang compiler and LLD
> 
> linker, version 3.8 or newer) with the following build options.
> 
> 
> 
> [BuildOptions]
> 
>    GCC:*_*_AARCH64_PP_FLAGS = -mbranch-protection=bti
> 
>    GCC:*_*_AARCH64_CC_FLAGS = -mbranch-protection=bti
> 
>    GCC:*_*_AARCH64_DLINK_FLAGS = -fuse-ld=lld -Wl,--no-relax,--no-pie,-z,bti-report=error
> 
> 
> 
> 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>
> 
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> 
> Cc: Bob Feng <bob.c.feng@intel.com>
> 
> 
> 
> Ard Biesheuvel (17):
> 
>    MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note
> 
>    MdePkg/BaseCpuLib AARCH64: Make asm files BTI compatible
> 
>    MdePkg/BaseIoLibIntrinsic AARCH64: Make asm files BTI compatible
> 
>    MdePkg/BaseLib AARCH64: Make LongJump() BTI compatible
> 
>    MdePkg/BaseLib AARCH64: Make asm files BTI compatible
> 
>    MdePkg/BaseMemoryLibOptDxe AARCH64: Make asm files BTI compatible
> 
>    MdePkg/BaseSynchronizationLib AARCH64: Make asm files BTI compatible
> 
>    MdePkg/BaseRngLib AARCH64: Make asm files BTI compatible
> 
>    ArmPkg: Emit BTI opcodes when BTI codegen is enabled
> 
>    ArmPkg/GccLto AARCH64: Add BTI note to LTO helper library
> 
>    ArmPkg, BaseTools AARCH64: Add BTI ELF note to .hii objects
> 
>    ArmPlatformPkg/PrePeiCore: Make vector table object BTI compatible
> 
>    BaseTools/GenFw: Parse IBT/BTI support status from ELF note
> 
>    BaseTools/GenFw: Add DllCharacteristicsEx field to debug data
> 
>    MdePkg: Update MemoryAttributesTable to v2.10
> 
>    MdePkg/PeCoffLib: Capture DLL characteristics fieldis in image context
> 
>    MdeModulePkg: Enable forward edge CFI in mem attributes table
> 
> 
> 
>   ArmPkg/Include/AsmMacroIoLibV8.h                                |   3 +-
> 
>   ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S       |   3 +-
> 
>   ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S                       |   4 +-
> 
>   ArmPkg/Library/GccLto/liblto-aarch64.a                          | Bin 1016 -> 1128 bytes
> 
>   ArmPkg/Library/GnuNoteBti.bin                                   | Bin 0 -> 32 bytes
> 
>   ArmPlatformPkg/PrePeiCore/AArch64/Exception.S                   |   2 +
> 
>   ArmVirtPkg/Library/ArmPlatformLibQemu/IdMap.S                   |   2 +
> 
>   BaseTools/Conf/tools_def.template                               |   4 +-
> 
>   BaseTools/Source/C/GenFw/Elf64Convert.c                         | 104 +++++++++++++++++---
> 
>   BaseTools/Source/C/GenFw/GenFw.c                                |   3 +-
> 
>   BaseTools/Source/C/GenFw/elf_common.h                           |   9 ++
> 
>   BaseTools/Source/C/Include/IndustryStandard/PeImage.h           |  13 ++-
> 
>   MdeModulePkg/Core/Dxe/DxeMain.h                                 |   2 +
> 
>   MdeModulePkg/Core/Dxe/Image/Image.c                             |  10 ++
> 
>   MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c              |   8 +-
> 
>   MdePkg/Include/AArch64/ProcessorBind.h                          |  31 ++++++
> 
>   MdePkg/Include/Guid/MemoryAttributesTable.h                     |   8 +-
> 
>   MdePkg/Include/IndustryStandard/PeImage.h                       |  13 ++-
> 
>   MdePkg/Include/Library/PeCoffLib.h                              |   6 ++
> 
>   MdePkg/Library/BaseCpuLib/AArch64/CpuFlushTlb.S                 |   1 +
> 
>   MdePkg/Library/BaseCpuLib/AArch64/CpuSleep.S                    |   1 +
> 
>   MdePkg/Library/BaseIoLibIntrinsic/AArch64/ArmVirtMmio.S         |   8 ++
> 
>   MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.S                  |   1 +
> 
>   MdePkg/Library/BaseLib/AArch64/DisableInterrupts.S              |   1 +
> 
>   MdePkg/Library/BaseLib/AArch64/EnableInterrupts.S               |   1 +
> 
>   MdePkg/Library/BaseLib/AArch64/GetInterruptsState.S             |   1 +
> 
>   MdePkg/Library/BaseLib/AArch64/MemoryFence.S                    |   1 +
> 
>   MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S                |   5 +-
> 
>   MdePkg/Library/BaseLib/AArch64/SpeculationBarrier.S             |   1 +
> 
>   MdePkg/Library/BaseLib/AArch64/SwitchStack.S                    |   2 +
> 
>   MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CompareGuid.S        |   1 +
> 
>   MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CompareMem.S         |   1 +
> 
>   MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CopyMem.S            |   1 +
> 
>   MdePkg/Library/BaseMemoryLibOptDxe/AArch64/ScanMem.S            |   1 +
> 
>   MdePkg/Library/BaseMemoryLibOptDxe/AArch64/SetMem.S             |   5 +
> 
>   MdePkg/Library/BasePeCoffLib/BasePeCoff.c                       |  46 ++++++---
> 
>   MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.S              |   3 +-
> 
>   MdePkg/Library/BaseRngLib/AArch64/ArmRng.S                      |   1 +
> 
>   MdePkg/Library/BaseSynchronizationLib/AArch64/Synchronization.S |   5 +
> 
>   39 files changed, 270 insertions(+), 42 deletions(-)
> 
>   create mode 100644 ArmPkg/Library/GnuNoteBti.bin
> 
> 
> 

Re: [PATCH v2 00/17] Enable BTI support in memory attributes table

[Leif Lindholm]

On Mon, Mar 27, 2023 at 13:00:55 +0200, Ard Biesheuvel wrote:
> Implement version 2 of the memory attributes table, which now contains a
> flag informing the OS whether or not code regions may be mapped with CFI
> mitigations such as IBT or BTI enabled.
> 
> This series covers roughly the following parts:
> 
> - (AARCH64) Annotate ELF objects generated from asm as BTI compatible
>   when BTI codegen is enabled
> - Update the BaseTools to emit the appropriate PE/COFF annotation when a
>   BTI/IBT compatible ELF executable is converted to PE/COFF
> - Take this PE/COFF annotation into account when populating the memory
>   attributes table in the DXE core

For any patches I haven't explicitly commented on in this set:
Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>

(but I did comment on patch 17 in the review of patch 15)

/
    Leif

> TODO:
> - X64 changes to make the code IBT compatible and emit the ELF note
> - Figure out how to generate such executables with native PE toolchains
> - Implement BTI/IBT enforcement at boot time - this is something I
>   intend to look into next.
> 
> Can be tested with the CLANG38 toolchain (both Clang compiler and LLD
> linker, version 3.8 or newer) with the following build options.
> 
> [BuildOptions]
>   GCC:*_*_AARCH64_PP_FLAGS = -mbranch-protection=bti
>   GCC:*_*_AARCH64_CC_FLAGS = -mbranch-protection=bti
>   GCC:*_*_AARCH64_DLINK_FLAGS = -fuse-ld=lld -Wl,--no-relax,--no-pie,-z,bti-report=error
> 
> 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>
> Cc: Marvin H??user <mhaeuser@posteo.de>
> Cc: Bob Feng <bob.c.feng@intel.com>
> 
> Ard Biesheuvel (17):
>   MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note
>   MdePkg/BaseCpuLib AARCH64: Make asm files BTI compatible
>   MdePkg/BaseIoLibIntrinsic AARCH64: Make asm files BTI compatible
>   MdePkg/BaseLib AARCH64: Make LongJump() BTI compatible
>   MdePkg/BaseLib AARCH64: Make asm files BTI compatible
>   MdePkg/BaseMemoryLibOptDxe AARCH64: Make asm files BTI compatible
>   MdePkg/BaseSynchronizationLib AARCH64: Make asm files BTI compatible
>   MdePkg/BaseRngLib AARCH64: Make asm files BTI compatible
>   ArmPkg: Emit BTI opcodes when BTI codegen is enabled
>   ArmPkg/GccLto AARCH64: Add BTI note to LTO helper library
>   ArmPkg, BaseTools AARCH64: Add BTI ELF note to .hii objects
>   ArmPlatformPkg/PrePeiCore: Make vector table object BTI compatible
>   BaseTools/GenFw: Parse IBT/BTI support status from ELF note
>   BaseTools/GenFw: Add DllCharacteristicsEx field to debug data
>   MdePkg: Update MemoryAttributesTable to v2.10
>   MdePkg/PeCoffLib: Capture DLL characteristics fieldis in image context
>   MdeModulePkg: Enable forward edge CFI in mem attributes table
> 
>  ArmPkg/Include/AsmMacroIoLibV8.h                                |   3 +-
>  ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S       |   3 +-
>  ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S                       |   4 +-
>  ArmPkg/Library/GccLto/liblto-aarch64.a                          | Bin 1016 -> 1128 bytes
>  ArmPkg/Library/GnuNoteBti.bin                                   | Bin 0 -> 32 bytes
>  ArmPlatformPkg/PrePeiCore/AArch64/Exception.S                   |   2 +
>  ArmVirtPkg/Library/ArmPlatformLibQemu/IdMap.S                   |   2 +
>  BaseTools/Conf/tools_def.template                               |   4 +-
>  BaseTools/Source/C/GenFw/Elf64Convert.c                         | 104 +++++++++++++++++---
>  BaseTools/Source/C/GenFw/GenFw.c                                |   3 +-
>  BaseTools/Source/C/GenFw/elf_common.h                           |   9 ++
>  BaseTools/Source/C/Include/IndustryStandard/PeImage.h           |  13 ++-
>  MdeModulePkg/Core/Dxe/DxeMain.h                                 |   2 +
>  MdeModulePkg/Core/Dxe/Image/Image.c                             |  10 ++
>  MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c              |   8 +-
>  MdePkg/Include/AArch64/ProcessorBind.h                          |  31 ++++++
>  MdePkg/Include/Guid/MemoryAttributesTable.h                     |   8 +-
>  MdePkg/Include/IndustryStandard/PeImage.h                       |  13 ++-
>  MdePkg/Include/Library/PeCoffLib.h                              |   6 ++
>  MdePkg/Library/BaseCpuLib/AArch64/CpuFlushTlb.S                 |   1 +
>  MdePkg/Library/BaseCpuLib/AArch64/CpuSleep.S                    |   1 +
>  MdePkg/Library/BaseIoLibIntrinsic/AArch64/ArmVirtMmio.S         |   8 ++
>  MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.S                  |   1 +
>  MdePkg/Library/BaseLib/AArch64/DisableInterrupts.S              |   1 +
>  MdePkg/Library/BaseLib/AArch64/EnableInterrupts.S               |   1 +
>  MdePkg/Library/BaseLib/AArch64/GetInterruptsState.S             |   1 +
>  MdePkg/Library/BaseLib/AArch64/MemoryFence.S                    |   1 +
>  MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S                |   5 +-
>  MdePkg/Library/BaseLib/AArch64/SpeculationBarrier.S             |   1 +
>  MdePkg/Library/BaseLib/AArch64/SwitchStack.S                    |   2 +
>  MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CompareGuid.S        |   1 +
>  MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CompareMem.S         |   1 +
>  MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CopyMem.S            |   1 +
>  MdePkg/Library/BaseMemoryLibOptDxe/AArch64/ScanMem.S            |   1 +
>  MdePkg/Library/BaseMemoryLibOptDxe/AArch64/SetMem.S             |   5 +
>  MdePkg/Library/BasePeCoffLib/BasePeCoff.c                       |  46 ++++++---
>  MdePkg/Library/BaseRngLib/AArch64/ArmReadIdIsar0.S              |   3 +-
>  MdePkg/Library/BaseRngLib/AArch64/ArmRng.S                      |   1 +
>  MdePkg/Library/BaseSynchronizationLib/AArch64/Synchronization.S |   5 +
>  39 files changed, 270 insertions(+), 42 deletions(-)
>  create mode 100644 ArmPkg/Library/GnuNoteBti.bin
> 
> -- 
> 2.39.2
> 

Re: [edk2-devel] [PATCH v2 15/17] MdePkg: Update MemoryAttributesTable to v2.10

[Ard Biesheuvel]

On Mon, 27 Mar 2023 at 15:30, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> On Mon, Mar 27, 2023 at 13:01:10 +0200, 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.
> >
> > Link: https://uefi.org/specs/UEFI/2.10/04_EFI_System_Table.html#efi-memory-attributes-table
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
> > Acked-by: Michael Kubacki <michael.kubacki@microsoft.com>
> > ---
> >  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 82f83a67b96d38c5..238c14ff92dfed31 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;
>
> Does this not cause a bisect breakage vs patch 17?
>

Yeah, rebase error - this change should update the reference to
Reserved as well in
MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c, and patch 17/17
should only change the value that gets assigned to it.

Re: [edk2-devel] [PATCH v2 15/17] MdePkg: Update MemoryAttributesTable to v2.10

[Leif Lindholm]

On Wed, Mar 29, 2023 at 18:47:50 +0200, Ard Biesheuvel wrote:
> On Mon, 27 Mar 2023 at 15:30, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
> >
> > On Mon, Mar 27, 2023 at 13:01:10 +0200, 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.
> > >
> > > Link: https://uefi.org/specs/UEFI/2.10/04_EFI_System_Table.html#efi-memory-attributes-table
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
> > > Acked-by: Michael Kubacki <michael.kubacki@microsoft.com>
> > > ---
> > >  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 82f83a67b96d38c5..238c14ff92dfed31 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;
> >
> > Does this not cause a bisect breakage vs patch 17?
> >
> 
> Yeah, rebase error - this change should update the reference to
> Reserved as well in
> MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c, and patch 17/17
> should only change the value that gets assigned to it.

Ah, good.
With that fixed, for 15 and 17:
Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>

/
    Leif


> 
> 
> 
> 

Re: [edk2-devel] [PATCH v2 01/17] MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note

[Ard Biesheuvel]

On Mon, 27 Mar 2023 at 16:24, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> On Mon, Mar 27, 2023 at 15:12:29 +0100, Pedro Falcato wrote:
> > On Mon, Mar 27, 2023 at 12:01 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > Implement a CPP macro that can be called from .S files to emit the .note
> > > section carrying the annotation that informs the linker that the object
> > > file is compatible with BTI control flow integrity checks.
> > >
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > >  MdePkg/Include/AArch64/ProcessorBind.h | 31 ++++++++++++++++++++
> > >  1 file changed, 31 insertions(+)
> > >
> > > diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
> > > index abe2571245c665f3..11814f1ffaef698a 100644
> > > --- a/MdePkg/Include/AArch64/ProcessorBind.h
> > > +++ b/MdePkg/Include/AArch64/ProcessorBind.h
> > > @@ -186,6 +186,37 @@ typedef INT64 INTN;
> > >  #define GCC_ASM_IMPORT(func__)  \
> > >           .extern  _CONCATENATE (__USER_LABEL_PREFIX__, func__)
> > >
> > > +#if defined(__ARM_FEATURE_BTI_DEFAULT) && __ARM_FEATURE_BTI_DEFAULT == 1
> > > +#define AARCH64_BTI(__type)                                        \
> > > +    .ifnc         __type,                                         ;\
> > > +    bti           __type                                          ;\
> > > +    .endif                                                        ;\
> > > +    .ifndef       .Lgnu_bti_notesize                              ;\
> > > +    .pushsection  .note.gnu.property, "a"                         ;\
> > > +    .set          NT_GNU_PROPERTY_TYPE_0, 0x5                     ;\
> > > +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_AND, 0xc0000000  ;\
> > > +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_BTI, 0x1         ;\
> > > +    .align        3                                               ;\
> > > +    .long         .Lnamesize                                      ;\
> > > +    .long         .Lgnu_bti_notesize                              ;\
> > > +    .long         NT_GNU_PROPERTY_TYPE_0                          ;\
> > > +0:  .asciz        "GNU"                                           ;\
> > > +    .set          .Lnamesize, . - 0b                              ;\
> > > +    .align        3                                               ;\
> > > +1:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_AND              ;\
> > > +    .long         .Lvalsize                                       ;\
> > > +2:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_BTI              ;\
> > > +    .set          .Lvalsize, . - 2b                               ;\
> > > +    .align        3                                               ;\
> > > +    .set          .Lgnu_bti_notesize, . - 1b                      ;\
> > > +    .popsection                                                   ;\
> > > +    .endif
> > > +#endif
> > > +
> > > +#endif
> > > +
> > > +#ifndef AARCH64_BTI
> > > +#define AARCH64_BTI(__type)
> > >  #endif
> > >
> > >  /**
> > > --
> > > 2.39.2
> >
> > Patch-set wide comment: is there any chance we could take this
> > opportunity to introduce a global ASM_FUNC (or a more Linux-named
> > ENTRY(FuncName))?
> > It seems to be that the current way is a bit error prone and you end
> > up repeating yourself quite a bit with:
> >
> > ASM_PFX(Foo):
> > AARCH64_BTI(c)
> > <code>
> >
> > having a:
> > ASM_FUNC(Foo)
> > <code>
> >
> > that does proper PFX and BTI expansion internally seems better to me.
>
> I was thinking while looking at this patch that ASM_FUNC could
> probably do with moving over to this file from AsmMacroIoLibV8.h.
> I didn't take the thought far enough to consider including the BTI
> bits in that, but I guess that could make sense.
>

Yeah, but I'd prefer it if we could do that globally, and not just for
AArch64 as part of this series.

Re: [edk2-devel] [PATCH v2 00/17] Enable BTI support in memory attributes table

[Ard Biesheuvel]

On Wed, 29 Mar 2023 at 18:31, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> On Mon, Mar 27, 2023 at 13:00:55 +0200, Ard Biesheuvel wrote:
> > Implement version 2 of the memory attributes table, which now contains a
> > flag informing the OS whether or not code regions may be mapped with CFI
> > mitigations such as IBT or BTI enabled.
> >
> > This series covers roughly the following parts:
> >
> > - (AARCH64) Annotate ELF objects generated from asm as BTI compatible
> >   when BTI codegen is enabled
> > - Update the BaseTools to emit the appropriate PE/COFF annotation when a
> >   BTI/IBT compatible ELF executable is converted to PE/COFF
> > - Take this PE/COFF annotation into account when populating the memory
> >   attributes table in the DXE core
>
> For any patches I haven't explicitly commented on in this set:
> Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>
>
> (but I did comment on patch 17 in the review of patch 15)
>

Thanks.

I have pushed the AArch64 specific codegen changes and the MdePkg one
that updates the definition of the table.

That leaves the following changes:

  BaseTools/GenFw: Parse IBT/BTI support status from ELF note
  BaseTools/GenFw: Add DllCharacteristicsEx field to debug data

Liming, Bob, Rebecca: any comments here?

  MdePkg/PeCoffLib: Capture DLL characteristics fieldis in image context
  MdeModulePkg: Enable forward edge CFI in mem attributes table

Jiewen, Liming?

Re: [edk2-devel] [PATCH v2 01/17] MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note

[Leif Lindholm]

On Thu, Mar 30, 2023 at 09:28:45 +0200, Ard Biesheuvel wrote:
> > > Patch-set wide comment: is there any chance we could take this
> > > opportunity to introduce a global ASM_FUNC (or a more Linux-named
> > > ENTRY(FuncName))?
> > > It seems to be that the current way is a bit error prone and you end
> > > up repeating yourself quite a bit with:
> > >
> > > ASM_PFX(Foo):
> > > AARCH64_BTI(c)
> > > <code>
> > >
> > > having a:
> > > ASM_FUNC(Foo)
> > > <code>
> > >
> > > that does proper PFX and BTI expansion internally seems better to me.
> >
> > I was thinking while looking at this patch that ASM_FUNC could
> > probably do with moving over to this file from AsmMacroIoLibV8.h.
> > I didn't take the thought far enough to consider including the BTI
> > bits in that, but I guess that could make sense.
> 
> Yeah, but I'd prefer it if we could do that globally, and not just for
> AArch64 as part of this series.

Yeah, no issue with that.
This set adds primitives that will be useful for that in future.

/
    Leif

Re: [edk2-devel] [PATCH v2 16/17] MdePkg/PeCoffLib: Capture DLL characteristics fieldis in image context

[Michael Kubacki]

On 3/27/2023 7:01 AM, Ard Biesheuvel wrote:
> When loading a PE/COFF image, capture the DLL characteristics fields of
> the header into our image context structure so we can refer to them when
> mapping the image.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>   MdePkg/Include/IndustryStandard/PeImage.h | 13 +++++-
>   MdePkg/Include/Library/PeCoffLib.h        |  6 +++
>   MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 46 +++++++++++++++-----
>   3 files changed, 53 insertions(+), 12 deletions(-)
> 
> diff --git a/MdePkg/Include/IndustryStandard/PeImage.h b/MdePkg/Include/IndustryStandard/PeImage.h
> index dd4cc25483bc4bcf..a5b9b848ed391f98 100644
> --- a/MdePkg/Include/IndustryStandard/PeImage.h
> +++ b/MdePkg/Include/IndustryStandard/PeImage.h
> @@ -625,7 +625,8 @@ typedef struct {
>     UINT32    FileOffset;  ///< The file pointer to the debug data.
> 
>   } EFI_IMAGE_DEBUG_DIRECTORY_ENTRY;
> 
>   
> 
> -#define EFI_IMAGE_DEBUG_TYPE_CODEVIEW  2    ///< The Visual C++ debug information.
> 
> +#define EFI_IMAGE_DEBUG_TYPE_CODEVIEW               2    ///< The Visual C++ debug information.
> 
> +#define EFI_IMAGE_DEBUG_TYPE_EX_DLLCHARACTERISTICS  20
> 
>   
> 
>   ///
> 
>   /// Debug Data Structure defined in Microsoft C++.
> 
> @@ -669,6 +670,16 @@ typedef struct {
>     //
> 
>   } EFI_IMAGE_DEBUG_CODEVIEW_MTOC_ENTRY;
> 
>   
> 
> +///
> 
> +/// Extended DLL Characteristics
> 
> +///
> 
> +#define EFI_IMAGE_DLLCHARACTERISTICS_EX_CET_COMPAT          0x0001
> 
> +#define EFI_IMAGE_DLLCHARACTERISTICS_EX_FORWARD_CFI_COMPAT  0x0040
> 
> +
> 
> +typedef struct {
> 
> +  UINT16    DllCharacteristicsEx;
> 
> +} EFI_IMAGE_DEBUG_EX_DLLCHARACTERISTICS_ENTRY;
> 
> +
> 
>   ///
> 
>   /// Resource format.
> 
>   ///
> 
> diff --git a/MdePkg/Include/Library/PeCoffLib.h b/MdePkg/Include/Library/PeCoffLib.h
> index b45879453785c77d..d21c932076c072f6 100644
> --- a/MdePkg/Include/Library/PeCoffLib.h
> +++ b/MdePkg/Include/Library/PeCoffLib.h
> @@ -171,6 +171,12 @@ typedef struct {
>     ///
> 
>     UINT16                      ImageType;
> 
>     ///
> 
> +  /// Set by PeCoffLoaderGetImageInfo() to the DLL flags stored in the PE/COFF header and
> 
> +  /// in the DllCharacteristicsEx debug table.
> 
> +  ///
> 
> +  UINT16                      DllCharacteristics;
> 
> +  UINT16                      DllCharacteristicsEx;
> 
I know DllCharacteristics has a size of 2 in the spec, but the 
DllCharacteristicsEx is defined as 4 bytes. I will try to get a spec 
update to clarify this.

> +  ///
> 
>     /// Set by PeCoffLoaderGetImageInfo() to TRUE if the PE/COFF image does not contain
> 
>     /// relocation information.
> 
>     ///
> 
> diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> index 97a8aaf8c73d3e3c..4b71176a0c7c2ed0 100644
> --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> @@ -308,10 +308,11 @@ PeCoffLoaderGetPeHeader (
>         //
> 
>         // Use PE32 offset
> 
>         //
> 
> -      ImageContext->ImageType        = Hdr.Pe32->OptionalHeader.Subsystem;
> 
> -      ImageContext->ImageSize        = (UINT64)Hdr.Pe32->OptionalHeader.SizeOfImage;
> 
> -      ImageContext->SectionAlignment = Hdr.Pe32->OptionalHeader.SectionAlignment;
> 
> -      ImageContext->SizeOfHeaders    = Hdr.Pe32->OptionalHeader.SizeOfHeaders;
> 
> +      ImageContext->ImageType          = Hdr.Pe32->OptionalHeader.Subsystem;
> 
> +      ImageContext->ImageSize          = (UINT64)Hdr.Pe32->OptionalHeader.SizeOfImage;
> 
> +      ImageContext->SectionAlignment   = Hdr.Pe32->OptionalHeader.SectionAlignment;
> 
> +      ImageContext->SizeOfHeaders      = Hdr.Pe32->OptionalHeader.SizeOfHeaders;
> 
> +      ImageContext->DllCharacteristics = Hdr.Pe32->OptionalHeader.DllCharacteristics;
> 
>       } else if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
> 
>         //
> 
>         // 1. Check FileHeader.NumberOfRvaAndSizes filed.
> 
> @@ -429,10 +430,11 @@ PeCoffLoaderGetPeHeader (
>         //
> 
>         // Use PE32+ offset
> 
>         //
> 
> -      ImageContext->ImageType        = Hdr.Pe32Plus->OptionalHeader.Subsystem;
> 
> -      ImageContext->ImageSize        = (UINT64)Hdr.Pe32Plus->OptionalHeader.SizeOfImage;
> 
> -      ImageContext->SectionAlignment = Hdr.Pe32Plus->OptionalHeader.SectionAlignment;
> 
> -      ImageContext->SizeOfHeaders    = Hdr.Pe32Plus->OptionalHeader.SizeOfHeaders;
> 
> +      ImageContext->ImageType          = Hdr.Pe32Plus->OptionalHeader.Subsystem;
> 
> +      ImageContext->ImageSize          = (UINT64)Hdr.Pe32Plus->OptionalHeader.SizeOfImage;
> 
> +      ImageContext->SectionAlignment   = Hdr.Pe32Plus->OptionalHeader.SectionAlignment;
> 
> +      ImageContext->SizeOfHeaders      = Hdr.Pe32Plus->OptionalHeader.SizeOfHeaders;
> 
> +      ImageContext->DllCharacteristics = Hdr.Pe32Plus->OptionalHeader.DllCharacteristics;
> 
>       } else {
> 
>         ImageContext->ImageError = IMAGE_ERROR_INVALID_MACHINE_TYPE;
> 
>         return RETURN_UNSUPPORTED;
> 
> @@ -545,8 +547,9 @@ PeCoffLoaderGetPeHeader (
>     Retrieves information about a PE/COFF image.
> 
>   
> 
>     Computes the PeCoffHeaderOffset, IsTeImage, ImageType, ImageAddress, ImageSize,
> 
> -  DestinationAddress, RelocationsStripped, SectionAlignment, SizeOfHeaders, and
> 
> -  DebugDirectoryEntryRva fields of the ImageContext structure.
> 
> +  DestinationAddress, RelocationsStripped, SectionAlignment, SizeOfHeaders,
> 
> +  DllCharacteristics, DllCharacteristicsEx and DebugDirectoryEntryRva fields of
> 
> +  the ImageContext structure.
> 
>     If ImageContext is NULL, then return RETURN_INVALID_PARAMETER.
> 
>     If the PE/COFF image accessed through the ImageRead service in the ImageContext
> 
>     structure is not a supported PE/COFF image type, then return RETURN_UNSUPPORTED.
> 
> @@ -752,7 +755,28 @@ PeCoffLoaderGetImageInfo (
>                 ImageContext->ImageSize += DebugEntry.SizeOfData;
> 
>               }
> 
>   
> 
> -            return RETURN_SUCCESS;
> 
> +            continue;
> 
> +          }
> 
> +
> 
> +          if (DebugEntry.Type == EFI_IMAGE_DEBUG_TYPE_EX_DLLCHARACTERISTICS) {
> 
> +            Size     = sizeof (EFI_IMAGE_DEBUG_EX_DLLCHARACTERISTICS_ENTRY);
> 
> +            ReadSize = sizeof (EFI_IMAGE_DEBUG_EX_DLLCHARACTERISTICS_ENTRY);
> 
> +            Status   = ImageContext->ImageRead (
> 
> +                                       ImageContext->Handle,
> 
> +                                       DebugEntry.FileOffset,
> 
> +                                       &Size,
> 
> +                                       &ImageContext->DllCharacteristicsEx
> 
> +                                       );
> 
> +            if (RETURN_ERROR (Status) || (Size != ReadSize)) {
> 
> +              ImageContext->ImageError = IMAGE_ERROR_IMAGE_READ;
> 
> +              if (Size != ReadSize) {
> 
> +                Status = RETURN_UNSUPPORTED;
> 
> +              }
> 
> +
> 
> +              return Status;
> 
> +            }
> 
> +
> 
> +            continue;
> 
>             }
> 
>           }
> 
>         }
> 

Re: [edk2-devel] [PATCH v2 16/17] MdePkg/PeCoffLib: Capture DLL characteristics fieldis in image context

[Ard Biesheuvel]

On Thu, 30 Mar 2023 at 16:45, Michael Kubacki
<mikuback@linux.microsoft.com> wrote:
>
> On 3/27/2023 7:01 AM, Ard Biesheuvel wrote:
> > When loading a PE/COFF image, capture the DLL characteristics fields of
> > the header into our image context structure so we can refer to them when
> > mapping the image.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >   MdePkg/Include/IndustryStandard/PeImage.h | 13 +++++-
> >   MdePkg/Include/Library/PeCoffLib.h        |  6 +++
> >   MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 46 +++++++++++++++-----
> >   3 files changed, 53 insertions(+), 12 deletions(-)
> >
> > diff --git a/MdePkg/Include/IndustryStandard/PeImage.h b/MdePkg/Include/IndustryStandard/PeImage.h
> > index dd4cc25483bc4bcf..a5b9b848ed391f98 100644
> > --- a/MdePkg/Include/IndustryStandard/PeImage.h
> > +++ b/MdePkg/Include/IndustryStandard/PeImage.h
> > @@ -625,7 +625,8 @@ typedef struct {
> >     UINT32    FileOffset;  ///< The file pointer to the debug data.
> >
> >   } EFI_IMAGE_DEBUG_DIRECTORY_ENTRY;
> >
> >
> >
> > -#define EFI_IMAGE_DEBUG_TYPE_CODEVIEW  2    ///< The Visual C++ debug information.
> >
> > +#define EFI_IMAGE_DEBUG_TYPE_CODEVIEW               2    ///< The Visual C++ debug information.
> >
> > +#define EFI_IMAGE_DEBUG_TYPE_EX_DLLCHARACTERISTICS  20
> >
> >
> >
> >   ///
> >
> >   /// Debug Data Structure defined in Microsoft C++.
> >
> > @@ -669,6 +670,16 @@ typedef struct {
> >     //
> >
> >   } EFI_IMAGE_DEBUG_CODEVIEW_MTOC_ENTRY;
> >
> >
> >
> > +///
> >
> > +/// Extended DLL Characteristics
> >
> > +///
> >
> > +#define EFI_IMAGE_DLLCHARACTERISTICS_EX_CET_COMPAT          0x0001
> >
> > +#define EFI_IMAGE_DLLCHARACTERISTICS_EX_FORWARD_CFI_COMPAT  0x0040
> >
> > +
> >
> > +typedef struct {
> >
> > +  UINT16    DllCharacteristicsEx;
> >
> > +} EFI_IMAGE_DEBUG_EX_DLLCHARACTERISTICS_ENTRY;
> >
> > +
> >
> >   ///
> >
> >   /// Resource format.
> >
> >   ///
> >
> > diff --git a/MdePkg/Include/Library/PeCoffLib.h b/MdePkg/Include/Library/PeCoffLib.h
> > index b45879453785c77d..d21c932076c072f6 100644
> > --- a/MdePkg/Include/Library/PeCoffLib.h
> > +++ b/MdePkg/Include/Library/PeCoffLib.h
> > @@ -171,6 +171,12 @@ typedef struct {
> >     ///
> >
> >     UINT16                      ImageType;
> >
> >     ///
> >
> > +  /// Set by PeCoffLoaderGetImageInfo() to the DLL flags stored in the PE/COFF header and
> >
> > +  /// in the DllCharacteristicsEx debug table.
> >
> > +  ///
> >
> > +  UINT16                      DllCharacteristics;
> >
> > +  UINT16                      DllCharacteristicsEx;
> >
> I know DllCharacteristics has a size of 2 in the spec, but the
> DllCharacteristicsEx is defined as 4 bytes. I will try to get a spec
> update to clarify this.
>

Thanks. I'll change this to 4 in the next respin.

Re: [edk2-devel] [PATCH v2 17/17] MdeModulePkg: Enable forward edge CFI in mem attributes table

[osde]

Turns out my old email was getting sent to a lot of folks spam, so 
resending with hopefully a better email...

On 3/27/2023 4:01 AM, Ard Biesheuvel wrote:
> 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.
> 
> Given that the PE/COFF spec now defines a DllCharacteristicsEx flag that
> indicates whether or not a loaded image is compatible with this, we can
> wire this up to the flag in the memory attributes table, and set it if
> all loaded runtime image are compatible with it.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>   MdeModulePkg/Core/Dxe/DxeMain.h                    |  2 ++
>   MdeModulePkg/Core/Dxe/Image/Image.c                | 10 ++++++++++
>   MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c |  8 +++++++-
>   3 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
> index 815a6b4bd844a452..43daa037be441150 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/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
> index 8704ebea9a7c88c0..9dbfb2a1fad22ced 100644
> --- a/MdeModulePkg/Core/Dxe/Image/Image.c
> +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
> @@ -1399,6 +1399,16 @@ CoreLoadImageCommon (
>       CoreNewDebugImageInfoEntry (EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL, &Image->Info, Image->Handle);
> 
>     }
> 
>   
> 
> +  //
> 
> +  // Check whether we are loading a runtime image that lacks support for
> 
> +  // IBT/BTI landing pads.
> 
> +  //
> 
> +  if ((Image->ImageContext.ImageCodeMemoryType == EfiRuntimeServicesCode) &&
> 
> +      ((Image->ImageContext.DllCharacteristicsEx & EFI_IMAGE_DLLCHARACTERISTICS_EX_FORWARD_CFI_COMPAT) == 0))
> 
> +  {
> 
> +    gMemoryAttributesTableForwardCfi = FALSE;
> 
> +  }

If I understand this correctly, we are disabling Forward CFI if we 
attempt to load any runtime images that don't support it. Would it make
sense to have a PCD to determine whether we strictly enforce
Forward CFI (i.e. don't load this incompatible image) in such a case? We
have a similar option for non-NX_COMPAT images.

Thanks,
Oliver

> 
> +
> 
>     //
> 
>     // 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 e079213711875f89..fd127ee167e1ac9a 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  = TRUE;
> 
>   
> 
>   /**
> 
>     Install MemoryAttributesTable.
> 
> @@ -182,7 +183,12 @@ 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));
> 

Re: [edk2-devel] [PATCH v2 17/17] MdeModulePkg: Enable forward edge CFI in mem attributes table

[Ard Biesheuvel]

On Mon, 3 Apr 2023 at 17:48, Oliver Smith-Denny
<osde@linux.microsoft.com> wrote:
>
> Turns out my old email was getting sent to a lot of folks spam, so
> resending with hopefully a better email...
>
> On 3/27/2023 4:01 AM, Ard Biesheuvel wrote:
> > 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.
> >
> > Given that the PE/COFF spec now defines a DllCharacteristicsEx flag that
> > indicates whether or not a loaded image is compatible with this, we can
> > wire this up to the flag in the memory attributes table, and set it if
> > all loaded runtime image are compatible with it.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >   MdeModulePkg/Core/Dxe/DxeMain.h                    |  2 ++
> >   MdeModulePkg/Core/Dxe/Image/Image.c                | 10 ++++++++++
> >   MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c |  8 +++++++-
> >   3 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
> > index 815a6b4bd844a452..43daa037be441150 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/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
> > index 8704ebea9a7c88c0..9dbfb2a1fad22ced 100644
> > --- a/MdeModulePkg/Core/Dxe/Image/Image.c
> > +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
> > @@ -1399,6 +1399,16 @@ CoreLoadImageCommon (
> >       CoreNewDebugImageInfoEntry (EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL, &Image->Info, Image->Handle);
> >
> >     }
> >
> >
> >
> > +  //
> >
> > +  // Check whether we are loading a runtime image that lacks support for
> >
> > +  // IBT/BTI landing pads.
> >
> > +  //
> >
> > +  if ((Image->ImageContext.ImageCodeMemoryType == EfiRuntimeServicesCode) &&
> >
> > +      ((Image->ImageContext.DllCharacteristicsEx & EFI_IMAGE_DLLCHARACTERISTICS_EX_FORWARD_CFI_COMPAT) == 0))
> >
> > +  {
> >
> > +    gMemoryAttributesTableForwardCfi = FALSE;
> >
> > +  }
>
> If I understand this correctly, we are disabling Forward CFI if we
> attempt to load any runtime images that don't support it. Would it make
> sense to have a PCD to determine whether we strictly enforce
> Forward CFI (i.e. don't load this incompatible image) in such a case? We
> have a similar option for non-NX_COMPAT images.
>

These changes only affect what the OS sees, and if the OS wants to
implement a certain policy around this, it is free to do so. I don't
think this belongs in the firmware though,

*However*, if/when we wire up forward CFI enforcement at boot time, it
would be appropriate to have a configurable policy around this, and
reject 3rd party images that do not implement forward CFI if the
firmware is configured for strict enforcement.

I intend to look into that next, but given how tedious and painful it
is to get changes reviewed, I'm not sure this will be anytime soon.

Re: [edk2-devel] [PATCH v2 17/17] MdeModulePkg: Enable forward edge CFI in mem attributes table

[Oliver Smith-Denny]

On 4/4/2023 3:41 AM, Ard Biesheuvel wrote:
> On Mon, 3 Apr 2023 at 17:48, Oliver Smith-Denny
> <osde@linux.microsoft.com> wrote:
>>
>> Turns out my old email was getting sent to a lot of folks spam, so
>> resending with hopefully a better email...
>>
>> On 3/27/2023 4:01 AM, Ard Biesheuvel wrote:
>>> 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.
>>>
>>> Given that the PE/COFF spec now defines a DllCharacteristicsEx flag that
>>> indicates whether or not a loaded image is compatible with this, we can
>>> wire this up to the flag in the memory attributes table, and set it if
>>> all loaded runtime image are compatible with it.
>>>
>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>> ---
>>>    MdeModulePkg/Core/Dxe/DxeMain.h                    |  2 ++
>>>    MdeModulePkg/Core/Dxe/Image/Image.c                | 10 ++++++++++
>>>    MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c |  8 +++++++-
>>>    3 files changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
>>> index 815a6b4bd844a452..43daa037be441150 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/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
>>> index 8704ebea9a7c88c0..9dbfb2a1fad22ced 100644
>>> --- a/MdeModulePkg/Core/Dxe/Image/Image.c
>>> +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
>>> @@ -1399,6 +1399,16 @@ CoreLoadImageCommon (
>>>        CoreNewDebugImageInfoEntry (EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL, &Image->Info, Image->Handle);
>>>
>>>      }
>>>
>>>
>>>
>>> +  //
>>>
>>> +  // Check whether we are loading a runtime image that lacks support for
>>>
>>> +  // IBT/BTI landing pads.
>>>
>>> +  //
>>>
>>> +  if ((Image->ImageContext.ImageCodeMemoryType == EfiRuntimeServicesCode) &&
>>>
>>> +      ((Image->ImageContext.DllCharacteristicsEx & EFI_IMAGE_DLLCHARACTERISTICS_EX_FORWARD_CFI_COMPAT) == 0))
>>>
>>> +  {
>>>
>>> +    gMemoryAttributesTableForwardCfi = FALSE;
>>>
>>> +  }
>>
>> If I understand this correctly, we are disabling Forward CFI if we
>> attempt to load any runtime images that don't support it. Would it make
>> sense to have a PCD to determine whether we strictly enforce
>> Forward CFI (i.e. don't load this incompatible image) in such a case? We
>> have a similar option for non-NX_COMPAT images.
>>
> 
> These changes only affect what the OS sees, and if the OS wants to
> implement a certain policy around this, it is free to do so. I don't
> think this belongs in the firmware though,
> 
> *However*, if/when we wire up forward CFI enforcement at boot time, it
> would be appropriate to have a configurable policy around this, and
> reject 3rd party images that do not implement forward CFI if the
> firmware is configured for strict enforcement.
> 
> I intend to look into that next, but given how tedious and painful it
> is to get changes reviewed, I'm not sure this will be anytime soon.

I see, thanks for the explanation, makes sense to me.

For the patchset (sorry, with the email change I don't have the top 
level entry): Reviewed-by: Oliver Smith-Denny <osde@linux.microsoft.com>

> 
> 
> 
> 

Re: [edk2-devel] [PATCH v2 17/17] MdeModulePkg: Enable forward edge CFI in mem attributes table

[Ard Biesheuvel]

On Tue, 4 Apr 2023 at 17:00, Oliver Smith-Denny
<osde@linux.microsoft.com> wrote:
>
> On 4/4/2023 3:41 AM, Ard Biesheuvel wrote:
> > On Mon, 3 Apr 2023 at 17:48, Oliver Smith-Denny
> > <osde@linux.microsoft.com> wrote:
> >>
> >> Turns out my old email was getting sent to a lot of folks spam, so
> >> resending with hopefully a better email...
> >>
> >> On 3/27/2023 4:01 AM, Ard Biesheuvel wrote:
> >>> 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.
> >>>
> >>> Given that the PE/COFF spec now defines a DllCharacteristicsEx flag that
> >>> indicates whether or not a loaded image is compatible with this, we can
> >>> wire this up to the flag in the memory attributes table, and set it if
> >>> all loaded runtime image are compatible with it.
> >>>
> >>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >>> ---
> >>>    MdeModulePkg/Core/Dxe/DxeMain.h                    |  2 ++
> >>>    MdeModulePkg/Core/Dxe/Image/Image.c                | 10 ++++++++++
> >>>    MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c |  8 +++++++-
> >>>    3 files changed, 19 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
> >>> index 815a6b4bd844a452..43daa037be441150 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/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
> >>> index 8704ebea9a7c88c0..9dbfb2a1fad22ced 100644
> >>> --- a/MdeModulePkg/Core/Dxe/Image/Image.c
> >>> +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
> >>> @@ -1399,6 +1399,16 @@ CoreLoadImageCommon (
> >>>        CoreNewDebugImageInfoEntry (EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL, &Image->Info, Image->Handle);
> >>>
> >>>      }
> >>>
> >>>
> >>>
> >>> +  //
> >>>
> >>> +  // Check whether we are loading a runtime image that lacks support for
> >>>
> >>> +  // IBT/BTI landing pads.
> >>>
> >>> +  //
> >>>
> >>> +  if ((Image->ImageContext.ImageCodeMemoryType == EfiRuntimeServicesCode) &&
> >>>
> >>> +      ((Image->ImageContext.DllCharacteristicsEx & EFI_IMAGE_DLLCHARACTERISTICS_EX_FORWARD_CFI_COMPAT) == 0))
> >>>
> >>> +  {
> >>>
> >>> +    gMemoryAttributesTableForwardCfi = FALSE;
> >>>
> >>> +  }
> >>
> >> If I understand this correctly, we are disabling Forward CFI if we
> >> attempt to load any runtime images that don't support it. Would it make
> >> sense to have a PCD to determine whether we strictly enforce
> >> Forward CFI (i.e. don't load this incompatible image) in such a case? We
> >> have a similar option for non-NX_COMPAT images.
> >>
> >
> > These changes only affect what the OS sees, and if the OS wants to
> > implement a certain policy around this, it is free to do so. I don't
> > think this belongs in the firmware though,
> >
> > *However*, if/when we wire up forward CFI enforcement at boot time, it
> > would be appropriate to have a configurable policy around this, and
> > reject 3rd party images that do not implement forward CFI if the
> > firmware is configured for strict enforcement.
> >
> > I intend to look into that next, but given how tedious and painful it
> > is to get changes reviewed, I'm not sure this will be anytime soon.
>
> I see, thanks for the explanation, makes sense to me.
>
> For the patchset (sorry, with the email change I don't have the top
> level entry): Reviewed-by: Oliver Smith-Denny <osde@linux.microsoft.com>
>

Thanks!