[COFF] Mark images with no exception handlers for /safeseh

[rnk]

Summary

DLLs and executables with no exception handlers need to be marked with
IMAGE_DLL_CHARACTERISTICS_NO_SEH, even if they have a load config.

Discovered here when building Chromium with LLD on Windows:
https://crbug.com/833951

[rnk]

@mstorsjo will this be a problem for mingw? Does the mingw CRT provide a load config for each image? This could be an undesirable behavior change if mingw does not provide a load config, and the user links in a 32-bit object file that has exception handlers that they don't actually need for program correctness, but now the DLL is marked as not using safe SEH.

[mstorsjo]

In D45778#1071192, @rnk wrote:

@mstorsjo will this be a problem for mingw? Does the mingw CRT provide a load config for each image?

I have no idea what a load config is - what should I look for and where?

This could be an undesirable behavior change if mingw does not provide a load config, and the user links in a 32-bit object file that has exception handlers that they don't actually need for program correctness, but now the DLL is marked as not using safe SEH.

Fwiw, no mingw setups use SEH for exceptions on i386 so far - it's only used on x86_64. On i386, it's either dwarf or sjlj.

What effect does it have if a DLL is marked as not using safe SEH?

[rnk]

• Set the flag if there are handlers but no load config

[rnk]

In D45778#1071211, @mstorsjo wrote:

What effect does it have if a DLL is marked as not using safe SEH?

If it also lacks a load config, it is more easily expoitable. If a DLL doesn't have a IMAGE_LOAD_CONFIG_DIRECTORY64 and it doesn't have the "no SEH" flag set, then any address in that DLL can be used as an exception handler. For 32-bit, exception handler function pointers are stored on the stack, so only a stack buffer overrun followed by an exception is needed to start an exploit chain.

After thinking that through, I think we should adjust the check to always add the "no SEH" flag if there is no load config. That's the safe thing to do.

[rnk]

• remove tab

[ruiu]

lld/COFF/Writer.cpp

833–834 ↗

(On Diff #142986)

Maybe we should move these conditions to createSEHTable and make it a Config member (such as Config->SEH?)

[rnk]

lld/COFF/Writer.cpp

833–834 ↗

(On Diff #142986)

That makes sense, but I wouldn't want to put mutable state on Config. We can just put it on Writer.

[rnk]

• move logic to createSEHTable

[ruiu]

LGTM

lld/COFF/Writer.cpp

830 ↗

(On Diff #143002)

Maybe you should move I386 to createSEHTable as well?

[rnk]

• fix hello32.test: default to marking image as "no seh", then remove it if we record handlers

[rnk]

• fix boolean logic thinko

[rnk]

lld/COFF/Writer.cpp

830 ↗

(On Diff #143002)

I found a way to do this that I like.

lld/trunk/COFF/Writer.cpp

private:
 
   std::unique_ptr<FileOutputBuffer> &Buffer;
   std::vector<OutputSection *> OutputSections;
   std::vector<char> Strtab;
   std::vector<llvm::object::coff_symbol16> OutputSymtab;
   IdataContents Idata;
   DelayLoadContents DelayIdata;
   EdataContents Edata;
-  RVATableChunk *GuardFidsTable = nullptr;
-  RVATableChunk *SEHTable = nullptr;
+  bool SetNoSEHCharacteristic = false;
 
   DebugDirectoryChunk *DebugDirectory = nullptr;
   std::vector<Chunk *> DebugRecords;
   CVDebugRecordChunk *BuildId = nullptr;
   Optional<codeview::DebugInfo> PreviousBuildId;
   ArrayRef<uint8_t> SectionTable;
 
   uint64_t FileSize;
template <typename PEHeaderTy> void Writer::writeHeader() {
   if (!Config->AllowBind)
     PE->DLLCharacteristics |= IMAGE_DLL_CHARACTERISTICS_NO_BIND;
   if (Config->NxCompat)
     PE->DLLCharacteristics |= IMAGE_DLL_CHARACTERISTICS_NX_COMPAT;
   if (!Config->AllowIsolation)
     PE->DLLCharacteristics |= IMAGE_DLL_CHARACTERISTICS_NO_ISOLATION;
   if (Config->GuardCF != GuardCFLevel::Off)
     PE->DLLCharacteristics |= IMAGE_DLL_CHARACTERISTICS_GUARD_CF;
-  if (Config->Machine == I386 && !SEHTable &&
-      !Symtab->findUnderscore("_load_config_used"))
+  if (SetNoSEHCharacteristic)
     PE->DLLCharacteristics |= IMAGE_DLL_CHARACTERISTICS_NO_SEH;
   if (Config->TerminalServerAware)
     PE->DLLCharacteristics |= IMAGE_DLL_CHARACTERISTICS_TERMINAL_SERVER_AWARE;
   PE->NumberOfRvaAndSize = NumberfOfDataDirectory;
   if (TextSec->getVirtualSize()) {
     PE->BaseOfCode = TextSec->getRVA();
     PE->SizeOfCode = TextSec->getRawSize();
   }

 
 void Writer::openFile(StringRef Path) {
   Buffer = CHECK(
       FileOutputBuffer::create(Path, FileSize, FileOutputBuffer::F_executable),
       "failed to open " + Path);
 }
 
 void Writer::createSEHTable() {
+  // Set the no SEH characteristic on x86 binaries unless we find exception
+  // handlers.
+  SetNoSEHCharacteristic = true;
+
   SymbolRVASet Handlers;
   for (ObjFile *File : ObjFile::Instances) {
     // FIXME: We should error here instead of earlier unless /safeseh:no was
     // passed.
     if (!File->hasSafeSEH())
       return;
 
     markSymbolsForRVATable(File, File->getSXDataChunks(), Handlers);
   }
 
+  // Remove the "no SEH" characteristic if all object files were built with
+  // safeseh, we found some exception handlers, and there is a load config in
+  // the object.
+  SetNoSEHCharacteristic =
+      Handlers.empty() || !Symtab->findUnderscore("_load_config_used");
+
   maybeAddRVATable(std::move(Handlers), "__safe_se_handler_table",
                    "__safe_se_handler_count");
 }
 
 // Add a symbol to an RVA set. Two symbols may have the same RVA, but an RVA set
 // cannot contain duplicates. Therefore, the set is uniqued by Chunk and the
 // symbol's offset into that Chunk.
 static void addSymbolToRVASet(SymbolRVASet &RVASet, Defined *S) {

lld/trunk/test/COFF/safeseh-notable.s

+# RUN: llvm-mc -triple i686-windows-msvc %s -filetype=obj -o %t.obj
+# RUN: lld-link %t.obj -safeseh -out:%t.exe -entry:main
+# RUN: llvm-readobj -file-headers %t.exe | FileCheck %s
+
+# This object lacks a _load_config_used global, so we set
+# IMAGE_DLL_CHARACTERISTICS_NO_SEH even though there is an exception handler.
+# This is a more secure default. If someone wants to use a CRT without a load
+# config and they want to use 32-bit SEH, they will need to provide a
+# safeseh-compatible load config.
+
+# CHECK-LABEL: Characteristics [
+# CHECK:   IMAGE_DLL_CHARACTERISTICS_NO_SEH
+# CHECK: ]
+
+# CHECK-LABEL:  DataDirectory {
+# CHECK:    LoadConfigTableRVA: 0x0
+# CHECK:    LoadConfigTableSize: 0x0
+# CHECK:  }
+
+# CHECK-NOT: LoadConfig
+# CHECK-NOT: SEHTable
+
+        .def     @feat.00;
+        .scl    3;
+        .type   0;
+        .endef
+        .globl  @feat.00
+@feat.00 = 1
+
+        .text
+        .def     _main; .scl    2; .type   32; .endef
+        .globl  _main
+_main:
+        pushl $_my_handler
+        movl $42, %eax
+        popl %ecx
+        ret
+
+        .def     _my_handler; .scl    3; .type   32; .endef
+_my_handler:
+        ret
+
+.safeseh _my_handler

lld/trunk/test/COFF/safeseh.s

 # RUN: llvm-mc -triple i686-windows-msvc %s -filetype=obj -o %t.obj
 # RUN: lld-link %t.obj -safeseh -out:%t.exe -opt:noref -entry:main
 # RUN: llvm-readobj -coff-basereloc -coff-load-config -file-headers %t.exe | FileCheck %s --check-prefix=CHECK-NOGC
 # RUN: lld-link %t.obj -safeseh -out:%t.exe -opt:ref -entry:main
 # RUN: llvm-readobj -coff-basereloc -coff-load-config -file-headers %t.exe | FileCheck %s --check-prefix=CHECK-GC
 
 # __safe_se_handler_table needs to be relocated against ImageBase.
 # check that the relocation is present.
+#
 # CHECK-NOGC-NOT: IMAGE_DLL_CHARACTERISTICS_NO_SEH
 # CHECK-NOGC: BaseReloc [
 # CHECK-NOGC:   Entry {
 # CHECK-NOGC:     Type: HIGHLOW
 # CHECK-NOGC: LoadConfig [
 # CHECK-NOGC:   Size: 0x48
 # CHECK-NOGC:   SEHandlerTable: 0x402048
 # CHECK-NOGC:   SEHandlerCount: 1
 # CHECK-NOGC: ]
 # CHECK-NOGC: SEHTable [
 # CHECK-NOGC-NEXT:   0x401006
 # CHECK-NOGC-NEXT: ]
 
-# Without the SEH table, the address is absolute, so check that we do
-# not have a relocation for it.
-# CHECK-GC-NOT: IMAGE_DLL_CHARACTERISTICS_NO_SEH
+# If we enable GC, the exception handler should be removed, and we should add
+# the DLL characteristic flag that indicates that there are no exception
+# handlers in this DLL. The exception handler table in the load config should
+# be empty and there should be no relocations for it.
+#
+# CHECK-GC: Characteristics [
+# CHECK-GC:   IMAGE_DLL_CHARACTERISTICS_NO_SEH
+# CHECK-GC: ]
 # CHECK-GC: BaseReloc [
 # CHECK-GC-NEXT: ]
 # CHECK-GC: LoadConfig [
 # CHECK-GC:   Size: 0x48
 # CHECK-GC:   SEHandlerTable: 0x0
 # CHECK-GC:   SEHandlerCount: 0
 # CHECK-GC: ]
 # CHECK-GC-NOT: SEHTable