[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.

This seems like a much more robust approach for mitigating inline asm. Thanks for that!

For now, my biggest question is around layering and separation of concerns. Do we really want to do //transformations// in the //parser//? It seems like that opens the door to making the parser API surprising and less helpful. For example, if the LVI mitigation is enabled the Parse and Emit operations of the parser no longer round-trip, which is very new and very nonobvious.

I definitely understand that you put the code here because it was the one path you could find that all attempts to emit ASM would go through.  And I know it's frustrating I don't have a concrete suggestion for a different approach. Still working on that.



================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:3158
+bool X86AsmParser::applyLVICFIMitigation(MCInst &Inst) {
+  switch (Inst.getOpcode()) {
+  case X86::RET:
----------------
Let's add a comment about how this list was created, or maybe a reference to public documentation if this list is pulled from there. Otherwise folks coming afterward have no way to tell if this list is correct.


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:3186
+/// returns `true` if a mitigation was applied or warning was emitted.
+bool X86AsmParser::applyLVILoadHardeningMitigation(MCInst &Inst,
+                                                   MCStreamer &Out) {
----------------
Once the details of LVI have faded, future maintainers will really appreciate a comment here about what this mitigation *is*, what it's doing, etc.


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:3192
+    switch (Opcode) {
+    case X86::CMPSB:
+    case X86::CMPSW:
----------------
Same comment as for the list above.


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

https://reviews.llvm.org/D76158