[hwasan] Respect returns attribute when tracking values.

[fmayer]

formatting

[fmayer]

format

[fmayer]

comments

[fmayer]

add test

[vitalybuka]

llvm/lib/Analysis/StackSafetyAnalysis.cpp

397

Isn't this in the call will make it unsafe anyway?

416

does SCEV can look through such calls?

llvm/lib/Analysis/ValueTracking.cpp

4536

There is "TEST_P(FindAllocaForValueTest, findAllocaForValue*" tests which need to be extended for this test.
Improving findAllocaForValue is a patch by itself and may affect other components, so I ask you to not mix this with StackSafetyAnalysis (if possible)

llvm/test/Instrumentation/HWAddressSanitizer/stack-safety-analysis.ll

159

Having that retptr has no body here, SAFETY should make it "call to retptr" unsafe anyway

[fmayer]

split change

[fmayer]

llvm/lib/Analysis/StackSafetyAnalysis.cpp

397

This is for being able to prove accesses safe, not the whole alloca.

416

Yes, it does.

llvm/test/Instrumentation/HWAddressSanitizer/stack-safety-analysis.ll

159

Yes, but it can tell that the store is safe.

llvm/lib/Analysis/StackSafetyAnalysis.cpp

for (const Use &UI : V->uses()) {
                    UI, Ptr, DL.getTypeStoreSize(I->getOperand(0)->getType())));
         break;
       }
 
       case Instruction::Ret:
         // Information leak.
         // FIXME: Process parameters correctly. This is a leak only if we return
         // alloca.
         US.addRange(I, UnknownRange);
         break;
 
       case Instruction::Call:
       case Instruction::Invoke: {
         if (I->isLifetimeStartOrEnd())
           break;
 
         if (AI && !SL.isAliveAfter(AI, I)) {
           US.addRange(I, UnknownRange);
           break;
         }
 
         if (const MemIntrinsic *MI = dyn_cast<MemIntrinsic>(I)) {
           US.addRange(I, getMemIntrinsicAccessRange(MI, UI, Ptr));
           break;
         }
 
         const auto &CB = cast<CallBase>(*I);
+        if (CB.getReturnedArgOperand() == V) {
+          if (Visited.insert(I).second)
+            WorkList.push_back(cast<const Instruction>(I));
+        }
+
         if (!CB.isArgOperand(&UI)) {
           US.addRange(I, UnknownRange);
           break;
         }
 
         unsigned ArgNo = CB.getArgOperandNo(&UI);
         if (CB.isByValArgument(ArgNo)) {
           US.addRange(I, getAccessRange(

llvm/lib/Analysis/ValueTracking.cpp

if (AllocaInst *AI = dyn_cast<AllocaInst>(V)) {
         AddWork(IncValue);
     } else if (auto *SI = dyn_cast<SelectInst>(V)) {
       AddWork(SI->getTrueValue());
       AddWork(SI->getFalseValue());
     } else if (GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(V)) {
       if (OffsetZero && !GEP->hasAllZeroIndices())
         return nullptr;
       AddWork(GEP->getPointerOperand());
+    } else if (CallBase *CB = dyn_cast<CallBase>(V)) {
+      Value *Returned = CB->getReturnedArgOperand();
+      if (Returned)
+        AddWork(Returned);
+      else
+        return nullptr;
     } else {
       return nullptr;
     }
   } while (!Worklist.empty());
 
   return Result;
 }
 

llvm/test/Analysis/StackSafetyAnalysis/local.ll

 ; RUN: opt -S -passes="print<stack-safety-local>" -disable-output < %s 2>&1 | FileCheck %s --check-prefixes=CHECK,LOCAL
 ; RUN: opt -S -passes="print-stack-safety" -disable-output < %s 2>&1 | FileCheck %s --check-prefixes=CHECK,GLOBAL
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 @sink = global i8* null, align 8
 
 declare void @llvm.memset.p0i8.i32(i8* %dest, i8 %val, i32 %len, i1 %isvolatile)
 declare void @llvm.memcpy.p0i8.p0i8.i32(i8* %dest, i8* %src, i32 %len, i1 %isvolatile)
 declare void @llvm.memmove.p0i8.p0i8.i32(i8* %dest, i8* %src, i32 %len, i1 %isvolatile)
 declare void @llvm.memset.p0i8.i64(i8* %dest, i8 %val, i64 %len, i1 %isvolatile)
 
+declare i8* @retptr(i8* returned)
+
 ; Address leaked.
 define void @LeakAddress() {
 ; CHECK-LABEL: @LeakAddress dso_preemptable{{$}}
 ; CHECK-NEXT: args uses:
 ; CHECK-NEXT: allocas uses:
 ; CHECK-NEXT: x[4]: full-set{{$}}
 ; GLOBAL-NEXT: safe accesses:
 ; CHECK-EMPTY:
entry:
   %x = alloca i32, align 4
   %x1 = ptrtoint i32* %x to i64
   %x2 = add i64 %x1, 2
   %x3 = inttoptr i64 %x2 to i8*
   store i8 0, i8* %x3, align 1
   ret void
 }
 
+define void @StoreInBounds6() {
+; CHECK-LABEL: @StoreInBounds6 dso_preemptable{{$}}
+; CHECK-NEXT: args uses:
+; CHECK-NEXT: allocas uses:
+; GLOBAL-NEXT: x[4]: full-set, @retptr(arg0, [0,1)){{$}}
+; LOCAL-NEXT: x[4]: [0,1), @retptr(arg0, [0,1)){{$}}
+; GLOBAL-NEXT: safe accesses:
+; GLOBAL-NEXT: store i8 0, i8* %x2, align 1
+; CHECK-EMPTY:
+entry:
+  %x = alloca i32, align 4
+  %x1 = bitcast i32* %x to i8*
+  %x2 = call i8* @retptr(i8* %x1)
+  store i8 0, i8* %x2, align 1
+  ret void
+}
+
 define dso_local void @WriteMinMax(i8* %p) {
 ; CHECK-LABEL: @WriteMinMax{{$}}
 ; CHECK-NEXT: args uses:
 ; CHECK-NEXT: p[]: full-set
 ; CHECK-NEXT: allocas uses:
 ; GLOBAL-NEXT: safe accesses:
 ; CHECK-EMPTY:
 entry:
entry:
   %x = alloca i32, align 4
   %x1 = bitcast i32* %x to i8*
   %x2 = getelementptr i8, i8* %x1, i64 2
   %x3 = bitcast i8* %x2 to i32*
   store i32 0, i32* %x3, align 1
   ret void
 }
 
+define void @StoreOutOfBounds2() {
+; CHECK-LABEL: @StoreOutOfBounds2 dso_preemptable{{$}}
+; CHECK-NEXT: args uses:
+; CHECK-NEXT: allocas uses:
+; GLOBAL-NEXT: x[4]: full-set, @retptr(arg0, [2,3)){{$}}
+; LOCAL-NEXT: x[4]: [2,6), @retptr(arg0, [2,3)){{$}}
+; GLOBAL-NEXT: safe accesses:
+; CHECK-EMPTY:
+entry:
+  %x = alloca i32, align 4
+  %x1 = bitcast i32* %x to i8*
+  %x2 = getelementptr i8, i8* %x1, i64 2
+  %x3 = call i8* @retptr(i8* %x2)
+  %x4 = bitcast i8* %x3 to i32*
+  store i32 0, i32* %x4, align 1
+  ret void
+}
+
 ; There is no difference in load vs store handling.
 define void @LoadInBounds() {
 ; CHECK-LABEL: @LoadInBounds dso_preemptable{{$}}
 ; CHECK-NEXT: args uses:
 ; CHECK-NEXT: allocas uses:
 ; CHECK-NEXT: x[4]: [0,1){{$}}
 ; GLOBAL-NEXT: safe accesses:
 ; GLOBAL-NEXT: %v = load i8, i8* %x1, align 1

llvm/test/Instrumentation/HWAddressSanitizer/stack-safety-analysis.ll

entry:
   call void @llvm.lifetime.start.p0i8(i64 1, i8* nonnull %buf.sroa.0)
   %c = call i1 @cond()
   %ptr = select i1 %c, i8* %x, i8* %buf.sroa.0
   store volatile i8 0, i8* %ptr, align 4, !tbaa !8
   call void @llvm.lifetime.end.p0i8(i64 1, i8* nonnull %buf.sroa.0)
   ret i32 0
 }
 
+; Check whether we see through the returns attribute of functions.
+define i32 @test_retptr(i32* %a) sanitize_hwaddress {
+entry:
+  ; CHECK-LABEL: @test_retptr
+  ; NOSAFETY: call {{.*}}__hwasan_generate_tag
+  ; NOSAFETY: call {{.*}}__hwasan_store
+  ; SAFETY: call {{.*}}__hwasan_generate_tag
+  ; SAFETY-NOT: call {{.*}}__hwasan_store
+  ; NOSTACK-NOT: call {{.*}}__hwasan_generate_tag
+  ; NOSTACK-NOT: call {{.*}}__hwasan_store
+  %buf.sroa.0 = alloca i8, align 4
+  call void @llvm.lifetime.start.p0i8(i64 1, i8* nonnull %buf.sroa.0)
+  %ptr = call i8* @retptr(i8* %buf.sroa.0)
+  store volatile i8 0, i8* %ptr, align 4, !tbaa !8
+  call void @llvm.lifetime.end.p0i8(i64 1, i8* nonnull %buf.sroa.0)
+  ret i32 0
+}
+
 ; Function Attrs: argmemonly mustprogress nofree nosync nounwind willreturn
 declare void @llvm.lifetime.start.p0i8(i64 immarg, i8* nocapture)
 
 ; Function Attrs: argmemonly mustprogress nofree nosync nounwind willreturn
 declare void @llvm.lifetime.end.p0i8(i64 immarg, i8* nocapture)
 
 declare i1 @cond()
 declare void @use(i8* nocapture)
 declare i32 @getoffset()
 declare i8* @getptr(i8* nocapture)
+declare i8* @retptr(i8* returned)
 
 !8 = !{!9, !9, i64 0}
 !9 = !{!"omnipotent char", !10, i64 0}
 !10 = !{!"Simple C/C++ TBAA"}