[PATCH] Insert random NOPs to increase security against ROP attacks (llvm)

[JF Bastien]

Same thing as on the clang side on updating the description to "noop".

Can you explain why the default values that you chose are the right ones? I think that out of the box the -fdiversity flag should provide the "right" defaults, for some "right" that you get to define.

Please run clang-format over the new code.

It would be good to have a new test which exercise x86-32 and architectures which implement insertNoop (Mips, PowerPC, R600), to make sure these work. It would be good to add ARM and Aarch64 to that list, but that can be a different patch.


================
Comment at: include/llvm/CodeGen/NoopInsertion.h:30
@@ +29,3 @@
+  NoopInsertion();
+  ~NoopInsertion();
+
----------------
`override`

================
Comment at: include/llvm/CodeGen/NoopInsertion.h:34
@@ +33,3 @@
+
+  void getAnalysisUsage(AnalysisUsage &AU) const override;
+
----------------
The above 2 can be `private` or `protected`.

================
Comment at: include/llvm/CodeGen/NoopInsertion.h:37
@@ +36,3 @@
+private:
+  RandomNumberGenerator *RNG = NULL;
+
----------------
`nullptr`. Can you also make this a `std::unique_ptr`?

================
Comment at: include/llvm/Support/RandomNumberGenerator.h:34
@@ -33,1 +33,3 @@
 public:
+  typedef uint_fast64_t result_type;
+
----------------
`RNG::result_type`

================
Comment at: include/llvm/Support/RandomNumberGenerator.h:46
@@ -36,1 +45,3 @@
+    return std::mt19937_64::max();
+  }
 
----------------
I think `constexpr` still isn't usable in LLVM, so this may have to be `LLVM_CONSTEXPR` from `llvm/Support/Compiler.h`.

================
Comment at: include/llvm/Support/RandomNumberGenerator.h:59
@@ -47,3 +58,3 @@
   // implementations.
   std::mt19937_64 Generator;
 
----------------
`typedef std::mt19937_64 RNG` and use it here as well as for `min`, `max`, and `result_type`.

================
Comment at: include/llvm/Target/TargetInstrInfo.h:875
@@ +874,3 @@
+                          MachineBasicBlock::iterator MI,
+                          RandomNumberGenerator *RNG) const {
+    insertNoop(MBB, MI);
----------------
Remove `RNG` here, since it isn't used (keep just `RandomNumberGenerator*`).

================
Comment at: lib/CodeGen/NoopInsertion.cpp:64
@@ +63,3 @@
+  if (RNG != NULL)
+    delete RNG;
+}
----------------
Remove when using `std::unique_ptr`.

================
Comment at: lib/CodeGen/NoopInsertion.cpp:84
@@ +83,3 @@
+    // cannot be a range-based for loop since we need to pass the
+    // iterator to insertNoop()
+    for (MachineBasicBlock::iterator I = BB.begin(); I != BB.end(); ++I) {
----------------
No need to comment range-based for loop.

================
Comment at: lib/CodeGen/NoopInsertion.cpp:85
@@ +84,3 @@
+    // iterator to insertNoop()
+    for (MachineBasicBlock::iterator I = BB.begin(); I != BB.end(); ++I) {
+      if (I->isPseudo())
----------------
``` for (MachineBasicBlock::iterator I = BB.begin(), E = BB.end(); I != E; ++I) {```

Can you also end at `FirstTerm`, and remove the `break` below?

================
Comment at: lib/CodeGen/NoopInsertion.cpp:104
@@ +103,3 @@
+  }
+  return true;
+}
----------------
This may not have inserted a noop, you should return false in that case. You could just track the number of inserted noops in the loop, and update `InsertedNoops` here, and return whether the local version was non-zero.

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:5536
@@ +5535,3 @@
+  // This set of Noop instructions was carefully chosen to not be
+  // useful as ROP gadgets.
+  enum { NOP,
----------------
Please explain *why* they're not useful as ROP gadgets. The landmine concept isn't obvious from the code, and the length of each nop presumably also matters.

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:5550
@@ +5549,3 @@
+
+  static std::uniform_int_distribution<unsigned> Distribution(0,MAX_NOPS-1);
+
----------------
No need to be `static` since this class has no data members.

http://reviews.llvm.org/D3392

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/