lld-link: align sections to 16 bytes if referenced from the gfids table

[inglorion]

Summary

Control flow guard works best when targets it checks are 16-byte aligned.
Microsoft's link.exe helps ensure this by aligning code from sections
that are referenced from the gfids table to 16 bytes when linking with
-guard:cf, even if the original section specifies a smaller alignment.
This change implements that behavior in lld-link.

See https://crbug.com/857012 for more details.

[pcc]

lld/COFF/Writer.cpp

1050 ↗

(On Diff #153218)

I think this can be simplified: the alignment should always be a power of 2.

[inglorion]

lld/COFF/Writer.cpp

1050 ↗

(On Diff #153218)

So just if (Alignment < 16) Alignment = 16 would do it?

[pcc]

lld/COFF/Writer.cpp

1050 ↗

(On Diff #153218)

Yes.

[inglorion]

simplified as suggested by @pcc (thanks!)

[ruiu]

LGTM

lld/COFF/Writer.cpp

1049 ↗

(On Diff #153228)

Please replace auto with the real type.

[hans]

Nice! I like how simple this turned out.

From the commit message:
PR857012 is the Chromium bug number. LLVM hasn't had that many bugs :-p
Maybe just change to "See also https://crbug.com/857012" for details or something like that.

lld/test/COFF/guardcf-align.s

11 ↗

(On Diff #153228)

It would be good to have a comment about the intention of these checks, since it's not obvious from a quick read.

The regexes are a little tricky to read. Will there be more entries in the gfid than the one for foo? If not, could we just check that there's only one entry and that it's aligned?

Also it would be nice to check that it's really foo that's getting aligned. Maybe we could generate a map file (/lldmap:file) and check foo's alignment there?

Oh, and could we be getting lucky and foo is aligned because it's first in the segment or something? Maybe there should be more than one function that we check the alignment of just to be sure?

[inglorion]

lld/test/COFF/guardcf-align.s

11 ↗

(On Diff #153228)

I've added a comment to explain what we're looking for with the regexes.

The table actually contains multiple entries. We're checking that all of them end in 0 (which means they're 16-byte aligned). We could constrain the check to only foo (using the map, as you suggest), but really we want all of them to be aligned. foo is just a symbol in a section specifically crafted to cause alignment problems.

You're right that foo could end up being aligned by accident, but it's unlikely that all entries will be aligned by accident. In fact, they aren't, without the patch.

[hans]

lld/test/COFF/guardcf-align.s

11 ↗

(On Diff #153228)

Okay, sounds good.

[inglorion]

improvements suggested by @ruiu and @hans (thanks!)

COFF/Writer.cpp

if (File->hasGuardCF()) {
       markSymbolsWithRelocations(File, AddressTakenSyms);
     }
   }
 
   // Mark the image entry as address-taken.
   if (Config->Entry)
     addSymbolToRVASet(AddressTakenSyms, cast<Defined>(Config->Entry));
 
+  // Ensure sections referenced in the gfid table are 16-byte aligned.
+  for (const ChunkAndOffset &C : AddressTakenSyms)
+    if (C.InputChunk->Alignment < 16)
+      C.InputChunk->Alignment = 16;
+
   maybeAddRVATable(std::move(AddressTakenSyms), "__guard_fids_table",
                    "__guard_fids_count");
 
   // Add the longjmp target table unless the user told us not to.
   if (Config->GuardCF == GuardCFLevel::Full)
     maybeAddRVATable(std::move(LongJmpTargets), "__guard_longjmp_table",
                      "__guard_longjmp_count");
 

test/COFF/Inputs/guardcf-align-foobar.yaml

+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:
+  - Name:            .text.foo
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+    Alignment:       1
+    SectionData:     31C0C3
+  - Name:            .text.bar
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+    Alignment:       1
+    SectionData:     FFE1
+symbols:
+  - Name:            .text.foo
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          3
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        3963538403
+      Number:          1
+  - Name:            .text.bar
+    Value:           0
+    SectionNumber:   2
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          2
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        1143549852
+      Number:          2
+  - Name:            foo
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            bar
+    Value:           0
+    SectionNumber:   2
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+...

test/COFF/guardcf-align.s

+# RUN: llvm-mc -triple x86_64-windows-msvc -filetype=obj -o %t.obj %s
+# RUN: yaml2obj < %p/Inputs/guardcf-align-foobar.yaml \
+# RUN:     > %T/guardcf-align-foobar.obj
+# RUN: lld-link -out:%T/guardcf-align.exe -entry:main -guard:cf \
+# RUN:     %t.obj %T/guardcf-align-foobar.obj
+# RUN: llvm-readobj -coff-load-config %T/guardcf-align.exe | FileCheck %s
+
+# Check that the gfids table contains at least one entry that ends in 0
+# and no entries that end in something other than 0.
+# CHECK: GuardFidTable [
+# CHECK-NOT: 0x{{[0-9A-Fa-f]+[^0]$}}
+# CHECK: 0x{{[0-9A-Fa-f]+0$}}
+# CHECK-NOT: 0x{{[0-9A-Fa-f]+[^0]$}}
+# CHECK: ]
+
+# @feat.00 and _load_config_used to indicate we have gfids.
+        .def     @feat.00;
+        .scl    3;
+        .type   0;
+        .endef
+        .globl  @feat.00
+@feat.00 = 0x801
+
+        .section .rdata,"dr"
+.globl _load_config_used
+_load_config_used:
+        .long 256
+        .fill 124, 1, 0
+        .quad __guard_fids_table
+        .quad __guard_fids_count
+        .long __guard_flags
+        .fill 128, 1, 0
+
+# Functions that are called indirectly.
+        .section        .gfids$y,"dr"
+        .symidx foo
+
+
+        .section .text,"rx"
+        .global main
+main:
+        movq foo, %rcx
+        xorq %rax, %rax
+        callq bar
+        retq