[PATCH][ARM] Prevent stack guard address from being spilled

Akira Hatanaka ahatanak at gmail.com
Fri Jul 25 11:45:08 PDT 2014


I realized that it might be possible to use this scheme for 32-bit x86 too.
It looks like 32-bit x86 uses call+pop to get the PC and emulate
PC-relative load, in which case we can remat the stack guard address or the
value.

calll L0$pb

L0$pb:

popl %eax

movl L___stack_chk_guard$non_lazy_ptr-L0$pb(%eax), %eax

movl %eax, -1052(%ebp)       ## 4-byte Spill

movl (%eax), %eax

I'll add support for 32-bit x86 and update comments in a follow-up patch.



On Fri, Jul 25, 2014 at 9:20 AM, Bob Wilson <bob.wilson at apple.com> wrote:

>
> On Jul 18, 2014, at 4:33 PM, Akira Hatanaka <ahatanak at gmail.com> wrote:
>
> 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.
>
>
> Thanks. This looks good to commit now.
>
> Can you add comments to ARMTargetLowering::useLoadStackGuardNode
> and X86TargetLowering::useLoadStackGuardNode to explain why this is only
> enabled for certain targets? I.E., it provides better security but has so
> far only been implemented for MachO targets on ARM and x86_64, and that for
> 32-bit x86 it doesn’t work because we can’t remat without an extra
> register. You can add those comments in a follow-up patch if you prefer.
>
>
>
> 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.
>>
>>
>>
> <stack_guard3.patch>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140725/fe96678e/attachment.html>


More information about the llvm-commits mailing list