This is an archive of the discontinued LLVM Phabricator instance.

Add FileVerifier::isCFIProtected().
ClosedPublic

Authored by hctim on Sep 29 2017, 2:29 PM.

Details

Summary

Add a CFI protection check that is implemented by building a graph and inspecting the output to deduce if the indirect CF instruction is CFI protected. Also added the output of this instruction to printIndirectInstructions().

Event Timeline

hctim created this revision.Sep 29 2017, 2:29 PM
hctim updated this revision to Diff 117437.Oct 2 2017, 3:07 PM

Update to only diff with prev in stack.

hctim updated this revision to Diff 117452.Oct 2 2017, 4:45 PM

Fixed warnings.

On a mechanical note, you should add -U999999 to your git diff options to include full files in the diff (makes scrolling below/above added/removed lines possible in the web view)

tools/llvm-cfi-verify/FileVerifier.cpp
102

This is checked in two different places, might be worth having a helper function?

112

This would be a surprising reason to see 'Protected? No.' Perhaps worth making this an enum return value with no/yes/could-not-be-determined?

hctim updated this revision to Diff 117743.Oct 4 2017, 2:34 PM
hctim marked 2 inline comments as done.

Fixed issues from Vlad's comments.

hctim added inline comments.Oct 4 2017, 2:34 PM
tools/llvm-cfi-verify/FileVerifier.cpp
112

I'm not sure it would be surprising. Orphaned nodes are those which have no static cross-references to them, meaning the control flow graph dies with them.

[0x0: ud2]                   [0x1000: movb $0x1, %rax]
    |                               |
[0x1: nop] <-----------------[0x1002: callq %rax]
    |
[0x2: jmpq %rax]

In this example, the graph starts at 0x2, building up to 0x1. There are no xrefs to 0x1 (as 0x0 cannot fall through, and 0x1002's target is non-static), and hence 0x1 is added as an "orphaned node".

My assumption is that, in order for an indirect CF to be CFI protected, all possible ways to reach the indirect CF must be CFI protected. If a vcall then immediately makes another vcall without CFI checking (emulated in the example above), shouldn't it be flagged?

You have a better understanding of this than me though, feel free to correct me :)

hctim updated this revision to Diff 117882.Oct 5 2017, 1:51 PM

Backported D38564

hctim updated this revision to Diff 118109.Oct 6 2017, 5:22 PM

clang-format

hctim updated this revision to Diff 118112.Oct 6 2017, 5:25 PM

format was from prev in stack, put it down there.

tools/llvm-cfi-verify/FileAnalysis.cpp
168 ↗(On Diff #118112)

Place this in the first CL where you can also use it.

tools/llvm-cfi-verify/FileAnalysis.h
72 ↗(On Diff #118112)

isCFIProtectedIndirectBranch ?

tools/llvm-cfi-verify/llvm-cfi-verify.cpp
35

Only print this line for indirect branches?

hctim updated this revision to Diff 118256.Oct 9 2017, 12:44 PM
hctim marked 3 inline comments as done.

Updated from Vlad's comments.

hctim added inline comments.Oct 9 2017, 12:45 PM
tools/llvm-cfi-verify/FileAnalysis.h
72 ↗(On Diff #118112)

Related to https://reviews.llvm.org/D38379#inline-337653.

Covers indirect calls as well, LMK if you want this change.

tools/llvm-cfi-verify/llvm-cfi-verify.cpp
35

Related to https://reviews.llvm.org/D38379#inline-337653.

This currently only prints indirect calls + jumps. Should this be changed?

hctim updated this revision to Diff 118424.Oct 10 2017, 10:44 AM

Merged updates from stack.

tools/llvm-cfi-verify/FileAnalysis.h
72 ↗(On Diff #118112)

How about isIndirectInstructionCFIProtected() ?

tools/llvm-cfi-verify/llvm-cfi-verify.cpp
35

Ah, I just mis-read the loop condition here.

hctim updated this revision to Diff 119700.Oct 20 2017, 1:23 PM
hctim marked 4 inline comments as done.

Vlad's comments & merged changes from the stack.

hctim updated this revision to Diff 119701.Oct 20 2017, 1:24 PM

Added successful initialisation checks.

Harbormaster completed remote builds in B11337: Diff 119701.
hctim updated this revision to Diff 119935.Oct 23 2017, 2:05 PM

Patched in changes from master.

vlad.tsyrklevich accepted this revision.Oct 25 2017, 2:14 PM
This revision is now accepted and ready to land.Oct 25 2017, 2:14 PM
hctim updated this revision to Diff 120313.Oct 25 2017, 2:18 PM

Merged master in preparation for submission.

This revision was automatically updated to reflect the committed changes.