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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:3167
+  switch (Inst.getOpcode()) {
+  case X86::RET:
+  case X86::RETL:
----------------
sconstab wrote:
> 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?
I suspect it doesn't have the mayLoad flag because from the perspective of the codegen passes that normally use that flag, the load of the return address isn't really something we would care about. It could just as well be reading the return address from some stack dedicated return address stack or a stack implemented in hardware or something.

Not sure what the implications of just adding the flag would be to codegen.


================
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) {
----------------
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.


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

https://reviews.llvm.org/D76158