[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 2 inline comments as done.
sconstab added inline comments.


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:3167
+  switch (Inst.getOpcode()) {
+  case X86::RET:
+  case X86::RETL:
----------------
mattdr wrote:
> Following up on my previous comment: it seems like this is supposed to be the full set of instructions that combine a load from memory with a control flow change dependent on that load.
> 
> How are we sure the list is complete? Just from a quick look at the [[ https://github.com/llvm/llvm-project/blob/1c545f6dbcbb3ada2dfef2c6afbc1ca8939135cb/llvm/lib/Target/X86/X86InstrControl.td#L23-L57 | return instructions LLVM has for X86 ]], it looks like `LRETL` and `LRETQ` exist and would not be mitigated.
> 
> Can we just look at the `MCInstDesc` and warn for instructions that `mayLoad` and `mayAffectControlFlow`? Then this list will stay up to date even if new instructions are added.
> 
I looked into this and unfortunately the mayLoad attribute is not set for any of the RET family (see https://github.com/llvm/llvm-project/blob/b1d581019f5d4d176ad70ddffee13db247a13ef1/llvm/lib/Target/X86/X86InstrControl.td#L21). Either this is a bug or I don't actually understand what the mayLoad attribute actually means. It seems pretty strange since the POPr family has mayLoad. @craig.topper : opinion?


================
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:
> 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?


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

https://reviews.llvm.org/D76158