This is an archive of the discontinued LLVM Phabricator instance.

[X86] Automatically harden inline assembly RET instructions against Load Value Injection (LVI)

Authored by sconstab on Jun 9 2020, 2:22 PM.



Previously, the X86AsmParser would issue a warning whenever a ret instruction is encountered. This patch changes the behavior to automatically transform each ret instruction in an inline assembly stream into:

shlq $0, (%rsp)

which is secure, according to

Diff Detail

Event Timeline

sconstab created this revision.Jun 9 2020, 2:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2020, 2:22 PM
Herald added subscribers: jfb, hiraditya. · View Herald Transcript
craig.topper added inline comments.Jun 9 2020, 2:39 PM

Why did emitInstruction have to get repeated everywhere? Aren't we still emitting the original ret from the user after the mitigation? So its not any different than inserting an LFENCE. Am I missing something?

sconstab marked an inline comment as done.Jun 9 2020, 3:39 PM
sconstab added inline comments.

It's a little more complicated now because the ret mitigation requires the LFENCE to be inserted *before* ret, whereas all other loading instructions need the LFENCE after. But maybe there is a cleaner way to implement this that I am missing?

sconstab marked an inline comment as not done.Jun 9 2020, 3:39 PM
craig.topper added inline comments.Jun 9 2020, 4:09 PM

right the others are after. why can't i ever remember that.

What if we called applyLVICFIMitigation before the emitInstruction and called applyLVILoadHardeningMitigation after?

sconstab updated this revision to Diff 269715.Jun 9 2020, 6:07 PM

Addressed suggestion by @craig.topper to consolidate calls to emitInstruction().

sconstab marked an inline comment as done.Jun 9 2020, 6:07 PM
This revision is now accepted and ready to land.Jun 10 2020, 11:17 AM
mattdr accepted this revision.Jun 10 2020, 3:50 PM

LGTM, one question about source locations


Seems like this will have implications for debugging. Can we copy the SMLoc from the original instruction, for this and for the other MCInsts we're creating?

craig.topper added inline comments.Jun 10 2020, 4:17 PM

You mean like debugging in GDB?

SMLoc is just the pointer into the memory buffer that holds the assembly text. It has no effect on debug locations in the final binary. It's main use is to print diagnostics in the compiler.

Ack, thanks for setting me straight on that. Yes, I was talking about the mapping of assembly instructions back to sourceloc that could be displayed in GDB.