[PATCH] D75936: Add a Pass to X86 that builds a Condensed CFG for Load Value Injection (LVI) Gadgets [4/6]

Craig Topper via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Apr 8 11:25:42 PDT 2020


craig.topper marked 37 inline comments as done and an inline comment as not done.
craig.topper added inline comments.


================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:41
+class ImmutableGraph {
+  using Traits = GraphTraits<ImmutableGraph<NodeValueT, EdgeValueT> *>;
+  template <typename> friend class ImmutableGraphBuilder;
----------------
sconstab wrote:
> mattdr wrote:
> > I think this self-reference to `ImmutableGraph` dropped the `SizeT` parameter.
> Yup. Good catch.
Removed the template argument


================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:73
+    // The end of this Node's edges is the beginning of the next node's edges.
+    const Edge *edges_end() const { return (this + 1)->Edges; }
+    ArrayRef<Edge> edges() const {
----------------
sconstab wrote:
> mattdr wrote:
> > sconstab wrote:
> > > mattdr wrote:
> > > > Seems like you also want to add a comment here that we know we will never be asked for `edges_end` for the last stored node -- that is, we know that `this + 1` always refers to a valid Node (which is presumably a dummy/sentinel)
> > > Not sure I agree. I cannot think of a conventional use of this interface that would perform an operation on the sentinel.
> > > ```
> > > G->nodes_end().edges_end(); // invalid use of any end iterator
> > > SomeNode.edges_end(); // invalid if SomeNode == G->nodes_end()
> > > ```
> > > That is, the way that we "know" that we will never be asked for `edges_end()` for the last stored node is that the ask itself would already violate C++ conventions.
> > I believe any operation on the last `Node` in the array will end up accessing the sentinel:
> > 
> > ```
> > Node* LastNode = G->nodes_begin() + (G->nodes_size() - 1);  // valid reference to the last node
> > LastNode->edges_end();  // uses `this+1`, accessing the sentinel value in the Nodes array
> > ```
> > 
> `G->nodes_size()` will return the size without the sentinel node, so your example should actually operate on the last data node. Right?
Comment added to clarify the extra node was allocated.


================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:117
+    NodeSet(const ImmutableGraph &G, bool ContainsAll = false)
+        : G{G}, V{static_cast<unsigned>(G.nodes_size()), ContainsAll} {}
+    bool insert(const Node &N) {
----------------
sconstab wrote:
> mattdr wrote:
> > How do we know that a value of `size_type` (aka `SizeT`) can be cast to `unsigned` without truncation?
> Ah. We do not know that. We could have a static assert here, but maybe the best thing to do would be to follow Matt's earlier advice and fix `size_type` to `int`, rather than have it as a template parameter. Anything larger would break the `BitVectors` and/or waste space.
Removed the template argument


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:113
+          NumFences(NumFences), NumGadgets(NumGadgets) {}
+    MachineFunction &getMF() { // FIXME: This function should be cleaner
+      for (const Node &N : nodes())
----------------
mattdr wrote:
> sconstab wrote:
> > mattdr wrote:
> > > Cleaner how?
> > Maybe by keeping a member reference to the associated `MachineFunction`?
> Let's put that in the comment instead.
I found a way to remove the getMF method entirely.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:233
+  if (!STI->useLVILoadHardening() || !STI->is64Bit())
+    return false; // FIXME: support 32-bit
+
----------------
sconstab wrote:
> mattdr wrote:
> > If the user requests hardening and we can't do it, it seems better to fail loudly so they don't accidentally deploy an unmitigated binary.
> @craig.topper I think this is related to the discussion we were having about what would happen for SLH on unsupported subtargets. I'm not sure what the most appropriate solution would be.
Added a fatal error. Which isn't great as it will generate a crash report in clang. But it will tell the user to file a compiler bug so I guess that's something.


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

https://reviews.llvm.org/D75936





More information about the llvm-commits mailing list