[PATCH] D41723: Introduce the "retpoline" x86 mitigation technique for variant #2 of the speculative execution vulnerabilities disclosed today, specifically identified by CVE-2017-5715, "Branch Target Injection", and is one of the two halves to Spectre..

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: llvm/lib/CodeGen/IndirectBrExpandPass.cpp:113
+      // index.
+      if (BBIndex == -1) {
+        BBIndex = BBs.size();
----------------
dexonsmith wrote:
> efriedma wrote:
> > chandlerc wrote:
> > > efriedma wrote:
> > > > blockaddresses are uniqued, so no block should ever have more than one blockaddress user.  So this should probably be an assertion.
> > > I just didn't want to hard code that assumption, but I can if you prefer.
> > If we violate that assumption, something has gone very wrong (either we've created a blockaddress in the wrong context, or we leaked a blockaddress from the context, or we have a blockaddress with an invalid block+function pair).
> > 
> > Although, on a related note, you might want to check Constant::isConstantUsed(), so we don't generate indexes for blockaddresses which aren't actually referenced anywhere.
> FWIW, I agree with Eli that it's fundamentally bad if constants haven't been uniqued properly.
Yeah, I'm convinced. It actually makes the code *way* simpler. I've implemented all of the suggestions here.


================
Comment at: llvm/lib/CodeGen/IndirectBrExpandPass.cpp:134
+  // Now rewrite each indirectbr to cast its loaded pointer to an integer and
+  // switch on it using the integer map from above.
+  for (auto *IBr : IndirectBrs) {
----------------
sanjoy wrote:
> chandlerc wrote:
> > sanjoy wrote:
> > > Do we care about inline assembly here?  The langref says "Finally, some targets may provide defined semantics when using the value as the operand to an inline assembly, but that is target specific."
> > I mean, yes, but also no. ;]
> > 
> > It would be nice to maybe preserve inline asm uses of blockaddr and not any others. And then force people to not rinse their blackaddr usage through inline asm and mix that with `-mretpoline`. That would allow the common usage I'm aware of to remain (showing label addresses in crash dumps in things like kernels) and really allow almost any usage wholly contained within inline asm to continue working perfectly.
> > 
> > But it seemed reasonable for a follow-up. That said, maybe its not too complex to add now...
> What do you think about `report_fatal_error`ing here if you encounter an inline assembly user?  That seems somewhat more friendly than silently "miscompiling" (in quotes) inline assembly.
I actually think that'd be much less friendly.

The primary use case I'm aware of is for diagnostics, and I'd much rather have low-quality diagnostics in some random subsystem than be unable to use retpolines...

I've added a FIXME about this but I'm pretty nervous about breaking round-trip-through-inline-asm to repair a quality loss in diagnostics. But we can revisit this if needed.


================
Comment at: llvm/lib/Target/X86/X86InstrCompiler.td:1160
           (TCRETURNdi tglobaladdr:$dst, imm:$off)>,
-          Requires<[NotLP64]>;
+          Requires<[NotLP64, NotUseRetpoline]>;
 
----------------
rnk wrote:
> chandlerc wrote:
> > AndreiGrischenko wrote:
> > > Hi Chandler,
> > > Do you really want to use "NotUseRetpoline" here? It will match RETPOLINE_TCRETURN32 then even it is not an indirect branch.
> > > I guess the following test case will crash llc.
> > > 
> > > ```
> > > target triple = "i386-unknown-linux-gnu"
> > > 
> > > define void @FOO() {
> > >   ret void
> > > }
> > > 
> > > define void @FOO1() {
> > > entry:
> > >   tail call void @FOO()
> > >   ret void
> > > }
> > > ```
> > > 
> > Reid, could you take a look?
> Here's a small diff on top of this one that fixes and tests it: https://reviews.llvm.org/P8056
Awesome, applied!


https://reviews.llvm.org/D41723