[PATCH][ARM] Prevent stack guard address from being spilled
Akira Hatanaka
ahatanak at gmail.com
Fri Jul 18 16:33:06 PDT 2014
Updated patch which includes the following changes is attached.
- Disabled using LOAD_STACK_GUARD if target is 32-bit x86. 32-bit x86 needs
an extra register to load the stack guard value, which prevents it from
being rematted.
- Added test cases for x86 and thumb1.
- Refactored and simplified functions in ARMBaseInstrInfo which expand the
LOAD_STACK_GUARD pseudo.
- Renamed function parameters and fixed indentations.
On Mon, Jul 14, 2014 at 10:49 PM, Akira Hatanaka <ahatanak at gmail.com> wrote:
> 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/20140718/1cb25745/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: stack_guard3.patch
Type: application/octet-stream
Size: 34155 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140718/1cb25745/attachment.obj>
More information about the llvm-commits
mailing list