Copy arguments passed by value into explicit allocas for ASan

[morehouse]

Summary

ASan determines the stack layout from alloca instructions. Since
arguments marked as "byval" do not have an explicit alloca instruction, ASan
does not produce red zones for them. This commit produces an explicit alloca
instruction and copies the byval argument into the allocated memory so that red
zones are produced.

[eugenis]

lib/Transforms/Instrumentation/AddressSanitizer.cpp

2543

Not sure if it matters here, but this is usually done with BasicBlock::getFirstInsertionPt()

2545

Local variables start with a capital letter.

2553

Better call it something like arg.getName() + ".byval", or whatever looks good in ASan error report. Please also add compiler-rt test for ASan detecting overflow of a byval arg.

2557

F.getParent()

test/Instrumentation/AddressSanitizer/stack-poisoning-byval-args.ll

16

Please make this test a little stronger: check that bar() gets a pointer in the combined alloca, not the original %a. Check that memcpy is from %a. Also the size of alloca (96) is irrelevant and checking it can only make the test more brittle.

[morehouse]

Integrate eugenis' feedback

[morehouse]

Can I add compiler-rt tests to this revision or do I need to open a separate one for that?

[morehouse]

Remove ArgNum variable.

[eugenis]

In D34789#795337, @morehouse wrote:

Can I add compiler-rt tests to this revision or do I need to open a separate one for that?

You can if you use git-monorepo or plain svn. AFAIK it can not be done with multirepo-git. Anyway, could be easier to start a separate revision.

[eugenis]

lib/Transforms/Instrumentation/AddressSanitizer.cpp

2544

You can avoid the cast by using this constructor: http://llvm-cs.pcc.me.uk/include/llvm/IR/IRBuilder.h#703

IRBuilder(BB, BB.getFirstInsertionPt());

2551

Check what happens if the arg has no name. AFAIK it can be referenced as %0 (%1, etc) in the function body in that case, but getName() would return nullptr.

2557

Check what happens when byval argument has not explicit align attribute.
AFAIK, it must be handled this way:

unsigned ArgAlign = FArg.getParamAlignment();
if (ArgAlign == 0) {
  Type *EltType = A->getType()->getPointerElementType();
  ArgAlign = DL.getABITypeAlignment(EltType);
}

[vitalybuka]

lib/Transforms/Instrumentation/AddressSanitizer.cpp

2541

Should we poison original location?

[eugenis]

lib/Transforms/Instrumentation/AddressSanitizer.cpp

2541

I'm thinking not, because it is not adjacent to any alloca - it's just a random location on the stack at this point.

[vitalybuka]

lib/Transforms/Instrumentation/AddressSanitizer.cpp

2541

But access to that location is a sign of a bug

[eugenis]

Right, but then why don't we add 5 new allocas in each function, poison them and never use them. Access to those would also be a bug.

[morehouse]

Addressed eugenis' new comments.

[eugenis]

lib/Transforms/Instrumentation/AddressSanitizer.cpp

2550

It looks like single-line if() usually do not use braces, at least in this source file.

2554

Arg.hasName

2555

Use Arg.getArgNo()

test/Instrumentation/AddressSanitizer/stack-poisoning-byval-args.ll

9

Please add tests for unnamed arg and for arg w/o align attribute.

17

Check alignment here and in the alloca instruction

[vitalybuka]

In D34789#795718, @eugenis wrote:

Right, but then why don't we add 5 new allocas in each function, poison them and never use them. Access to those would also be a bug.

This is not the same. We already have original space allocate on the stack, and we know that this should not be accessed.
We can investigate usefulness of this later.

[eugenis]

In D34789#795883, @vitalybuka wrote:

In D34789#795718, @eugenis wrote:

Right, but then why don't we add 5 new allocas in each function, poison them and never use them. Access to those would also be a bug.

This is not the same. We already have original space allocate on the stack, and we know that this should not be accessed.
We can investigate usefulness of this later.

You are right. Still, this is unlikely to ever catch anything, and it increases code size and adds some complexity to the instrumentation pass. I don't mind either way.

[vitalybuka]

lib/Transforms/Instrumentation/AddressSanitizer.cpp

194

Probably we need compile time flag disabled by default

[eugenis]

lib/Transforms/Instrumentation/AddressSanitizer.cpp

194

A flag would not hurt, but I think it should be on by default.

[morehouse]

Remove ArgNum counter. Add compile-time flag to disable redzones for byval arguments. Check memcpy alignment in test case. Add test case for unnamed argument with no explicit alignment.

[morehouse]

@eugenis You mentioned the test case should also check alignment in the alloca, but it looks like the alignment there is specified by the default "-asan-realign-stack" option independent of the argument alignment.

[eugenis]

In D34789#800988, @morehouse wrote:

@eugenis You mentioned the test case should also check alignment in the alloca, but it looks like the alignment there is specified by the default "-asan-realign-stack" option independent of the argument alignment.

Right, but what if the byval argument is aligned higher than ClRealignStack.

[eugenis]

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

2557 ↗

(On Diff #105470)

This looks suspicious. You construct a temporary StringRef to a temporary std::string. The const reference extends the lifetime of StringRef, but I don't think it does anything to the std::string it points to, and StringRef does not own the buffer.

It's better to build the alloca name in a local std::string variable.

llvm/test/Instrumentation/AddressSanitizer/stack-poisoning-byval-args.ll

28 ↗

(On Diff #105470)

I think you need "target datalayout" and "target triple" statements in the test, because type alignment is target-dependent.

[morehouse]

Replace StringRef usage with std::string. Add target datalayout and target triple lines to test. Add alignment check in alloca.

[eugenis]

I've reverted this and the other change in r307345, see the commit message for the (incomplete) list of failing bots.

[morehouse]

How can I reproduce the failures?

[morehouse]

Replace std::to_string with llvm::to_string for Android builds.

[morehouse]

@vitalybuka If this looks good, could you land this today since I don't yet have commit access?

[vitalybuka]

I usually reopen reviews for reverted patches.

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

14 ↗

(On Diff #106530)

I suspect not all new headers are rely needed

[vitalybuka]

Please ignore comments about headers.

[vitalybuka]

https://reviews.llvm.org/rL308387

lib/Transforms/Instrumentation/AddressSanitizer.cpp

 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/DepthFirstIterator.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/Triple.h"
+#include "llvm/ADT/Twine.h"
 #include "llvm/Analysis/MemoryBuiltins.h"
 #include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/Analysis/ValueTracking.h"
+#include "llvm/IR/Argument.h"
 #include "llvm/IR/CallSite.h"
 #include "llvm/IR/DIBuilder.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/Dominators.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/InlineAsm.h"
 #include "llvm/IR/InstVisitor.h"

 // This flag may need to be replaced with -f[no]asan-stack.
 static cl::opt<bool> ClStack("asan-stack", cl::desc("Handle stack memory"),
                              cl::Hidden, cl::init(true));
 static cl::opt<uint32_t> ClMaxInlinePoisoningSize(
     "asan-max-inline-poisoning-size",
     cl::desc(
         "Inline shadow poisoning for blocks up to the given size in bytes."),
     cl::Hidden, cl::init(64));
 static cl::opt<bool> ClUseAfterReturn("asan-use-after-return",
                                       cl::desc("Check stack-use-after-return"),
                                       cl::Hidden, cl::init(true));
 static cl::opt<bool> ClUseAfterScope("asan-use-after-scope",
                                      cl::desc("Check stack-use-after-scope"),
                                      cl::Hidden, cl::init(false));
 // This flag may need to be replaced with -f[no]asan-globals.
 static cl::opt<bool> ClGlobals("asan-globals",
                                cl::desc("Handle global objects"), cl::Hidden,
FunctionStackPoisoner(Function &F, AddressSanitizer &ASan)
         IntptrTy(ASan.IntptrTy),
         IntptrPtrTy(PointerType::get(IntptrTy, 0)),
         Mapping(ASan.Mapping),
         StackAlignment(1 << Mapping.Scale),
         EmptyInlineAsm(CallInst::Create(ASan.EmptyAsm)) {}
 
   bool runOnFunction() {
     if (!ClStack) return false;
+
+    copyArgsPassedByValToAllocas();
+
     // Collect alloca, ret, lifetime instructions etc.
     for (BasicBlock *BB : depth_first(&F.getEntryBlock())) visit(*BB);
 
     if (AllocaVec.empty() && DynamicAllocaVec.empty()) return false;
 
     initializeCallbacks(*F.getParent());
 
     processDynamicAllocas();
     processStaticAllocas();
 
     if (ClDebugStack) {
       DEBUG(dbgs() << F);
     }
     return true;
   }
 
+  // Arguments marked with the "byval" attribute are implicitly copied without
+  // using an alloca instruction.  To produce redzones for those arguments, we
+  // copy them a second time into memory allocated with an alloca instruction.
+  void copyArgsPassedByValToAllocas();
+
   // Finds all Alloca instructions and puts
   // poisoned red zones around all of them.
   // Then unpoison everything back before the function returns.
   void processStaticAllocas();
   void processDynamicAllocas();
 
   void createDynamicAllocasInitStorage();
 

 static int StackMallocSizeClass(uint64_t LocalStackSize) {
   assert(LocalStackSize <= kMaxStackMallocSize);
   uint64_t MaxSize = kMinStackMallocSize;
   for (int i = 0;; i++, MaxSize *= 2)
     if (LocalStackSize <= MaxSize) return i;
   llvm_unreachable("impossible LocalStackSize");
 }
 
+void FunctionStackPoisoner::copyArgsPassedByValToAllocas() {
+  BasicBlock &FirstBB = *F.begin();
+  Instruction *InsBefore = dyn_cast<Instruction>(FirstBB.begin());
+  IRBuilder<> IRB(InsBefore);
+  unsigned argNum = 0;
+  for (Argument &arg : F.args()) {
+    if (arg.hasByValAttr()) {
+      Type *type = arg.getType()->getPointerElementType();
+      unsigned align = arg.getParamAlignment();
+
+      AllocaInst *AI =
+          IRB.CreateAlloca(type, nullptr,
+                           Twine("Arg") + std::to_string(argNum) + "Copy");
+      AI->setAlignment(align);
+      arg.replaceAllUsesWith(AI);
+
+      const Module *m = arg.getParent()->getParent();
+      uint64_t allocSize = m->getDataLayout().getTypeAllocSize(type);
+      IRB.CreateMemCpy(AI, &arg, allocSize, align);
+    }
+    ++argNum;
+  }
+}
+
 PHINode *FunctionStackPoisoner::createPHI(IRBuilder<> &IRB, Value *Cond,
                                           Value *ValueIfTrue,
                                           Instruction *ThenTerm,
                                           Value *ValueIfFalse) {
   PHINode *PHI = IRB.CreatePHI(IntptrTy, 2);
   BasicBlock *CondBlock = cast<Instruction>(Cond)->getParent();
   PHI->addIncoming(ValueIfFalse, CondBlock);
   BasicBlock *ThenBlock = ThenTerm->getParent();

test/Instrumentation/AddressSanitizer/stack-poisoning-byval-args.ll

+; This check verifies that arguments passed by value get redzones.
+; RUN: opt < %s -asan -S | FileCheck %s
+
+%struct.A = type { [8 x i32] }
+
+declare i32 @bar(%struct.A*)
+
+define void @foo(%struct.A* byval align 8 %a) sanitize_address {
+entry:
+; CHECK-LABEL: foo
+; CHECK: call i64 @__asan_stack_malloc
+; CHECK: alloca i8, i64 96
+; CHECK: inttoptr i64 %{{[^ \t]+}} to %struct.A*
+; CHECK: call void @llvm.memcpy
+; CHECK: call i32 @bar
+; CHECK: ret void
+
+  %call = call i32 @bar(%struct.A* %a)
+  ret void
+}