This is an archive of the discontinued LLVM Phabricator instance.

Copy arguments passed by value into explicit allocas for ASan
ClosedPublic

Authored by morehouse on Jun 28 2017, 4:52 PM.

Details

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.

Event Timeline

morehouse created this revision.Jun 28 2017, 4:52 PM
eugenis added inline comments.Jun 28 2017, 6:03 PM
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 updated this revision to Diff 104663.Jun 29 2017, 9:23 AM

Integrate eugenis' feedback

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

morehouse updated this revision to Diff 104666.Jun 29 2017, 9:32 AM

Remove ArgNum variable.

eugenis edited edge metadata.Jun 29 2017, 11:45 AM

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 added inline comments.Jun 29 2017, 11:58 AM
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 added inline comments.Jun 29 2017, 12:18 PM
lib/Transforms/Instrumentation/AddressSanitizer.cpp
2541

Should we poison original location?

eugenis added inline comments.Jun 29 2017, 12:24 PM
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 added inline comments.Jun 29 2017, 12:26 PM
lib/Transforms/Instrumentation/AddressSanitizer.cpp
2541

But access to that location is a sign of a bug

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 updated this revision to Diff 104732.Jun 29 2017, 1:48 PM

Addressed eugenis' new comments.

eugenis added inline comments.Jun 29 2017, 2:19 PM
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 edited edge metadata.Jun 29 2017, 2:40 PM

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.

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 added inline comments.Jun 29 2017, 8:58 PM
lib/Transforms/Instrumentation/AddressSanitizer.cpp
194

Probably we need compile time flag disabled by default

eugenis added inline comments.Jun 30 2017, 11:42 AM
lib/Transforms/Instrumentation/AddressSanitizer.cpp
194

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

morehouse updated this revision to Diff 105470.Jul 6 2017, 10:29 AM

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.

@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 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 added inline comments.Jul 6 2017, 2:23 PM
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 updated this revision to Diff 105547.Jul 6 2017, 3:06 PM

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

eugenis accepted this revision.Jul 6 2017, 3:08 PM
This revision is now accepted and ready to land.Jul 6 2017, 3:08 PM
This revision was automatically updated to reflect the committed changes.

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

How can I reproduce the failures?

morehouse updated this revision to Diff 106530.Jul 13 2017, 2:09 PM

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

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

vitalybuka reopened this revision.Jul 18 2017, 2:57 PM

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

This revision is now accepted and ready to land.Jul 18 2017, 2:57 PM

Please ignore comments about headers.