This is an archive of the discontinued LLVM Phabricator instance.

Add Indirect Thunk Support to X86 to mitigate Load Value Injection (LVI) [2/6]
AbandonedPublic

Authored by craig.topper on Mar 10 2020, 10:00 AM.

Details

Summary

This pass replaces each indirect call/jump with a direct call to a thunk that looks like:

lfence
jmpq *%r11

This ensures that if the value in register %r11 was loaded from memory, then
the value in %r11 is (architecturally) correct prior to the jump.

Also adds a new target feature to X86: +lvi-cfi
("cfi" meaning control-flow integrity)

The feature can be added via clang CLI using -mlvi-cfi.

Diff Detail

Event Timeline

sconstab created this revision.Mar 10 2020, 10:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2020, 10:00 AM
craig.topper added inline comments.Mar 10 2020, 2:33 PM
clang/include/clang/Driver/Options.td
2295

Please add HelpText to these. It seems we've been bad about this and the lack of HelpText prevents the options from showing up in clang -help

sconstab retitled this revision from Add Indirect Thunk Support to X86 to mitigate Load Value Injection (LVI) to Add Indirect Thunk Support to X86 to mitigate Load Value Injection (LVI) [2/5].Mar 11 2020, 12:35 PM
sconstab updated this revision to Diff 249763.Mar 11 2020, 2:47 PM

Added help text for driver CLI options.

sconstab marked an inline comment as done.Mar 16 2020, 9:29 AM
sconstab retitled this revision from Add Indirect Thunk Support to X86 to mitigate Load Value Injection (LVI) [2/5] to Add Indirect Thunk Support to X86 to mitigate Load Value Injection (LVI) [2/6].

Looks great! Thanks for writing this! I had a bunch of nits (sorry!) and a few questions, otherwise LGTM. Please wait for sign off from at least one other person before submitting.

llvm/lib/Target/X86/X86FastISel.cpp
3210

nit: Update comment?

llvm/lib/Target/X86/X86FrameLowering.cpp
966

Would it make sense to add a separate check for LVI-CFI and have a corresponding error message referencing specifically LVI-CFI rather than retpolines?


I wrote this in several spots, sorry about the repetition.

2707

Would it make sense to add a separate check for LVI-CFI and have a corresponding error message referencing specifically LVI-CFI rather than retpolines?

llvm/lib/Target/X86/X86ISelLowering.cpp
30557

nit: Update comment to mention LVI-CFI

31844

Can you change this function name? If I'm understanding this code correctly, this no longer only applies to retpoline, but also to LVI thunks, so the naming is misleading.

llvm/lib/Target/X86/X86LoadValueInjectionIndirectThunks.cpp
79

I want to make sure I understand this correctly: You use this function to initialize InsertedThunks so that, for each module, InsertedThunks is shared across all the functions. This enables the Module to ensure the thunk is only inserted once. Is that right?

87

Why is 32-bit okay if it has SSE2 features? (Asking to understand since my processor knowledge is a bit weak. Thanks!)

92

Why did you decide to make this not skip functions with optnone?

98

I think this debugging message should be at the top of the function.

129

Should this use R11ThunkName instead of this string literal?

149

I see this list is from the retpoline pass. I don't know what each of these things do, but just wondering if you double checked these are the same attributes we want for this thunk?

llvm/lib/Target/X86/X86MCInstLower.cpp
1227

Would it make sense to add a separate check for LVI-CFI and have a corresponding error message referencing specifically LVI-CFI rather than retpolines?

1407

Would it make sense to add a separate check for LVI-CFI and have a corresponding error message referencing specifically LVI-CFI rather than retpolines?

llvm/test/CodeGen/X86/O0-pipeline.ll
76

nit: remove pass to follow what most of the other passes do here (requires change in pass code and here)

llvm/test/CodeGen/X86/O3-pipeline.ll
185

Same comment as in O0-pipeline.ll test

craig.topper added inline comments.Mar 17 2020, 1:09 PM
llvm/lib/Target/X86/X86LoadValueInjectionIndirectThunks.cpp
92

This suggestion came from myself and Andy Kaylor. skipFunction checks opt-bisect-limit and the optnone attribute. This would make the pass not run at -O0, but that seemed bad. The explicit optnone check here was to avoid the optnone behavior in skipFunction, but make opt-bisect-limit work. But looking this now, I think this left us in a situation where opt-bisect-limit doesn't work for this pass on optnone functions.

sconstab updated this revision to Diff 251193.Mar 18 2020, 3:21 PM

Addressed some of Zola's comments, and removed some unnecessary assertions.

sconstab marked 12 inline comments as done.Mar 18 2020, 3:28 PM
sconstab added inline comments.
llvm/lib/Target/X86/X86LoadValueInjectionIndirectThunks.cpp
79

As mentioned in a code comment at the beginning of this file, a lot of this code was lifted from X86RetpolineThunks.cpp, including the logic to insert one thunk per module. So I didn't actually compose this logic, but my understanding of it seems to match yours.

87

Actually it isn't okay (at this time), so you were correct to point this out. The thunks right now clearly only support 64-bit. It may be conceivable to support 32-bit that also has SSE2, but I think this would require some extra work.

129

I just removed the assertion because it wasn't really necessary.

149

I do think that these are correct. From the LLVM reference manual, NoUnwind means that "This function attribute indicates that the function never raises an exception," which should hold for the thunk. The Naked attribute "disables prologue / epilogue emission for the function," which is something we would not want.

zbrid added a comment.Mar 19 2020, 3:10 PM

I followed up with Chandler about whether it would make sense to integrate this with the existing retpolines pass as you and Craig suggested. He supported the idea. Could you create a new patch(es) to do the refactor/renaming of the retpolines thunking pass and instruction scheduling conditionals to be more general and then add the LVI option?

mattdr added a subscriber: mattdr.Apr 10 2020, 5:10 PM

I accidentally spent time reviewing this, only to cross-reference something in the LLVM code and find another diff (https://reviews.llvm.org/D76812) was written to answer Zola's request and has already been submitted.

craig.topper commandeered this revision.Apr 10 2020, 5:25 PM
craig.topper abandoned this revision.
craig.topper edited reviewers, added: sconstab; removed: craig.topper.

This was superceded by D76812