Clear the stack protector after checking it

[Flakebi]

Summary

SSPs cannot be leaked from the stack through uninitialized memory anymore, because they are removed after they are used.
This makes it in some cases harder for attackers to circumvent ssps and it has no (measurable) performance costs.

I developed this patch as part of my bachelor thesis. Therefore I measured the performance impact in a microbenchmark, which called a function with a ssp an a loop
(the benchmark executable did nothing else, the function wrote a single value into a stack local array so the ssp was generated).
In this benchmark, no change in performance was visible, it took as long as before.

[probinson]

+ @eugenis as this seems like his area.

[eugenis]

@pcc, @kcc I'm not sure about this. It adds one extra instruction per function, which is not exactly free. Do you have any estimates on how much additional protection does it provide? My gut feeling is that it would be only a minor speed bump in most cases.

[Flakebi]

It took a while, now the code, figures and a more detailed explanation are online: https://flakebi.de/uni-items/ba/#clear-ssp
Even with this microbenchmark, I was not able to measure a difference in performance in comparison to llvm without this patch.

[eugenis]

This looks fine to me in general. @timshen @etienneb @rnk

Please do the same thing in SDAG stack protector, otherwise you are missing some random configurations like glibc on aarch64, or arm32 on android.
Please add tests.

lib/CodeGen/SafeStack.cpp

464 ↗

(On Diff #136937)

Add a comment /*isVolatile*/ here and below.

[Flakebi]

The ssp is now set to zero in the SDAG variant and when a stack guard check function is used. I am not familiar with the selection DAG so please correct me if it can be improved.
I also added a test as requested.

[Flakebi]

Hi,

is there anything missing for this pull request?

[eugenis]

Sorry, but I'm not convinced that the overhead of this change is justified by the security benefit it provides.
I've measured code size overhead (using Chromium on Android as a benchmark) at 0.4%, which is not huge, but still significant.
On the other hand, I'm not at all sure that this would be anything but an inconvenience for an attacker. There are multiple copies of the cookie on the stack anyway (one per every live frame!). Also, taking advantage of the cookies left below SP will become even harder with the new -ftrivial-auto-var-init feature.

[eugenis]

Oh btw please upload diffs with full context in the future:
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

llvm/lib/CodeGen/SafeStack.cpp

                                 AllocaInst *StackGuardSlot, Value *StackGuard) {
   Value *V = IRB.CreateLoad(StackPtrTy, StackGuardSlot);
   Value *Cmp = IRB.CreateICmpNE(StackGuard, V);
+  PointerType *GuardPtrType = dyn_cast<PointerType>(StackGuard->getType());
+  // Zero the protector after it was checked to prohibit leaks.
+  IRB.CreateStore(ConstantPointerNull::get(GuardPtrType), StackGuardSlot,
+                  /* isVolatile */ true);
 
   auto SuccessProb = BranchProbabilityInfo::getBranchProbStackProtector(true);
   auto FailureProb = BranchProbabilityInfo::getBranchProbStackProtector(false);

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

       MachinePointerInfo::getFixedStack(DAG.getMachineFunction(), FI), Align,
       MachineMemOperand::MOVolatile);
 
+  // Zero the protector after it was checked to prohibit leaks.
+  EVT VT = GuardVal.getValueType();
+  SDValue Store = DAG.getStore(SDValue(GuardVal.getNode(), 1), dl,
+                               DAG.getConstant(0, dl, VT),
+                               StackSlotPtr, MachinePointerInfo::getFixedStack(
+                                 DAG.getMachineFunction(), FI),
+                               Align, MachineMemOperand::MOVolatile);
+
+
   if (TLI.useStackGuardXorFP())
     GuardVal = TLI.emitStackGuardXorFP(DAG, GuardVal, dl);
 

                    getValue(GuardCheckFn), std::move(Args));
 
     std::pair<SDValue, SDValue> Result = TLI.LowerCallTo(CLI);
-    DAG.setRoot(Result.second);
+
+    SmallVector<SDValue, 2> Chains(2);
+    Chains[0] = Store;
+    Chains[1] = Result.second;
+    SDValue Chain = DAG.getNode(ISD::TokenFactor, dl, MVT::Other, Chains);
+
+    DAG.setRoot(Chain);
     return;
   }
 

   }
 
   // Perform the comparison via a subtract/getsetcc.
-  EVT VT = Guard.getValueType();
   SDValue Sub = DAG.getNode(ISD::SUB, dl, VT, Guard, GuardVal);
 
   SDValue Cmp = DAG.getSetCC(dl, TLI.getSetCCResultType(DAG.getDataLayout(),

   // If the sub is not 0, then we know the guard/stackslot do not equal, so
   // branch to failure MBB.
   SDValue BrCond = DAG.getNode(ISD::BRCOND, dl,
-                               MVT::Other, GuardVal.getOperand(0),
+                               MVT::Other, Store,
                                Cmp, DAG.getBasicBlock(SPD.getFailureMBB()));
   // Otherwise branch to success MBB.
   SDValue Br = DAG.getNode(ISD::BR, dl,

   SDVTList VTs = DAG.getVTList(ValueVTs);
   SDValue Result;
   if (Opcode == ISD::STRICT_FP_ROUND)
-    Result = DAG.getNode(Opcode, sdl, VTs,
-                          { Chain, getValue(FPI.getArgOperand(0)),
+    Result = DAG.getNode(Opcode, sdl, VTs,
+                          { Chain, getValue(FPI.getArgOperand(0)),
                                DAG.getTargetConstant(0, sdl,
                                TLI.getPointerTy(DAG.getDataLayout())) });
   else if (FPI.isUnaryOp())

llvm/lib/CodeGen/StackProtector.cpp

       CallInst *Call = B.CreateCall(GuardCheck, {Guard});
       Call->setAttributes(GuardCheck->getAttributes());
       Call->setCallingConv(GuardCheck->getCallingConv());
+
+      PointerType *GuardPtrType = dyn_cast<PointerType>(Guard->getType());
+      // Zero the protector after it was checked to prohibit leaks.
+      B.CreateStore(ConstantPointerNull::get(GuardPtrType), AI,
+                    /* isVolatile */ true);
     } else {
       // Generate the epilogue with inline instrumentation.
       // If we do not support SelectionDAG based tail calls, generate IR level

       //     %1 = <stack guard>
       //     %2 = load StackGuardSlot
       //     %3 = cmp i1 %1, %2
+      //     store 0, StackGuardSlot
       //     br i1 %3, label %SP_return, label %CallStackCheckFailBlk
       //
       //   SP_return:

       Value *Guard = getStackGuard(TLI, M, B);
       LoadInst *LI2 = B.CreateLoad(B.getInt8PtrTy(), AI, true);
       Value *Cmp = B.CreateICmpEQ(Guard, LI2);
+      PointerType *GuardPtrType = dyn_cast<PointerType>(Guard->getType());
+      // Zero the protector after it was checked to prohibit leaks.
+      B.CreateStore(ConstantPointerNull::get(GuardPtrType), AI,
+                    /* isVolatile */ true);
       auto SuccessProb =
           BranchProbabilityInfo::getBranchProbStackProtector(true);
       auto FailureProb =

llvm/test/CodeGen/X86/stack-protector-zero.ll

+; RUN: llc -mtriple=i386-pc-linux-gnu < %s -o - | FileCheck --check-prefix=LINUX-I386 %s
+; RUN: llc -mtriple=x86_64-pc-linux-gnu < %s -o - | FileCheck --check-prefix=LINUX-X64 %s
+; RUN: llc -code-model=kernel -mtriple=x86_64-pc-linux-gnu < %s -o - | FileCheck --check-prefix=LINUX-KERNEL-X64 %s
+; RUN: llc -mtriple=x86_64-apple-darwin < %s -o - | FileCheck --check-prefix=DARWIN-X64 %s
+; RUN: llc -mtriple=amd64-pc-openbsd < %s -o - | FileCheck --check-prefix=OPENBSD-AMD64 %s
+; RUN: llc -mtriple=i386-pc-windows-msvc < %s -o - | FileCheck -check-prefix=MSVC-I386 %s
+; RUN: llc -mtriple=x86_64-w64-mingw32 < %s -o - | FileCheck --check-prefix=MINGW-X64 %s
+; RUN: llc -mtriple=x86_64-pc-linux-gnu < %s -o - | FileCheck --check-prefix=IGNORE_INTRIN %s
+
+@.str = private unnamed_addr constant [4 x i8] c"%s\0A\00", align 1
+
+; test1: array of [16 x i8]
+;        ssp attribute
+; Requires protector.
+define void @test1(i8* %a) sspreq {
+entry:
+; LINUX-I386-LABEL: test1:
+; LINUX-I386: mov{{l|q}} %gs:
+; LINUX-I386: mov{{l|q}} $0,
+; LINUX-I386: calll __stack_chk_fail
+
+; LINUX-X64-LABEL: test1:
+; LINUX-X64: mov{{l|q}} %fs:
+; LINUX-X64: mov{{l|q}} $0,
+; LINUX-X64: callq __stack_chk_fail
+
+; LINUX-KERNEL-X64-LABEL: test1:
+; LINUX-KERNEL-X64: mov{{l|q}} %gs:
+; LINUX-KERNEL-X64: mov{{l|q}} $0,
+; LINUX-KERNEL-X64: callq __stack_chk_fail
+
+; DARWIN-X64-LABEL: test1:
+; DARWIN-X64: mov{{l|q}} ___stack_chk_guard
+; DARWIN-X64: mov{{l|q}} $0,
+; DARWIN-X64: callq ___stack_chk_fail
+
+; OPENBSD-AMD64-LABEL: test1:
+; OPENBSD-AMD64: movq __guard_local(%rip)
+; OPENBSD-AMD64: movq $0,
+; OPENBSD-AMD64: callq __stack_smash_handler
+
+; MSVC-I386-LABEL: test1:
+; MSVC-I386: movl ___security_cookie,
+; MSVC-I386: calll @__security_check_cookie@4
+; MSVC-I386: movl $0,
+
+; MINGW-X64-LABEL: test1:
+; MINGW-X64: mov{{l|q}} .refptr.__stack_chk_guard
+; MINGW-X64: mov{{l|q}} $0,
+; MINGW-X64: callq __stack_chk_fail
+
+  %a.addr = alloca i8*, align 8
+  %buf = alloca [16 x i8], align 16
+  store i8* %a, i8** %a.addr, align 8
+  %arraydecay = getelementptr inbounds [16 x i8], [16 x i8]* %buf, i32 0, i32 0
+  %0 = load i8*, i8** %a.addr, align 8
+  %call = call i8* @strcpy(i8* %arraydecay, i8* %0)
+  %arraydecay1 = getelementptr inbounds [16 x i8], [16 x i8]* %buf, i32 0, i32 0
+  %call2 = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* %arraydecay1)
+  ret void
+}
+
+define void @__stack_chk_fail() #1 !dbg !6 {
+entry:
+  ret void
+}
+
+define i32 @IgnoreIntrinsicTest() #1 {
+; IGNORE_INTRIN: IgnoreIntrinsicTest:
+  %1 = alloca i32, align 4
+  %2 = bitcast i32* %1 to i8*
+  call void @llvm.lifetime.start.p0i8(i64 4, i8* nonnull %2)
+  store volatile i32 1, i32* %1, align 4
+  %3 = load volatile i32, i32* %1, align 4
+  %4 = mul nsw i32 %3, 42
+  call void @llvm.lifetime.end.p0i8(i64 4, i8* nonnull %2)
+  ret i32 %4
+; IGNORE_INTRIN-NOT: callq __stack_chk_fail
+; IGNORE_INTRIN:     .cfi_endproc
+}
+
+declare i8* @strcpy(i8*, i8*)
+declare i32 @printf(i8*, ...)
+declare void @llvm.lifetime.start.p0i8(i64, i8* nocapture)
+declare void @llvm.lifetime.end.p0i8(i64, i8* nocapture)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+!llvm.ident = !{!5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, emissionKind: FullDebug)
+!1 = !DIFile(filename: "test.c", directory: "/tmp")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{!"clang version x.y.z"}
+!6 = distinct !DISubprogram(name: "__stack_chk_fail", scope: !1, type: !7, unit: !0)
+!7 = !DISubroutineType(types: !2)