[PATCH] D75937: Add Support to X86 for Load Hardening to Mitigate Load Value Injection (LVI) [5/6]

[Matthew Riley via Phabricator via llvm-commits]

mattdr marked 2 inline comments as done.
mattdr added inline comments.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:541
+      if (isFence(Dest->getValue())) {
+        ElimNodes.insert(*Dest);
+        ElimEdges.insert(E);
----------------
sconstab wrote:
> mattdr wrote:
> > How are we sure we've removed all edges that pointed to this same destination node?
> I think it does? This loop iterates through ALL edges. For each CFG edge that ingresses a fence, add that fence to `ElimNodes`, and add that edge and all egress CFG edges to `ElimEdges`. The loop does *not* skip over edges that have been added to `ElimEdges`, and therefore I think this should ensure that all CFG edges pointing to a given fence are removed.
Sure enough, that makes sense.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:555-556
+  for (const auto &RootN : G.nodes()) {
+    if (llvm::none_of(RootN.edges(), MachineGadgetGraph::isGadgetEdge))
+      continue; // skip this node if it isn't a gadget source
+
----------------
sconstab wrote:
> mattdr wrote:
> > I think this is just intended as an optimization -- it's not necessary for correctness. Assuming that's right, suggest removing it since it doesn't really make things faster but it does add some extra complexity.
> Why do you think it doesn't make things faster? A majority of nodes in the graph are not gadget sinks, and this majority tends to grow rapidly as gadgets become mitigated. Each time this check passes, it saves an entire DFS traversal and one `O(E)` loop through the edges.
Ack! Yes, it seems I consistently misread this function and missed the difference between `isGadgetEdge` in some places and `IsCFGEdge` in others. (As I mentioned, having both represented in the same graph is confusing.) Now I see this is a different check than is done in the DFS.


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

https://reviews.llvm.org/D75937