[PATCH] D76158: Add inline assembly load hardening mitigation for Load Value Injection (LVI) on X86 [6/6]

[Scott Constable via Phabricator via llvm-commits]

sconstab marked 3 inline comments as done.
sconstab added inline comments.


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:3186
+  case X86::FARCALL64:
+    Warning(Inst.getLoc(), "Instruction may be vulnerable to LVI and "
+                           "requires manual mitigation");
----------------
mattdr wrote:
> sconstab wrote:
> > mattdr wrote:
> > > This warning should at least tell users *why* this is vulnerable, and ideally offer the next step for fixing it.
> > The manual mitigation is a little bit more involved than what would typically be conveyed in a compiler warning. Would it be acceptable for the warning to point to the LVI deep dive document?
> Sure, a link to an external site seems fine. Can we please link to the specific part of the page that explains the mitigations for this specific set of instructions?
@mattdr Working on it!


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:3204
+  auto Flags = Inst.getFlags();
+  if ((Flags & X86::REP_PREFIX) || (Flags & X86::REPNE_PREFIX)) {
+    switch (Opcode) {
----------------
mattdr wrote:
> craig.topper wrote:
> > I don't think this works if the rep prefix is on the line before the cmps/scas instruction. That will assemble to the same thing but the parser sees it as two instructions.
> @craig.topper any notion what the best workaround would be? One option is keeping track of whether we've seen a `REP` or `REPNE` before this.
> 
> Although perhaps a simpler approach is just not worrying about whether we saw `REP`. We're here to try to warn users about instructions that we can't mitigate. What's the likelihood that we see a `CMPSB` without `REP` and it turns out it _doesn't_ need mitigation?
@mattdr The difference is that `CMPSB` without `REP` can be mitigated simply by following it with an `LFENCE`. `REP CMPSB` must be manually decomposed into a loop, into which an `LFENCE` can be inserted.

I wonder if it would suffice to simply emit a warning/error if a lone `REP` is encountered to indicate that the user may need to manually mitigate?


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:3206
+    switch (Opcode) {
+    case X86::CMPSB:
+    case X86::CMPSW:
----------------
mattdr wrote:
> mattdr wrote:
> > I didn't find any mention of these instructions, why they would "require additional considerations", or what folks should actually *do* if they hit this warning by reading https://software.intel.com/security-software-guidance/insights/deep-dive-load-value-injection.
> > 
> > Here's my best guess:
> > This is a list of [[ https://cpu.fyi/d/484#G7.340223 | string instructions that can be used with `REP` ]] and which update `ZF` in `EFLAGS` instead of relying on an a count in `rcx` to terminate". When used with `REP` or `REPNE`, these instructions become interesting because an attacker could use LVI to change the target's speculative control flow by injecting a value for a load. Since the instruction is indivisible, there's no place to put an `LFENCE`, so like `RET` these instructions need to be split into load and compare/branch components.
> > 
> > Is that accurate?
> > 
> > If so, we could explain it that way and replace the hardcoded list with `hasImplicitDefOfPhysReg(X86::EFLAGS)`
> And if we hit the same problem as above with `RET`, where somehow the instruction tables are inaccurate and we can't rely on them yet, then it's similarly okay with me if we leave the hardcoded list of instructions as long as we provide really specific comments describing why the list is all and only these instructions.
Similar to providing a link to a public document in a warning message, I think it should suffice to have a link to said document (and specifically the relevant subsection of that document) in the comment.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76158/new/

https://reviews.llvm.org/D76158