[ARM] implement support for TLS register based stack protector

[ardb]

Summary

Implement support for loading the stack canary from a memory location held in
the TLS register, with an optional offset applied. This is used by the Linux
kernel to implement per-task stack canaries, which is impossible on SMP systems
when using a global variable for the stack canary.

Suggestions for how to test this and where to insert the test code into the
tree are kindly welcomed.

[nickdesaulniers]

Nice job! This looks like it hits every place that I was looking at for this.

In terms of tests, https://reviews.llvm.org/D100919 and https://reviews.llvm.org/D102742 are probably interesting.

In particular, we should test that clang no longer rejects -mstack-protector-guard=tls -mstack-protector-guard-offset=0 for --target=arm-linux-gnueabihf and --target=arm-linux-gnueabihf -mthumb. (clang/test/Driver/stack-protector-guard.c
)

Then we should test

target triple = "arm-unknown-linux-gnueabihf"

declare void @baz(i32*)

define void @foo(i64 %t) sspstrong {
  %vla = alloca i32, i64 %t, align 4
  call void @baz(i32* nonnull %vla)
  ret void
}
!llvm.module.flags = !{!1, !2}
!1 = !{i32 2, !"stack-protector-guard", !"tls"}
!2 = !{i32 2, !"stack-protector-guard-offset", !"0"}

generates mrc (and test again with "thumbv7-linux-gnu"). We can use -mtriple= flags to llc rather than hard coding the triple in IR. (create llvm/test/CodeGen/ARM/stack-guard-sysreg.ll)

This also involves implementing LOAD_STACK_GUARD for other ARM targets than
Mach-O.

It might be worthwhile to break this up into 2 commits; one that does just that, so that we can isolate any test changes or possible build breakages to that, then have this patch implementing support for tls stack guards on top.

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp

4900

I think we want to use Reg in this instruction, in order to load into the specified destination?

4904

do we need to add al?

llvm/lib/Target/ARM/ARMInstrInfo.cpp

102

do we have to worry about soft tp, ie. __aeabi_read_tp vs mrc?

[nickdesaulniers]

llvm/lib/Target/ARM/Thumb2InstrInfo.cpp

250

what about Thumb1InstrInfo::expandLoadStackGuard? Do we have mrc available in Thumb[1] as well?

[ardb]

I have split off the LOAD_STACK_GUARD changes into

[ARM] implement LOAD_STACK_GUARD for remaining targets

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp

4900

We use Reg, no? MRC does not take reg operands, but I pass it as the target register.

4904

It appears so.

llvm/lib/Target/ARM/ARMInstrInfo.cpp

102

I think we should only allow this hard TP is supported in the first place. I'll add a check somewhere.
Calling a helper in both the prologue and the epilogue for emulated TLS seems a bit too heavyweight to me, and the Linux kernel will not use it in that way anyway.

llvm/lib/Target/ARM/Thumb2InstrInfo.cpp

250

No Thumb1 does not have an encoding for MRC so we can ignore it here. It does mean we should not allow this feature to be turned out for such targets.

[ardb]

• split off LOAD_STACK_GUARD conversion
• deal with guard offsets >= 4096 bytes
• reject offsets < 0 or >= 1 MiB
• add backend test to check that the MRC/LDR sequence is emitted twice

[nickdesaulniers]

Codegen looks good, probably just need an additional conditional on ARMSubtarget::isReadTPHard(). With that and some more tests for cases we don't intend to support (thumb[1], soft tp) this LGTM. Great job @ardb !

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp

4900

ah, yep, sorry, LGTM.

llvm/lib/Target/ARM/ARMInstrInfo.cpp

102

I agree. Let's add a test that this only works with the -mattr=+read-tp-hard llc flag or "target-features"="+read-tp-hard" function attribute (one or the other, I don't think we need to exhaustively test both). But it would be good to verify what happens when +read-tp-hard is not set, in case someone else cares to add soft tp support here in the future. (They may never, it's more so to highlight whether such a change would be intentional).

ARMSubtarget::isReadTPHard() should be able to help us differentiate this case.

llvm/lib/Target/ARM/Thumb2InstrInfo.cpp

250

Let's add a test for such case.

llvm/test/CodeGen/ARM/stack-guard-tls.ll

5–11 ↗

(On Diff #383390)

You can convert --check-prefix=CHECK --check-prefix=CHECK-SMALL to --check-prefixes=CHECK,CHECK-SMALL.

[ardb]

llvm/lib/Target/ARM/ARMInstrInfo.cpp

102

I agree. Let's add a test that this only works with the -mattr=+read-tp-hard llc flag or "target-features"="+read-tp-hard" function attribute (one or the other, I don't think we need to exhaustively test both). But it would be good to verify what happens when +read-tp-hard is not set, in case someone else cares to add soft tp support here in the future. (They may never, it's more so to highlight whether such a change would be intentional).

ARMSubtarget::isReadTPHard() should be able to help us differentiate this case.

I could not figure out how to check this at command line parsing time. If we check it later, we basically crash the compiler, right? Not very elegant ...

llvm/lib/Target/ARM/Thumb2InstrInfo.cpp

250

Let's add a test for such case.

Same problem: I could not figure out how to decide early enough whether we are targetting Thumb1 or Thumb2/ARM

[peter.smith]

Adding ostannard to reviewers list. I'm going to be on vacation next week and Oliver is more familiar with this area than I am.

To prevent the option in Clang for targets that don't support Thumb2 it may be worth looking into clang/lib/Basic/Targets/ARM.cpp for example https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/Targets/ARM.cpp#L174

[ardb]

• add diagnostics to the frontend and asserts to the backend to ensure that the TLS stack protector is only used on target subarchs that implement the hardware TLS register to begin with
• ensure that the offset parameter is not omitted, as the default is INT_MAX which is out of bounds

[ardb]

• fix failure in newly added LLVM test

[nickdesaulniers]

clang/lib/Driver/ToolChains/Clang.cpp

3178–3180

Does GCC require these flags to be paired (-mstack-protector-guard= and -mstack-protector-guard-offset) ? Only for ARM and THUMB? Doesn't the offset only make sense for sysreg and tls, or global, too?

Seems to me like 0 would be a good default offset, if unspecified.

Perhaps this would good to check then warn on for all supported architectures, and perhaps as another child patch? (or just default to 0 and not require -mstack-protector-offset=).

3191–3192

Isn't this redundant/set elsewhere?

[ardb]

clang/lib/Driver/ToolChains/Clang.cpp

3178–3180

Does GCC require these flags to be paired (-mstack-protector-guard= and -mstack-protector-guard-offset) ? Only for ARM and THUMB? Doesn't the offset only make sense for sysreg and tls, or global, too?

GCC does not implement this for ARM yet. For AArch64, it requires all three options: guard, guard-reg and guard-offset.

Seems to me like 0 would be a good default offset, if unspecified.

The default is INT_MAX at the moment, and I wasn't sure if we can simply change that without breaking users.

Perhaps this would good to check then warn on for all supported architectures, and perhaps as another child patch? (or just default to 0 and not require -mstack-protector-offset=).

Yes, I think it makes sense to check this for AAch64 as well.

3191–3192

No, it is not. The alternative is requiring the user to pass -mtp=cp15 when using the TLS stack protector, which is silly because it is implied.

[nickdesaulniers]

clang/lib/Driver/ToolChains/Clang.cpp

3191–3192

Right. Perhaps we could emit diag::err_drv_argument_not_allowed_with if the user specified -mtp=soft with -mstack-protector-guard=tls.

It looks like a call to clang::driver::tools::arm::getReadTPMode() should being able to tell us whether someone explicitly tried to use soft.

[ardb]

• disallow -mtp=soft when TLS based stack protector is enabled

[ardb]

clang/lib/Driver/ToolChains/Clang.cpp

3191–3192

Right. Perhaps we could emit diag::err_drv_argument_not_allowed_with if the user specified -mtp=soft with -mstack-protector-guard=tls.

It looks like a call to clang::driver::tools::arm::getReadTPMode() should being able to tell us whether someone explicitly tried to use soft.

[nickdesaulniers]

Looks great! Nice job!

clang/lib/Driver/ToolChains/Clang.cpp

if (Str.startswith("ssp-buffer-size=")) {
       }
       A->claim();
     }
   }
 
   const std::string &TripleStr = EffectiveTriple.getTriple();
   if (Arg *A = Args.getLastArg(options::OPT_mstack_protector_guard_EQ)) {
     StringRef Value = A->getValue();
-    if (!EffectiveTriple.isX86() && !EffectiveTriple.isAArch64())
+    if (!EffectiveTriple.isX86() && !EffectiveTriple.isAArch64() &&
+        !EffectiveTriple.isARM() && !EffectiveTriple.isThumb())
       D.Diag(diag::err_drv_unsupported_opt_for_target)
           << A->getAsString(Args) << TripleStr;
-    if (EffectiveTriple.isX86() && Value != "tls" && Value != "global") {
+    if ((EffectiveTriple.isX86() ||
+         EffectiveTriple.isARM() ||
+         EffectiveTriple.isThumb()) &&
+        Value != "tls" && Value != "global") {
       D.Diag(diag::err_drv_invalid_value_with_suggestion)
           << A->getOption().getName() << Value << "tls global";
       return;
     }
     if (EffectiveTriple.isAArch64() && Value != "sysreg" && Value != "global") {
       D.Diag(diag::err_drv_invalid_value_with_suggestion)
           << A->getOption().getName() << Value << "sysreg global";
       return;
     }
     A->render(Args, CmdArgs);
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_mstack_protector_guard_offset_EQ)) {
     StringRef Value = A->getValue();
-    if (!EffectiveTriple.isX86() && !EffectiveTriple.isAArch64())
+    if (!EffectiveTriple.isX86() && !EffectiveTriple.isAArch64() &&
+        !EffectiveTriple.isARM() && !EffectiveTriple.isThumb())
       D.Diag(diag::err_drv_unsupported_opt_for_target)
           << A->getAsString(Args) << TripleStr;
     int Offset;
     if (Value.getAsInteger(10, Offset)) {
       D.Diag(diag::err_drv_invalid_value) << A->getOption().getName() << Value;
       return;
     }
     A->render(Args, CmdArgs);
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_mstack_protector_guard_reg_EQ)) {
     StringRef Value = A->getValue();
     if (!EffectiveTriple.isX86() && !EffectiveTriple.isAArch64())

clang/test/Driver/stack-protector-guard.c

 // INVALID-VALUE: error: invalid value 'local' in 'mstack-protector-guard=', expected one of: tls global
 
 // RUN: %clang -### -target x86_64-unknown-unknown -mstack-protector-guard-reg=fs %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-FS %s
 // RUN: %clang -### -target x86_64-unknown-unknown -mstack-protector-guard-reg=gs %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-GS %s
 
 // Invalid arch
-// RUN: not %clang -target arm-eabi-c -mstack-protector-guard=tls %s 2>&1 | \
+// RUN: not %clang -target powerpc64le-linux-gnu -mstack-protector-guard=tls %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=INVALID-ARCH %s
 // INVALID-ARCH: unsupported option '-mstack-protector-guard=tls' for target
 
 // RUN: not %clang -target powerpc64le-linux-gnu -mstack-protector-guard-reg=fs %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=INVALID-ARCH2 %s
 // INVALID-ARCH2: unsupported option '-mstack-protector-guard-reg=fs' for target
 
-// RUN: not %clang -target arm-linux-gnueabi -mstack-protector-guard-offset=10 %s 2>&1 | \
+// RUN: not %clang -target powerpc64le-linux-gnu -mstack-protector-guard-offset=10 %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=INVALID-ARCH3 %s
 // INVALID-ARCH3: unsupported option '-mstack-protector-guard-offset=10' for target
 
 // Invalid option value
 // RUN: not %clang -target x86_64-unknown-unknown -c -mstack-protector-guard-reg=cs %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=INVALID-REG %s
 // RUN: not %clang -target x86_64-unknown-unknown -c -mstack-protector-guard-reg=ds %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=INVALID-REG %s

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp

for (const auto &Reg : ScratchRegs) {
     STM.addReg(Reg, RegState::Kill);
   }
 
   BB->erase(MI);
 }
 
 bool ARMBaseInstrInfo::expandPostRAPseudo(MachineInstr &MI) const {
   if (MI.getOpcode() == TargetOpcode::LOAD_STACK_GUARD) {
-    assert(getSubtarget().getTargetTriple().isOSBinFormatMachO() &&
-           "LOAD_STACK_GUARD currently supported only for MachO.");
     expandLoadStackGuard(MI);
     MI.getParent()->erase(MI);
     return true;
   }
 
   if (MI.getOpcode() == ARM::MEMCPY) {
     expandMEMCPY(MI);
     return true;
if ((MI.getOperand(4).getImm() != 2 && MI.getOperand(4).getImm() != 3) ||
         MI.getOperand(4).getImm() != MI.getOperand(5).getImm() + 2) {
       ErrInfo = "Incorrect array index for MVE_VMOV_q_rr";
       return false;
     }
   }
   return true;
 }
 
-// LoadStackGuard has so far only been implemented for MachO. Different code
-// sequence is needed for other targets.
 void ARMBaseInstrInfo::expandLoadStackGuardBase(MachineBasicBlock::iterator MI,
                                                 unsigned LoadImmOpc,
                                                 unsigned LoadOpc) const {
   assert(!Subtarget.isROPI() && !Subtarget.isRWPI() &&
          "ROPI/RWPI not currently supported with stack guard");
 
   MachineBasicBlock &MBB = *MI->getParent();
   DebugLoc DL = MI->getDebugLoc();
   Register Reg = MI->getOperand(0).getReg();
+  MachineInstrBuilder MIB;
+  int Offset = 0;
+
+  if (LoadImmOpc == ARM::MRC || LoadImmOpc == ARM::t2MRC) {
+    BuildMI(MBB, MI, DL, get(LoadImmOpc), Reg)
+        .addImm(15)
+        .addImm(0)
+        .addImm(13)
+        .addImm(0)
+        .addImm(3)
+        .add(predOps(ARMCC::AL));
+
+    Module &M = *MBB.getParent()->getFunction().getParent();
+    Offset = M.getStackProtectorGuardOffset();
+  } else {
     const GlobalValue *GV =
         cast<GlobalValue>((*MI->memoperands_begin())->getValue());
-  MachineInstrBuilder MIB;
+
+    unsigned TargetFlags = ARMII::MO_NO_FLAG;
+    if (Subtarget.isTargetMachO())
+      TargetFlags |= ARMII::MO_NONLAZY;
+    else if (Subtarget.isGVInGOT(GV))
+      TargetFlags |= ARMII::MO_GOT;
 
     BuildMI(MBB, MI, DL, get(LoadImmOpc), Reg)
-      .addGlobalAddress(GV, 0, ARMII::MO_NONLAZY);
+        .addGlobalAddress(GV, 0, TargetFlags);
 
     if (Subtarget.isGVIndirectSymbol(GV)) {
       MIB = BuildMI(MBB, MI, DL, get(LoadOpc), Reg);
       MIB.addReg(Reg, RegState::Kill).addImm(0);
       auto Flags = MachineMemOperand::MOLoad |
                    MachineMemOperand::MODereferenceable |
                    MachineMemOperand::MOInvariant;
       MachineMemOperand *MMO = MBB.getParent()->getMachineMemOperand(
           MachinePointerInfo::getGOT(*MBB.getParent()), Flags, 4, Align(4));
       MIB.addMemOperand(MMO).add(predOps(ARMCC::AL));
     }
+  }
 
   MIB = BuildMI(MBB, MI, DL, get(LoadOpc), Reg);
   MIB.addReg(Reg, RegState::Kill)
-      .addImm(0)
+      .addImm(Offset)
       .cloneMemRefs(*MI)
       .add(predOps(ARMCC::AL));
 }
 
 bool
 ARMBaseInstrInfo::isFpMLxInstruction(unsigned Opcode, unsigned &MulOpc,
                                      unsigned &AddSubOpc,
                                      bool &NegAcc, bool &HasLane) const {

llvm/lib/Target/ARM/ARMISelLowering.cpp

ARMTargetLowering::shouldExpandAtomicCmpXchgInIR(AtomicCmpXchgInst *AI) const {
   return AtomicExpansionKind::None;
 }
 
 bool ARMTargetLowering::shouldInsertFencesForAtomic(
     const Instruction *I) const {
   return InsertFencesForAtomic;
 }
 
-// This has so far only been implemented for MachO.
 bool ARMTargetLowering::useLoadStackGuardNode() const {
-  return Subtarget->isTargetMachO();
+  return true;
 }
 
 void ARMTargetLowering::insertSSPDeclarations(Module &M) const {
   if (!Subtarget->getTargetTriple().isWindowsMSVCEnvironment())
     return TargetLowering::insertSSPDeclarations(M);
 
   // MSVC CRT has a global variable holding security cookie.
   M.getOrInsertGlobal("__security_cookie",

llvm/lib/Target/ARM/ARMInstrInfo.cpp

unsigned ARMInstrInfo::getUnindexedOpcode(unsigned Opc) const {
 
   return 0;
 }
 
 void ARMInstrInfo::expandLoadStackGuard(MachineBasicBlock::iterator MI) const {
   MachineFunction &MF = *MI->getParent()->getParent();
   const ARMSubtarget &Subtarget = MF.getSubtarget<ARMSubtarget>();
   const TargetMachine &TM = MF.getTarget();
+  MachineBasicBlock &MBB = *MI->getParent();
+  Module &M = *MBB.getParent()->getFunction().getParent();
+
+  if (M.getStackProtectorGuard() == "tls") {
+    expandLoadStackGuardBase(MI, ARM::MRC, ARM::LDRi12);
+    return;
+  }
 
-  if (!Subtarget.useMovt()) {
+  const GlobalValue *GV =
+      cast<GlobalValue>((*MI->memoperands_begin())->getValue());
+
+  if (!Subtarget.useMovt() || Subtarget.isGVInGOT(GV)) {
     if (TM.isPositionIndependent())
       expandLoadStackGuardBase(MI, ARM::LDRLIT_ga_pcrel, ARM::LDRi12);
     else
       expandLoadStackGuardBase(MI, ARM::LDRLIT_ga_abs, ARM::LDRi12);
     return;
   }
 
   if (!TM.isPositionIndependent()) {
     expandLoadStackGuardBase(MI, ARM::MOVi32imm, ARM::LDRi12);
     return;
   }
 
-  const GlobalValue *GV =
-      cast<GlobalValue>((*MI->memoperands_begin())->getValue());
-
   if (!Subtarget.isGVIndirectSymbol(GV)) {
     expandLoadStackGuardBase(MI, ARM::MOV_ga_pcrel, ARM::LDRi12);
     return;
   }
 
-  MachineBasicBlock &MBB = *MI->getParent();
   DebugLoc DL = MI->getDebugLoc();
   Register Reg = MI->getOperand(0).getReg();
   MachineInstrBuilder MIB;
 
   MIB = BuildMI(MBB, MI, DL, get(ARM::MOV_ga_pcrel_ldr), Reg)
             .addGlobalAddress(GV, 0, ARMII::MO_NONLAZY);
   auto Flags = MachineMemOperand::MOLoad |
                MachineMemOperand::MODereferenceable |

llvm/lib/Target/ARM/Thumb2InstrInfo.cpp

if (ARM::GPRPairRegClass.hasSubClassEq(RC)) {
     if (Register::isPhysicalRegister(DestReg))
       MIB.addReg(DestReg, RegState::ImplicitDefine);
     return;
   }
 
   ARMBaseInstrInfo::loadRegFromStackSlot(MBB, I, DestReg, FI, RC, TRI);
 }
 
 void Thumb2InstrInfo::expandLoadStackGuard(
     MachineBasicBlock::iterator MI) const {
   MachineFunction &MF = *MI->getParent()->getParent();
-  if (MF.getTarget().isPositionIndependent())
+  MachineBasicBlock &MBB = *MI->getParent();
+  Module &M = *MBB.getParent()->getFunction().getParent();
+
+  if (M.getStackProtectorGuard() == "tls") {
+    expandLoadStackGuardBase(MI, ARM::t2MRC, ARM::t2LDRi12);
+    return;
+  }
+
+  const GlobalValue *GV =
+    cast<GlobalValue>((*MI->memoperands_begin())->getValue());
+
+  if (MF.getSubtarget<ARMSubtarget>().isGVInGOT(GV))
+    expandLoadStackGuardBase(MI, ARM::tLDRLIT_ga_pcrel, ARM::t2LDRi12);
+  else if (MF.getTarget().isPositionIndependent())
     expandLoadStackGuardBase(MI, ARM::t2MOV_ga_pcrel, ARM::t2LDRi12);
   else
     expandLoadStackGuardBase(MI, ARM::t2MOVi32imm, ARM::t2LDRi12);
 }
 
 MachineInstr *Thumb2InstrInfo::commuteInstructionImpl(MachineInstr &MI,
                                                       bool NewMI,
                                                       unsigned OpIdx1,