This is an archive of the discontinued LLVM Phabricator instance.

Differential D48690

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

Authored by inglorion on Jun 27 2018, 5:20 PM.

Details

Reviewers
ruiu
hans
thakis
zturner
Commits
rGc103156c606d: lld-link: align sections to 16 bytes if referenced from the gfids table
rL335864: lld-link: align sections to 16 bytes if referenced from the gfids table
rLLD335864: lld-link: align sections to 16 bytes if referenced from the gfids table
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.

Diff Detail

Event Timeline

inglorion created this revision.Jun 27 2018, 5:20 PM
pcc added a subscriber: pcc.Jun 27 2018, 5:24 PM
pcc added inline comments.
lld/COFF/Writer.cpp
1050

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

inglorion added inline comments.Jun 27 2018, 5:39 PM
lld/COFF/Writer.cpp
1050

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

pcc added inline comments.Jun 27 2018, 5:39 PM
lld/COFF/Writer.cpp
1050

Yes.

inglorion updated this revision to Diff 153228.Jun 27 2018, 6:03 PM

simplified as suggested by @pcc (thanks!)

ruiu accepted this revision.Jun 27 2018, 9:43 PM

LGTM

lld/COFF/Writer.cpp
1049

Please replace auto with the real type.

This revision is now accepted and ready to land.Jun 27 2018, 9:43 PM
hans added a comment.Jun 28 2018, 4:44 AM

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

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 marked 2 inline comments as done.Jun 28 2018, 8:20 AM
inglorion added inline comments.
lld/test/COFF/guardcf-align.s
11

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 accepted this revision.Jun 28 2018, 8:24 AM
hans added inline comments.
lld/test/COFF/guardcf-align.s
11

Okay, sounds good.

inglorion updated this revision to Diff 153328.Jun 28 2018, 8:24 AM
inglorion marked an inline comment as done.
inglorion edited the summary of this revision. (Show Details)
inglorion removed a subscriber: pcc.

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

Harbormaster completed remote builds in B19838: Diff 153328.Jun 28 2018, 8:24 AM
Closed by commit rLLD335864: lld-link: align sections to 16 bytes if referenced from the gfids table (authored by inglorion). · Explain WhyJun 28 2018, 8:27 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lld/
COFF/
5 lines
test/
COFF/
Inputs/
51 lines
43 lines

Diff 153228

lld/COFF/Writer.cpp

Show First 20 Lines • Show All 1,039 Lines • ▼ Show 20 Lines if (File->hasGuardCF()) {
markSymbolsWithRelocations(File, AddressTakenSyms); markSymbolsWithRelocations(File, AddressTakenSyms);
} }
} }
// Mark the image entry as address-taken. // Mark the image entry as address-taken.
if (Config->Entry) if (Config->Entry)
addSymbolToRVASet(AddressTakenSyms, cast<Defined>(Config->Entry)); addSymbolToRVASet(AddressTakenSyms, cast<Defined>(Config->Entry));
// Ensure sections referenced in the gfid table are 16-byte aligned.
for (const auto &C : AddressTakenSyms)
ruiuUnsubmitted
Done
ReplyInline Actions

Please replace auto with the real type.

ruiu: Please replace `auto` with the real type.
if (C.InputChunk->Alignment < 16)
pccUnsubmitted
Not Done
ReplyInline Actions

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

pcc: I think this can be simplified: the alignment should always be a power of 2.
inglorionAuthorUnsubmitted
Not Done
ReplyInline Actions

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

inglorion: So just if (Alignment < 16) Alignment = 16 would do it?
pccUnsubmitted
Not Done
ReplyInline Actions

Yes.

pcc: Yes.
C.InputChunk->Alignment = 16;
maybeAddRVATable(std::move(AddressTakenSyms), "__guard_fids_table", maybeAddRVATable(std::move(AddressTakenSyms), "__guard_fids_table",
"__guard_fids_count"); "__guard_fids_count");
// Add the longjmp target table unless the user told us not to. // Add the longjmp target table unless the user told us not to.
if (Config->GuardCF == GuardCFLevel::Full) if (Config->GuardCF == GuardCFLevel::Full)
maybeAddRVATable(std::move(LongJmpTargets), "__guard_longjmp_table", maybeAddRVATable(std::move(LongJmpTargets), "__guard_longjmp_table",
"__guard_longjmp_count"); "__guard_longjmp_count");
▲ Show 20 Lines • Show All 224 Lines • Show Last 20 Lines

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

  • This file was added.
--- !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
...

lld/test/COFF/guardcf-align.s

  • This file was added.
# 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: GuardFidTable [
# CHECK-NOT: 0x{{[0-9A-Fa-f]+[^0]$}}
# CHECK: 0x{{[0-9A-Fa-f]+0$}}
# CHECK-NOT: 0x{{[0-9A-Fa-f]+[^0]$}}
hansUnsubmitted
Done
ReplyInline Actions

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?

hans: It would be good to have a comment about the intention of these checks, since it's not obvious…
inglorionAuthorUnsubmitted
Not Done
ReplyInline Actions

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.

inglorion: I've added a comment to explain what we're looking for with the regexes. The table actually…
hansUnsubmitted
Not Done
ReplyInline Actions

Okay, sounds good.

hans: Okay, sounds good.
# 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