[PATCH][ARM] Prevent stack guard address from being spilled
Akira Hatanaka
ahatanak at gmail.com
Mon Jul 14 22:49:43 PDT 2014
Updated patch attached.
I made the following changes in addition to fixing the problems Bob pointed
out:
- In SelectionDAGBuilder::visitIntrinsicCall, replaced the call to
LoadInst::getAlignment (which was returning 0) with
SelectionDAG::getEVTAlignment to get the load instruction's alignment.
- Changed STATIC to STATIC-NEXT in test case
CodeGen/ARM/stack_guard_remat.ll.
On Mon, Jul 14, 2014 at 12:43 PM, Bob Wilson <bob.wilson at apple.com> wrote:
>
> > On Jul 11, 2014, at 3:34 PM, Akira Hatanaka <ahatanak at gmail.com> wrote:
> >
> > 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.
>
> I have a few comments inline below….
>
> > diff --git a/include/llvm/Target/Target.td
> b/include/llvm/Target/Target.td
> > index f77cc7a..26e1242 100644
> > --- a/include/llvm/Target/Target.td
> > +++ b/include/llvm/Target/Target.td
> > @@ -841,6 +841,14 @@ def PATCHPOINT : Instruction {
> > let mayLoad = 1;
> > let usesCustomInserter = 1;
> > }
> > +def LOAD_STACK_GUARD : Instruction {
> > + let OutOperandList = (outs ptr_rc:$dst);
> > + let InOperandList = (ins);
> > + let mayLoad = 1;
> > + bit isReMaterializable = 1;
> > + let neverHasSideEffects = 1;
>
> neverHasSideEffects is deprecated. This should be "hasSideEffects = 0”.
>
> > + bit isPseudo = 1;
> > +}
> > }
> >
> >
> //===----------------------------------------------------------------------===//
> > diff --git a/include/llvm/Target/TargetLowering.h
> b/include/llvm/Target/TargetLowering.h
> > index 180f0f2..ef0fc27 100644
> > --- a/include/llvm/Target/TargetLowering.h
> > +++ b/include/llvm/Target/TargetLowering.h
> > @@ -2589,6 +2589,13 @@ public:
> > /// ARM 's' setting instructions.
> > virtual void
> > AdjustInstrPostInstrSelection(MachineInstr *MI, SDNode *Node) const;
> > +
> > + /// If this function returns true, SelectionDAGBuilder emits a
> > + /// LOAD_STACK_GUARD node when it is lowering
> Intrinsic::stackprotector.
> > + virtual bool emitLoadStackGuardNode() const {
>
> 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?
>
> > + return false;
> > + }
> > +
>
> There’s no need to add a blank line here.
>
> > };
> >
> > /// Given an LLVM IR type and return type attributes, compute the
> return value
> >
> > diff --git a/include/llvm/Target/TargetOpcodes.h
> b/include/llvm/Target/TargetOpcodes.h
> > index abb3eca..1fbd2ae 100644
> > --- a/include/llvm/Target/TargetOpcodes.h
> > +++ b/include/llvm/Target/TargetOpcodes.h
> > @@ -104,7 +104,13 @@ enum {
> > /// support optimizations for dynamic languages (such as javascript)
> that
> > /// rewrite calls to runtimes with more efficient code sequences.
> > /// This also implies a stack map.
> > - PATCHPOINT = 18
> > + PATCHPOINT = 18,
> > +
> > + /// This pseudo-instruction loads the stack guard value. Targets
> which need
> > + /// to prevent the stack guard value or address from being spilled to
> the
> > + /// stack should override TargetLowering::emitLoadStackGuardNode and
> > + /// additionally expand this pseudo after register allocation.
> > + LOAD_STACK_GUARD = 19
> > };
> > } // end namespace TargetOpcode
> > } // end namespace llvm
> >
> > diff --git a/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
> b/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
> > index 28d8e98..0d89776 100644
> > --- a/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
> > +++ b/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
> > @@ -1811,9 +1811,19 @@ void
> SelectionDAGBuilder::visitSPDescriptorParent(StackProtectorDescriptor &SPD,
> >
> > unsigned Align =
> > TLI->getDataLayout()->getPrefTypeAlignment(IRGuard->getType());
> > - SDValue Guard = DAG.getLoad(PtrTy, getCurSDLoc(), DAG.getEntryNode(),
> > - GuardPtr, MachinePointerInfo(IRGuard, 0),
> > - true, false, false, Align);
> > +
> > + SDValue Guard;
> > +
> > + // If emitLoadStackGuardNode returns true, retrieve the guard value
> from
> > + // the virtual register holding the value. Otherwise, emit a volatile
> load
> > + // to retrieve the stack guard value.
> > + if (TLI->emitLoadStackGuardNode())
> > + Guard = DAG.getCopyFromReg(DAG.getEntryNode(), getCurSDLoc(),
> > + SPD.getGuardReg(), PtrTy);
> > + else
> > + Guard = DAG.getLoad(PtrTy, getCurSDLoc(), DAG.getEntryNode(),
> > + GuardPtr, MachinePointerInfo(IRGuard, 0),
> > + true, false, false, Align);
> >
> > SDValue StackSlot = DAG.getLoad(PtrTy, getCurSDLoc(),
> DAG.getEntryNode(),
> > StackSlotPtr,
> > @@ -5220,8 +5230,35 @@ SelectionDAGBuilder::visitIntrinsicCall(const
> CallInst &I, unsigned Intrinsic) {
> > MachineFunction &MF = DAG.getMachineFunction();
> > MachineFrameInfo *MFI = MF.getFrameInfo();
> > EVT PtrTy = TLI->getPointerTy();
> > + SDValue Src, Chain = getRoot();
>
> > +
> > + if (TLI->emitLoadStackGuardNode()) {
> > + // Emit a LOAD_STACK_GUARD node.
> > + MachineSDNode *Node =
> DAG.getMachineNode(TargetOpcode::LOAD_STACK_GUARD,
> > + sdl, PtrTy, Chain);
> > + LoadInst *LI = cast<LoadInst>(I.getArgOperand(0));
> > + MachinePointerInfo MPInfo(LI->getPointerOperand());
> > + MachineInstr::mmo_iterator MemRefs = MF.allocateMemRefsArray(1);
> > + unsigned Flags = MachineMemOperand::MOLoad |
> > + MachineMemOperand::MOInvariant;
> > + *MemRefs = MF.getMachineMemOperand(MPInfo, Flags,
> > + PtrTy.getSizeInBits() / 8,
> > + LI->getAlignment());
> > + Node->setMemRefs(MemRefs, MemRefs + 1);
> > +
> > + // Copy the guard value to a virtual register so that it can be
> > + // retrieved in the epilogue.
> > + Src = SDValue(Node, 0);
> > + const TargetRegisterClass *RC =
> > + TLI->getRegClassFor(Src.getSimpleValueType());
> > + unsigned Reg = MF.getRegInfo().createVirtualRegister(RC);
> > +
> > + SPDescriptor.setGuardReg(Reg);
> > + Chain = DAG.getCopyToReg(Chain, sdl, Reg, Src);
> > + } else {
> > + Src = getValue(I.getArgOperand(0)); // The guard's value.
> > + }
> >
> > - SDValue Src = getValue(I.getArgOperand(0)); // The guard's value.
> > AllocaInst *Slot = cast<AllocaInst>(I.getArgOperand(1));
> >
> > int FI = FuncInfo.StaticAllocaMap[Slot];
> > @@ -5230,7 +5267,7 @@ SelectionDAGBuilder::visitIntrinsicCall(const
> CallInst &I, unsigned Intrinsic) {
> > SDValue FIN = DAG.getFrameIndex(FI, PtrTy);
> >
> > // Store the stack protector onto the stack.
> > - Res = DAG.getStore(getRoot(), sdl, Src, FIN,
> > + Res = DAG.getStore(Chain, sdl, Src, FIN,
> > MachinePointerInfo::getFixedStack(FI),
> > true, false, 0);
> > setValue(&I, Res);
> > diff --git a/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h
> b/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h
> > index 84679f9..cf0846f 100644
> > --- a/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h
> > +++ b/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h
> > @@ -397,7 +397,8 @@ private:
> > class StackProtectorDescriptor {
> > public:
> > StackProtectorDescriptor() : ParentMBB(nullptr),
> SuccessMBB(nullptr),
> > - FailureMBB(nullptr), Guard(nullptr) { }
> > + FailureMBB(nullptr), Guard(nullptr),
> > + GuardReg(0) { }
> > ~StackProtectorDescriptor() { }
> >
> > /// Returns true if all fields of the stack protector descriptor are
> > @@ -455,6 +456,9 @@ private:
> > MachineBasicBlock *getFailureMBB() { return FailureMBB; }
> > const Value *getGuard() { return Guard; }
> >
> > + unsigned getGuardReg() const { return GuardReg; }
> > + void setGuardReg(unsigned R) { GuardReg = R; }
> > +
> > private:
> > /// The basic block for which we are generating the stack protector.
> > ///
> > @@ -477,6 +481,9 @@ private:
> > /// stack protector stack slot.
> > const Value *Guard;
> >
> > + /// The virtual register holding the stack guard value.
> > + unsigned GuardReg;
> > +
> > /// Add a successor machine basic block to ParentMBB. If the
> successor mbb
> > /// has not been created yet (i.e. if SuccMBB = 0), then the
> machine basic
> > /// block will be created.
> > diff --git a/lib/Target/AArch64/AArch64ISelLowering.cpp
> b/lib/Target/AArch64/AArch64ISelLowering.cpp
> > index 07ff093..97bec0d 100644
> > --- a/lib/Target/AArch64/AArch64ISelLowering.cpp
> > +++ b/lib/Target/AArch64/AArch64ISelLowering.cpp
> > @@ -7914,6 +7914,10 @@ bool
> AArch64TargetLowering::shouldExpandAtomicInIR(Instruction *Inst) const {
> > return Inst->getType()->getPrimitiveSizeInBits() <= 128;
> > }
> >
> > +bool AArch64TargetLowering::emitLoadStackGuardNode() const {
> > + return Subtarget->isTargetMachO();
> > +}
> > +
> > TargetLoweringBase::LegalizeTypeAction
> > AArch64TargetLowering::getPreferredVectorAction(EVT VT) const {
> > MVT SVT = VT.getSimpleVT();
> > diff --git a/lib/Target/AArch64/AArch64ISelLowering.h
> b/lib/Target/AArch64/AArch64ISelLowering.h
> > index cb0b9ef..7500074 100644
> > --- a/lib/Target/AArch64/AArch64ISelLowering.h
> > +++ b/lib/Target/AArch64/AArch64ISelLowering.h
> > @@ -324,6 +324,7 @@ public:
> >
> > bool shouldExpandAtomicInIR(Instruction *Inst) const override;
> >
> > + bool emitLoadStackGuardNode() const override;
> > TargetLoweringBase::LegalizeTypeAction
> > getPreferredVectorAction(EVT VT) const override;
> > diff --git a/lib/Target/AArch64/AArch64InstrInfo.cpp
> b/lib/Target/AArch64/AArch64InstrInfo.cpp
> > index ce85b2c..bfb32d6 100644
> > --- a/lib/Target/AArch64/AArch64InstrInfo.cpp
> > +++ b/lib/Target/AArch64/AArch64InstrInfo.cpp
> > @@ -848,6 +848,29 @@ bool AArch64InstrInfo::optimizeCompareInstr(
> > return true;
> > }
> >
> > +bool
> > +AArch64InstrInfo::expandPostRAPseudo(MachineBasicBlock::iterator MI)
> const {
> > + if (MI->getOpcode() != TargetOpcode::LOAD_STACK_GUARD)
> > + return false;
> > +
> > + assert(Subtarget.isTargetMachO() &&
> > + "LOAD_STACK_GUARD currently supported only for MachO.”);
>
> 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?
>
> > +
> > + MachineBasicBlock &MBB = *MI->getParent();
> > + DebugLoc DL = MI->getDebugLoc();
> > + unsigned Reg = MI->getOperand(0).getReg();
> > + const GlobalValue *GV =
> > + cast<GlobalValue>((*MI->memoperands_begin())->getValue());
> > + MachineOperand GA = MachineOperand::CreateGA(GV, 0,
> AArch64II::MO_GOT);
>
> 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.
>
> > + BuildMI(MBB, MI, DL, get(AArch64::LOADgot), Reg).addOperand(GA);
> > + BuildMI(MBB, MI, DL, get(AArch64::LDRXui), Reg)
> > + .addReg(Reg, RegState::Kill).addImm(0)
> > + .addMemOperand(*MI->memoperands_begin());
> > + MBB.erase(MI);
> > +
> > + return true;
> > +}
> > +
> > /// Return true if this is this instruction has a non-zero immediate
> > bool AArch64InstrInfo::hasShiftedReg(const MachineInstr *MI) const {
> > switch (MI->getOpcode()) {
> >
>
>
> The rest of this looks fine.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140714/4dc6c85e/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: stack_guard2.patch
Type: application/octet-stream
Size: 31467 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140714/4dc6c85e/attachment.obj>
More information about the llvm-commits
mailing list