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

[Matthew Riley via Phabricator via llvm-commits]

mattdr added a comment.

First, want to apologize for the high latency -- thanks to //(gestures vaguely in the air)// I've been mostly offline during the day and trying to catch up at night.

I think I've probably said my piece here. Summarizing themes from my comments:

1. the parser seems like a weird place to put this mitigation
2. the lists of instructions feel very magic because there's no explanation of how they were arrived at. They can also be incomplete or become out of date. Consider fixing both by matching instructions by interesting **properties** instead of opcodes. Include an explanation of why the properties make the instructions interesting in the context of LVI.
3. the warnings we fire aren't actionable

Ultimately, though, these are code-quality and maintainability issues and I'm not a maintainer. Especially since I'm only occasionally-online right now, please don't consider me a blocker for the review.



================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:3167
+  switch (Inst.getOpcode()) {
+  case X86::RET:
+  case X86::RETL:
----------------
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.



================
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");
----------------
This warning should at least tell users *why* this is vulnerable, and ideally offer the next step for fixing it.


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:3206
+    switch (Opcode) {
+    case X86::CMPSB:
+    case X86::CMPSW:
----------------
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)`


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

https://reviews.llvm.org/D76158