<div dir="ltr">On Mon, Jul 14, 2014 at 12:43 PM, Bob Wilson <span dir="ltr"><<a href="mailto:bob.wilson@apple.com" target="_blank">bob.wilson@apple.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class=""><br>
> On Jul 11, 2014, at 3:34 PM, Akira Hatanaka <<a href="mailto:ahatanak@gmail.com">ahatanak@gmail.com</a>> wrote:<br>
><br>
> The attached patch fixes a bug where the address of the stack guard was being spilled to the stack, which is a potential security vulnerability attackers can take advantage of.<br>
<br>
</div>I have a few comments inline below….<br>
<br>
> diff --git a/include/llvm/Target/Target.td b/include/llvm/Target/Target.td<br>
> index f77cc7a..26e1242 100644<br>
> --- a/include/llvm/Target/Target.td<br>
> +++ b/include/llvm/Target/Target.td<br>
> @@ -841,6 +841,14 @@ def PATCHPOINT : Instruction {<br>
> let mayLoad = 1;<br>
> let usesCustomInserter = 1;<br>
> }<br>
> +def LOAD_STACK_GUARD : Instruction {<br>
> + let OutOperandList = (outs ptr_rc:$dst);<br>
> + let InOperandList = (ins);<br>
> + let mayLoad = 1;<br>
> + bit isReMaterializable = 1;<br>
> + let neverHasSideEffects = 1;<br>
<br>
neverHasSideEffects is deprecated. This should be "hasSideEffects = 0”.<br>
<br>
> + bit isPseudo = 1;<br>
> +}<br>
> }<br>
><br>
> //===----------------------------------------------------------------------===//<br>
> diff --git a/include/llvm/Target/TargetLowering.h b/include/llvm/Target/TargetLowering.h<br>
> index 180f0f2..ef0fc27 100644<br>
> --- a/include/llvm/Target/TargetLowering.h<br>
> +++ b/include/llvm/Target/TargetLowering.h<br>
> @@ -2589,6 +2589,13 @@ public:<br>
> /// ARM 's' setting instructions.<br>
> virtual void<br>
> AdjustInstrPostInstrSelection(MachineInstr *MI, SDNode *Node) const;<br>
> +<br>
> + /// If this function returns true, SelectionDAGBuilder emits a<br>
> + /// LOAD_STACK_GUARD node when it is lowering Intrinsic::stackprotector.<br>
> + virtual bool emitLoadStackGuardNode() const {<br>
<br>
The name of this function implies that it emits something. It doesn’t. It just returns a bool to say whether the target should use the new LOAD_STACK_GUARD node. The name should reflect that. How about useLoadStackGuardNode?<br>
<br>
> + return false;<br>
> + }<br>
> +<br>
<br>
There’s no need to add a blank line here.<br>
<br>
> };<br>
><br>
> /// Given an LLVM IR type and return type attributes, compute the return value<br>
><br>
> diff --git a/include/llvm/Target/TargetOpcodes.h b/include/llvm/Target/TargetOpcodes.h<br>
> index abb3eca..1fbd2ae 100644<br>
> --- a/include/llvm/Target/TargetOpcodes.h<br>
> +++ b/include/llvm/Target/TargetOpcodes.h<br>
> @@ -104,7 +104,13 @@ enum {<br>
> /// support optimizations for dynamic languages (such as javascript) that<br>
> /// rewrite calls to runtimes with more efficient code sequences.<br>
> /// This also implies a stack map.<br>
> - PATCHPOINT = 18<br>
> + PATCHPOINT = 18,<br>
> +<br>
> + /// This pseudo-instruction loads the stack guard value. Targets which need<br>
> + /// to prevent the stack guard value or address from being spilled to the<br>
> + /// stack should override TargetLowering::emitLoadStackGuardNode and<br>
> + /// additionally expand this pseudo after register allocation.<br>
> + LOAD_STACK_GUARD = 19<br>
> };<br>
> } // end namespace TargetOpcode<br>
> } // end namespace llvm<br>
><br>
> diff --git a/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp<br>
> index 28d8e98..0d89776 100644<br>
> --- a/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp<br>
> +++ b/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp<br>
> @@ -1811,9 +1811,19 @@ void SelectionDAGBuilder::visitSPDescriptorParent(StackProtectorDescriptor &SPD,<br>
><br>
> unsigned Align =<br>
> TLI->getDataLayout()->getPrefTypeAlignment(IRGuard->getType());<br>
> - SDValue Guard = DAG.getLoad(PtrTy, getCurSDLoc(), DAG.getEntryNode(),<br>
> - GuardPtr, MachinePointerInfo(IRGuard, 0),<br>
> - true, false, false, Align);<br>
> +<br>
> + SDValue Guard;<br>
> +<br>
> + // If emitLoadStackGuardNode returns true, retrieve the guard value from<br>
> + // the virtual register holding the value. Otherwise, emit a volatile load<br>
> + // to retrieve the stack guard value.<br>
> + if (TLI->emitLoadStackGuardNode())<br>
> + Guard = DAG.getCopyFromReg(DAG.getEntryNode(), getCurSDLoc(),<br>
> + SPD.getGuardReg(), PtrTy);<br>
> + else<br>
> + Guard = DAG.getLoad(PtrTy, getCurSDLoc(), DAG.getEntryNode(),<br>
> + GuardPtr, MachinePointerInfo(IRGuard, 0),<br>
> + true, false, false, Align);<br>
><br>
> SDValue StackSlot = DAG.getLoad(PtrTy, getCurSDLoc(), DAG.getEntryNode(),<br>
> StackSlotPtr,<br>
> @@ -5220,8 +5230,35 @@ SelectionDAGBuilder::visitIntrinsicCall(const CallInst &I, unsigned Intrinsic) {<br>
> MachineFunction &MF = DAG.getMachineFunction();<br>
> MachineFrameInfo *MFI = MF.getFrameInfo();<br>
> EVT PtrTy = TLI->getPointerTy();<br>
> + SDValue Src, Chain = getRoot();<br>
<br>
> +<br>
> + if (TLI->emitLoadStackGuardNode()) {<br>
> + // Emit a LOAD_STACK_GUARD node.<br>
> + MachineSDNode *Node = DAG.getMachineNode(TargetOpcode::LOAD_STACK_GUARD,<br>
> + sdl, PtrTy, Chain);<br>
> + LoadInst *LI = cast<LoadInst>(I.getArgOperand(0));<br>
> + MachinePointerInfo MPInfo(LI->getPointerOperand());<br>
> + MachineInstr::mmo_iterator MemRefs = MF.allocateMemRefsArray(1);<br>
> + unsigned Flags = MachineMemOperand::MOLoad |<br>
> + MachineMemOperand::MOInvariant;<br>
> + *MemRefs = MF.getMachineMemOperand(MPInfo, Flags,<br>
> + PtrTy.getSizeInBits() / 8,<br>
> + LI->getAlignment());<br>
> + Node->setMemRefs(MemRefs, MemRefs + 1);<br>
> +<br>
> + // Copy the guard value to a virtual register so that it can be<br>
> + // retrieved in the epilogue.<br>
> + Src = SDValue(Node, 0);<br>
> + const TargetRegisterClass *RC =<br>
> + TLI->getRegClassFor(Src.getSimpleValueType());<br>
> + unsigned Reg = MF.getRegInfo().createVirtualRegister(RC);<br>
> +<br>
> + SPDescriptor.setGuardReg(Reg);<br>
> + Chain = DAG.getCopyToReg(Chain, sdl, Reg, Src);<br>
> + } else {<br>
> + Src = getValue(I.getArgOperand(0)); // The guard's value.<br>
> + }<br>
><br>
> - SDValue Src = getValue(I.getArgOperand(0)); // The guard's value.<br>
> AllocaInst *Slot = cast<AllocaInst>(I.getArgOperand(1));<br>
><br>
> int FI = FuncInfo.StaticAllocaMap[Slot];<br>
> @@ -5230,7 +5267,7 @@ SelectionDAGBuilder::visitIntrinsicCall(const CallInst &I, unsigned Intrinsic) {<br>
> SDValue FIN = DAG.getFrameIndex(FI, PtrTy);<br>
><br>
> // Store the stack protector onto the stack.<br>
> - Res = DAG.getStore(getRoot(), sdl, Src, FIN,<br>
> + Res = DAG.getStore(Chain, sdl, Src, FIN,<br>
> MachinePointerInfo::getFixedStack(FI),<br>
> true, false, 0);<br>
> setValue(&I, Res);<br>
> diff --git a/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h b/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h<br>
> index 84679f9..cf0846f 100644<br>
> --- a/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h<br>
> +++ b/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h<br>
> @@ -397,7 +397,8 @@ private:<br>
> class StackProtectorDescriptor {<br>
> public:<br>
> StackProtectorDescriptor() : ParentMBB(nullptr), SuccessMBB(nullptr),<br>
> - FailureMBB(nullptr), Guard(nullptr) { }<br>
> + FailureMBB(nullptr), Guard(nullptr),<br>
> + GuardReg(0) { }<br>
> ~StackProtectorDescriptor() { }<br>
><br>
> /// Returns true if all fields of the stack protector descriptor are<br>
> @@ -455,6 +456,9 @@ private:<br>
> MachineBasicBlock *getFailureMBB() { return FailureMBB; }<br>
> const Value *getGuard() { return Guard; }<br>
><br>
> + unsigned getGuardReg() const { return GuardReg; }<br>
> + void setGuardReg(unsigned R) { GuardReg = R; }<br>
> +<br>
> private:<br>
> /// The basic block for which we are generating the stack protector.<br>
> ///<br>
> @@ -477,6 +481,9 @@ private:<br>
> /// stack protector stack slot.<br>
> const Value *Guard;<br>
><br>
> + /// The virtual register holding the stack guard value.<br>
> + unsigned GuardReg;<br>
> +<br>
> /// Add a successor machine basic block to ParentMBB. If the successor mbb<br>
> /// has not been created yet (i.e. if SuccMBB = 0), then the machine basic<br>
> /// block will be created.<br>
> diff --git a/lib/Target/AArch64/AArch64ISelLowering.cpp b/lib/Target/AArch64/AArch64ISelLowering.cpp<br>
> index 07ff093..97bec0d 100644<br>
> --- a/lib/Target/AArch64/AArch64ISelLowering.cpp<br>
> +++ b/lib/Target/AArch64/AArch64ISelLowering.cpp<br>
> @@ -7914,6 +7914,10 @@ bool AArch64TargetLowering::shouldExpandAtomicInIR(Instruction *Inst) const {<br>
> return Inst->getType()->getPrimitiveSizeInBits() <= 128;<br>
> }<br>
><br>
> +bool AArch64TargetLowering::emitLoadStackGuardNode() const {<br>
> + return Subtarget->isTargetMachO();<br>
> +}<br>
> +<br>
> TargetLoweringBase::LegalizeTypeAction<br>
> AArch64TargetLowering::getPreferredVectorAction(EVT VT) const {<br>
> MVT SVT = VT.getSimpleVT();<br>
> diff --git a/lib/Target/AArch64/AArch64ISelLowering.h b/lib/Target/AArch64/AArch64ISelLowering.h<br>
> index cb0b9ef..7500074 100644<br>
> --- a/lib/Target/AArch64/AArch64ISelLowering.h<br>
> +++ b/lib/Target/AArch64/AArch64ISelLowering.h<br>
> @@ -324,6 +324,7 @@ public:<br>
><br>
> bool shouldExpandAtomicInIR(Instruction *Inst) const override;<br>
><br>
> + bool emitLoadStackGuardNode() const override;<br>
> TargetLoweringBase::LegalizeTypeAction<br>
> getPreferredVectorAction(EVT VT) const override;<br>
> diff --git a/lib/Target/AArch64/AArch64InstrInfo.cpp b/lib/Target/AArch64/AArch64InstrInfo.cpp<br>
> index ce85b2c..bfb32d6 100644<br>
> --- a/lib/Target/AArch64/AArch64InstrInfo.cpp<br>
> +++ b/lib/Target/AArch64/AArch64InstrInfo.cpp<br>
> @@ -848,6 +848,29 @@ bool AArch64InstrInfo::optimizeCompareInstr(<br>
> return true;<br>
> }<br>
><br>
> +bool<br>
> +AArch64InstrInfo::expandPostRAPseudo(MachineBasicBlock::iterator MI) const {<br>
> + if (MI->getOpcode() != TargetOpcode::LOAD_STACK_GUARD)<br>
> + return false;<br>
> +<br>
> + assert(Subtarget.isTargetMachO() &&<br>
> + "LOAD_STACK_GUARD currently supported only for MachO.”);<br>
<br>
At least for AArch64, this expansion doesn’t look very specific to MachO. How much harder would it be to make it general? Or are you just being cautious here because you can’t test the other platforms?<br>
<br></blockquote><div><br></div><div>To make this more general for AArch64, I have to cover all the cases that are covered in AArch64TargetLowering::lowerGlobalAddress. It's probably not hard, I think.</div><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> +<br>
> + MachineBasicBlock &MBB = *MI->getParent();<br>
> + DebugLoc DL = MI->getDebugLoc();<br>
> + unsigned Reg = MI->getOperand(0).getReg();<br>
> + const GlobalValue *GV =<br>
> + cast<GlobalValue>((*MI->memoperands_begin())->getValue());<br>
> + MachineOperand GA = MachineOperand::CreateGA(GV, 0, AArch64II::MO_GOT);<br>
<br>
I noticed that you’re using MachineOperand::CreateGA throughout this patch, but almost everywhere else we use MIB’s addGlobalAddress method instead. I don’t see much difference but unless there’s some reason to prefer direct access to CreateGA, it would be better to follow the usual convention.<br>
<br>
> + BuildMI(MBB, MI, DL, get(AArch64::LOADgot), Reg).addOperand(GA);<br>
> + BuildMI(MBB, MI, DL, get(AArch64::LDRXui), Reg)<br>
> + .addReg(Reg, RegState::Kill).addImm(0)<br>
> + .addMemOperand(*MI->memoperands_begin());<br>
> + MBB.erase(MI);<br>
> +<br>
> + return true;<br>
> +}<br>
> +<br>
> /// Return true if this is this instruction has a non-zero immediate<br>
> bool AArch64InstrInfo::hasShiftedReg(const MachineInstr *MI) const {<br>
> switch (MI->getOpcode()) {<br>
><br>
<br>
<br>
The rest of this looks fine.</blockquote></div><br></div></div>