[LLD][COFF] Add load config checks to warn if incorrect for CFGuard

[alvinhochun]

Summary

Control Flow Guard requires specific flags and VA's be included in the
load config directory to be functional. In case CFGuard is enabled via
linker flags, we can check to make sure this is the case and give the
user a warning if otherwise.

MSVC provides a proper _load_config_used by default, so this is more
relevant for the MinGW target in which current versions of mingw-w64
does not provide this symbol.

The checks (only if CFGuard is enabled) include:

• The _load_config_used struct shall exist.
• Alignment of the _load_config_used struct (shall be aligned to pointer size.)
• The _load_config_used struct shall be large enough to contain the required fields.
• The values of the following fields are checked against the expected values:
  • GuardCFFunctionTable
  • GuardCFFunctionCount
  • GuardFlags
  • GuardAddressTakenIatEntryTable
  • GuardAddressTakenIatEntryCount
  • GuardLongJumpTargetTable
  • GuardLongJumpTargetCount
  • GuardEHContinuationTable
  • GuardEHContinuationCount

[mstorsjo]

This looks like a good idea IMO! Especially the check for the basic condition that _load_config_used must exist if this flag is enabled; I was surprised to see that you don't get any diagnostics for this combination today.

Other than that, the rest of the checks are certainly useful when implementing these things. (I didn't check all the details of what is verified.) On the other hand, since LLD is quite focused on performance, LLD omits pedantic checks of all inputs where it's relevant for performance (i.e. it's considered ok to not handle corrupted inputs cleanly) - but these checks shouldn't really have any performance impact I think, so they could be considered fine in that aspect.

lld/COFF/Writer.cpp

2131

Do you think it would be worthwhile to check that the section chunk itself also has got the right alignment? Since even if the section doesn't flag the right required alignment, we can accidentally end up on a correctly aligned RVA anyway.

[alvinhochun]

lld/COFF/Writer.cpp

2131

Initially I was only concerned about giving the user some insight to help with the mysterious "This app can't run on your PC" message that appears when you try to run the executable. My thinking is that if it happens to end up aligned correctly then the final executable will still work and the warning isn't needed...

A check for the actual alignment of the section chunk would be nice to have, but I suspect it may be a bit tricky to implement correctly.

[alvinhochun]

Moved some code around and added a check for the actual alignment of the section chunk

[rnk]

This seems like a nice QoI improvement!

[chapuni]

lld/test/COFF/guard-warnings.s

5

It fails if x86 is not configured.

[mstorsjo]

lld/test/COFF/guard-warnings.s

5

Fixed, sorry about that!

lld/COFF/Writer.cpp

private:
   size_t addEntryToStringTable(StringRef str);
 
   OutputSection *findSection(StringRef name);
   void addBaserels();
   void addBaserelBlocks(std::vector<Baserel> &v);
 
   uint32_t getSizeOfInitializedData();
 
+  void checkLoadConfig();
+  template <typename T> void checkLoadConfigGuardData(const T *loadConfig);
+
   std::unique_ptr<FileOutputBuffer> &buffer;
   std::map<PartialSectionKey, PartialSection *> partialSections;
   std::vector<char> strtab;
   std::vector<llvm::object::coff_symbol16> outputSymtab;
   IdataContents idata;
   Chunk *importTableStart = nullptr;
   uint64_t importTableSize = 0;
   Chunk *edataStart = nullptr;
void Writer::run() {
 
   openFile(config->outputFile);
   if (config->is64()) {
     writeHeader<pe32plus_header>();
   } else {
     writeHeader<pe32_header>();
   }
   writeSections();
+  checkLoadConfig();
   sortExceptionTable();
 
   // Fix up the alignment in the TLS Directory's characteristic field,
   // if a specific alignment value is needed
   if (tlsAlignment)
     fixTlsAlignment();
 
   t1.stop();
object::coff_tls_directory64 *tlsDir =
         reinterpret_cast<object::coff_tls_directory64 *>(&secBuf[tlsOffset]);
     tlsDir->setAlignment(tlsAlignment);
   } else {
     object::coff_tls_directory32 *tlsDir =
         reinterpret_cast<object::coff_tls_directory32 *>(&secBuf[tlsOffset]);
     tlsDir->setAlignment(tlsAlignment);
   }
 }
+
+void Writer::checkLoadConfig() {
+  Symbol *sym = ctx.symtab.findUnderscore("_load_config_used");
+  auto *b = cast_if_present<DefinedRegular>(sym);
+  if (!b) {
+    if (config->guardCF != GuardCFLevel::Off)
+      warn("Control Flow Guard is enabled but '_load_config_used' is missing");
+    return;
+  }
+
+  OutputSection *sec = ctx.getOutputSection(b->getChunk());
+  uint8_t *buf = buffer->getBufferStart();
+  uint8_t *secBuf = buf + sec->getFileOff();
+  uint8_t *symBuf = secBuf + (b->getRVA() - sec->getRVA());
+  uint32_t expectedAlign = config->is64() ? 8 : 4;
+  if (b->getChunk()->getAlignment() < expectedAlign)
+    warn("'_load_config_used' is misaligned (expected alignment to be " +
+         Twine(expectedAlign) + " bytes, got " +
+         Twine(b->getChunk()->getAlignment()) + " instead)");
+  else if (!isAligned(Align(expectedAlign), b->getRVA()))
+    warn("'_load_config_used' is misaligned (RVA is 0x" +
+         Twine::utohexstr(b->getRVA()) + " not aligned to " +
+         Twine(expectedAlign) + " bytes)");
+
+  if (config->is64())
+    checkLoadConfigGuardData(
+        reinterpret_cast<const coff_load_configuration64 *>(symBuf));
+  else
+    checkLoadConfigGuardData(
+        reinterpret_cast<const coff_load_configuration32 *>(symBuf));
+}
+
+template <typename T>
+void Writer::checkLoadConfigGuardData(const T *loadConfig) {
+  size_t loadConfigSize = loadConfig->Size;
+
+#define RETURN_IF_NOT_CONTAINS(field)                                          \
+  if (loadConfigSize < offsetof(T, field) + sizeof(T::field)) {                \
+    warn("'_load_config_used' structure too small to include " #field);        \
+    return;                                                                    \
+  }
+
+#define IF_CONTAINS(field)                                                     \
+  if (loadConfigSize >= offsetof(T, field) + sizeof(T::field))
+
+#define CHECK_VA(field, sym)                                                   \
+  if (auto *s = dyn_cast<DefinedSynthetic>(ctx.symtab.findUnderscore(sym)))    \
+    if (loadConfig->field != config->imageBase + s->getRVA())                  \
+      warn(#field " not set correctly in '_load_config_used'");
+
+#define CHECK_ABSOLUTE(field, sym)                                             \
+  if (auto *s = dyn_cast<DefinedAbsolute>(ctx.symtab.findUnderscore(sym)))     \
+    if (loadConfig->field != s->getVA())                                       \
+      warn(#field " not set correctly in '_load_config_used'");
+
+  if (config->guardCF == GuardCFLevel::Off)
+    return;
+  RETURN_IF_NOT_CONTAINS(GuardFlags)
+  CHECK_VA(GuardCFFunctionTable, "__guard_fids_table")
+  CHECK_ABSOLUTE(GuardCFFunctionCount, "__guard_fids_count")
+  CHECK_ABSOLUTE(GuardFlags, "__guard_flags")
+  IF_CONTAINS(GuardAddressTakenIatEntryCount) {
+    CHECK_VA(GuardAddressTakenIatEntryTable, "__guard_iat_table")
+    CHECK_ABSOLUTE(GuardAddressTakenIatEntryCount, "__guard_iat_count")
+  }
+
+  if (!(config->guardCF & GuardCFLevel::LongJmp))
+    return;
+  RETURN_IF_NOT_CONTAINS(GuardLongJumpTargetCount)
+  CHECK_VA(GuardLongJumpTargetTable, "__guard_longjmp_table")
+  CHECK_ABSOLUTE(GuardLongJumpTargetCount, "__guard_longjmp_count")
+
+  if (!(config->guardCF & GuardCFLevel::EHCont))
+    return;
+  RETURN_IF_NOT_CONTAINS(GuardEHContinuationCount)
+  CHECK_VA(GuardEHContinuationTable, "__guard_eh_cont_table")
+  CHECK_ABSOLUTE(GuardEHContinuationCount, "__guard_eh_cont_count")
+
+#undef RETURN_IF_NOT_CONTAINS
+#undef IF_CONTAINS
+#undef CHECK_VA
+#undef CHECK_ABSOLUTE
+}

lld/test/COFF/guard-warnings.s

+# Make a DLL that exports exportfn1.
+# RUN: yaml2obj %p/Inputs/export.yaml -o %basename_t-exp.obj
+# RUN: lld-link /out:%basename_t-exp.dll /dll %basename_t-exp.obj /export:exportfn1 /implib:%basename_t-exp.lib
+# RUN: split-file %s %t
+# RUN: llvm-mc -triple x86_64-windows-msvc %t/main.s -filetype=obj -o %t/main.obj
+
+# RUN: lld-link %t/main.obj -guard:cf,longjmp,ehcont -out:%t-missing.exe -entry:main %basename_t-exp.lib 2>&1 | FileCheck %s --check-prefix=WARN_MISSING
+# WARN_MISSING: warning: Control Flow Guard is enabled but '_load_config_used' is missing
+
+# RUN: llvm-mc -triple x86_64-windows-msvc %t/loadcfg-invalid.s -filetype=obj -o %t/loadcfg-invalid.obj
+# RUN: lld-link %t/main.obj %t/loadcfg-invalid.obj -guard:cf,longjmp,ehcont -out:%t-invalid.exe -entry:main %basename_t-exp.lib 2>&1 | FileCheck %s --check-prefix=WARN_INVALID
+# WARN_INVALID:      warning: GuardCFFunctionTable not set correctly in '_load_config_used'
+# WARN_INVALID-NEXT: warning: GuardCFFunctionCount not set correctly in '_load_config_used'
+# WARN_INVALID-NEXT: warning: GuardFlags not set correctly in '_load_config_used'
+# WARN_INVALID-NEXT: warning: GuardAddressTakenIatEntryTable not set correctly in '_load_config_used'
+# WARN_INVALID-NEXT: warning: GuardAddressTakenIatEntryCount not set correctly in '_load_config_used'
+# WARN_INVALID-NEXT: warning: GuardLongJumpTargetTable not set correctly in '_load_config_used'
+# WARN_INVALID-NEXT: warning: GuardLongJumpTargetCount not set correctly in '_load_config_used'
+# WARN_INVALID-NEXT: warning: GuardEHContinuationTable not set correctly in '_load_config_used'
+# WARN_INVALID-NEXT: warning: GuardEHContinuationCount not set correctly in '_load_config_used'
+
+# RUN: llvm-mc -triple x86_64-windows-msvc %t/loadcfg-small112.s -filetype=obj -o %t/loadcfg-small112.obj
+# RUN: lld-link %t/main.obj %t/loadcfg-small112.obj -guard:cf,longjmp -out:%t-small112.exe -entry:main %basename_t-exp.lib 2>&1 | FileCheck %s --check-prefix=WARN_SMALL_112
+# WARN_SMALL_112: warning: '_load_config_used' structure too small to include GuardFlags
+
+# RUN: llvm-mc -triple x86_64-windows-msvc %t/loadcfg-small148.s -filetype=obj -o %t/loadcfg-small148.obj
+# RUN: lld-link %t/main.obj %t/loadcfg-small148.obj -guard:cf,longjmp -out:%t-small148.exe -entry:main %basename_t-exp.lib 2>&1 | FileCheck %s --check-prefix=WARN_SMALL_148
+# WARN_SMALL_148: warning: '_load_config_used' structure too small to include GuardLongJumpTargetCount
+
+# RUN: llvm-mc -triple x86_64-windows-msvc %t/loadcfg-small244.s -filetype=obj -o %t/loadcfg-small244.obj
+# RUN: lld-link %t/main.obj %t/loadcfg-small244.obj -guard:cf,longjmp,ehcont -out:%t-small244.exe -entry:main %basename_t-exp.lib 2>&1 | FileCheck %s --check-prefix=WARN_SMALL_244
+# WARN_SMALL_244: warning: '_load_config_used' structure too small to include GuardEHContinuationCount
+
+# RUN: llvm-mc -triple x86_64-windows-msvc %t/loadcfg-misaligned1.s -filetype=obj -o %t/loadcfg-misaligned1.obj
+# RUN: lld-link %t/main.obj %t/loadcfg-misaligned1.obj -guard:cf,longjmp,ehcont -out:%t-misaligned1.exe -entry:main %basename_t-exp.lib 2>&1 | FileCheck %s --check-prefix=WARN_ALIGN1
+# WARN_ALIGN1: warning: '_load_config_used' is misaligned (expected alignment to be 8 bytes, got 4 instead)
+
+# RUN: llvm-mc -triple x86_64-windows-msvc %t/loadcfg-misaligned2.s -filetype=obj -o %t/loadcfg-misaligned2.obj
+# RUN: lld-link %t/main.obj %t/loadcfg-misaligned2.obj -guard:cf,longjmp,ehcont -out:%t-misaligned2.exe -entry:main %basename_t-exp.lib 2>&1 | FileCheck %s --check-prefix=WARN_ALIGN2
+# WARN_ALIGN2: warning: '_load_config_used' is misaligned (RVA is 0x{{[0-9A-F]*}}2 not aligned to 8 bytes)
+
+# RUN: llvm-mc -triple x86_64-windows-msvc %t/loadcfg-full.s -filetype=obj -o %t/loadcfg-full.obj
+# RUN: lld-link %t/main.obj %t/loadcfg-full.obj -guard:cf,longjmp,ehcont -out:%t.exe -entry:main %basename_t-exp.lib 2>&1 | FileCheck %s --check-prefix=NOWARN --allow-empty
+# NOWARN-NOT: warning
+
+# Sanity check to make sure the no-warn version has the expected data.
+# RUN: llvm-readobj --file-headers --coff-load-config %t.exe | FileCheck --check-prefix=CHECK %s
+# CHECK: ImageBase: 0x140000000
+# CHECK: LoadConfig [
+# CHECK:   SEHandlerTable: 0x0
+# CHECK:   SEHandlerCount: 0
+# CHECK:   GuardCFCheckFunction: 0x0
+# CHECK:   GuardCFCheckDispatch: 0x0
+# CHECK:   GuardCFFunctionTable: 0x14000{{([0-9A-F]{4})}}
+# CHECK:   GuardCFFunctionCount: 1
+# CHECK:   GuardFlags [ (0x410500)
+# CHECK:     CF_FUNCTION_TABLE_PRESENT (0x400)
+# CHECK:     CF_INSTRUMENTED (0x100)
+# CHECK:     CF_LONGJUMP_TABLE_PRESENT (0x10000)
+# CHECK:     EH_CONTINUATION_TABLE_PRESENT (0x400000)
+# CHECK:   ]
+# CHECK:   GuardAddressTakenIatEntryTable: 0x14000{{([0-9A-F]{4})}}
+# CHECK:   GuardAddressTakenIatEntryCount: 1
+# CHECK:   GuardLongJumpTargetTable: 0x14000{{([0-9A-F]{4})}}
+# CHECK:   GuardLongJumpTargetCount: 1
+# CHECK:   GuardEHContinuationTable: 0x14000{{([0-9A-F]{4})}}
+# CHECK:   GuardEHContinuationCount: 1
+# CHECK: ]
+# CHECK-NEXT: GuardFidTable [
+# CHECK-NEXT:   0x14000{{([0-9A-F]{4})}}
+# CHECK-NEXT: ]
+# CHECK-NEXT: GuardIatTable [
+# CHECK-NEXT:   0x14000{{([0-9A-F]{4})}}
+# CHECK-NEXT: ]
+# CHECK-NEXT: GuardLJmpTable [
+# CHECK-NEXT:   0x14000{{([0-9A-F]{4})}}
+# CHECK-NEXT: ]
+# CHECK-NEXT: GuardEHContTable [
+# CHECK-NEXT:   0x14000{{([0-9A-F]{4})}}
+# CHECK-NEXT: ]
+
+
+#--- main.s
+
+# We need @feat.00 to have 0x4000 | 0x800 to indicate /guard:cf and /guard:ehcont.
+        .def     @feat.00;
+        .scl    3;
+        .type   0;
+        .endef
+        .globl  @feat.00
+@feat.00 = 0x4800
+        .def     main; .scl    2; .type   32; .endef
+        .globl  main                            # -- Begin function main
+        .p2align  4, 0x90
+main:
+        mov %eax, %eax
+  movq __imp_exportfn1(%rip), %rax
+        callq *%rax
+        nop
+# Fake setjmp target
+$cfgsj_main0:
+        mov %ebx, %ebx
+        nop
+# Fake ehcont target
+$ehgcr_0_1:
+        mov %ecx, %ecx
+        nop
+        retq
+                                        # -- End function
+        .section  .gfids$y,"dr"
+        .symidx main
+        .section  .giats$y,"dr"
+        .symidx __imp_exportfn1
+        .section  .gljmp$y,"dr"
+        .symidx $cfgsj_main0
+        .section  .gehcont$y,"dr"
+        .symidx $ehgcr_0_1
+        .addrsig_sym main
+        .addrsig_sym __imp_exportfn1
+        .section  .rdata,"dr"
+
+#--- loadcfg-invalid.s
+
+.globl _load_config_used
+        .p2align 3
+_load_config_used:
+        .long 312
+        .fill 308, 1, 0
+
+#--- loadcfg-small112.s
+
+.globl _load_config_used
+        .p2align 3
+_load_config_used:
+        .long 112
+        .fill 108, 1, 0
+
+#--- loadcfg-small148.s
+
+.globl _load_config_used
+        .p2align 3
+_load_config_used:
+        .long 148
+        .fill 124, 1, 0
+        .quad __guard_fids_table
+        .quad __guard_fids_count
+        .long __guard_flags
+
+#--- loadcfg-small244.s
+
+.globl _load_config_used
+        .p2align 3
+_load_config_used:
+        .long 244
+        .fill 124, 1, 0
+        .quad __guard_fids_table
+        .quad __guard_fids_count
+        .long __guard_flags
+        .fill 12, 1, 0
+        .quad __guard_iat_table
+        .quad __guard_iat_count
+        .quad __guard_longjmp_table
+        .quad __guard_longjmp_count
+        .fill 52, 1, 0          # Up to HotPatchTableOffset
+
+#--- loadcfg-misaligned1.s
+
+.globl _load_config_used
+        .fill 2, 1, 0           # offset by 2
+        .p2align 2              # then align to 0x04
+_load_config_used:
+        .long 312
+        .fill 124, 1, 0
+        .quad __guard_fids_table
+        .quad __guard_fids_count
+        .long __guard_flags
+        .fill 12, 1, 0
+        .quad __guard_iat_table
+        .quad __guard_iat_count
+        .quad __guard_longjmp_table
+        .quad __guard_longjmp_count
+        .fill 72, 1, 0
+        .quad __guard_eh_cont_table
+        .quad __guard_eh_cont_count
+        .fill 32, 1, 0
+
+#--- loadcfg-misaligned2.s
+
+.globl _load_config_used
+        .p2align 4              # align to 0x10
+        .fill 2, 1, 0           # then offset by 2
+_load_config_used:
+        .long 312
+        .fill 124, 1, 0
+        .quad __guard_fids_table
+        .quad __guard_fids_count
+        .long __guard_flags
+        .fill 12, 1, 0
+        .quad __guard_iat_table
+        .quad __guard_iat_count
+        .quad __guard_longjmp_table
+        .quad __guard_longjmp_count
+        .fill 72, 1, 0
+        .quad __guard_eh_cont_table
+        .quad __guard_eh_cont_count
+        .fill 32, 1, 0
+
+#--- loadcfg-full.s
+
+.globl _load_config_used
+        .p2align 3
+_load_config_used:
+        .long 312
+        .fill 124, 1, 0
+        .quad __guard_fids_table
+        .quad __guard_fids_count
+        .long __guard_flags
+        .fill 12, 1, 0
+        .quad __guard_iat_table
+        .quad __guard_iat_count
+        .quad __guard_longjmp_table
+        .quad __guard_longjmp_count
+        .fill 72, 1, 0
+        .quad __guard_eh_cont_table
+        .quad __guard_eh_cont_count
+        .fill 32, 1, 0