This is an archive of the discontinued LLVM Phabricator instance.

Differential D19112

[asan] [SystemZ] Add slop for stack address detection.
ClosedPublic

Authored by • koriakin on Apr 14 2016, 6:26 AM.

Details

Reviewers
samsonov
uweigand
eugenis
Commits
rG48f32510dacd: [asan] [SystemZ] Add slop for stack address detection.
rCRT266593: [asan] [SystemZ] Add slop for stack address detection.
rL266593: [asan] [SystemZ] Add slop for stack address detection.
Summary

On s390, siginfo reports the faulting address with page granularity -
we need to mask off the low bits of sp before comparison.

Diff Detail

Repository
rL LLVM

Event Timeline

• koriakin updated this revision to Diff 53702.Apr 14 2016, 6:26 AM
• koriakin retitled this revision from to [asan] [SystemZ] Add slop for stack address detection..
• koriakin updated this object.
• koriakin added reviewers: uweigand, samsonov, eugenis.
• koriakin set the repository for this revision to rL LLVM.
• koriakin added a project: Restricted Project.
• koriakin added a subscriber: llvm-commits.
eugenis accepted this revision.Apr 14 2016, 12:18 PM
eugenis edited edge metadata.

LGTM w/ a nit

lib/asan/asan_posix.cc
49

Do you mean that sig.addr is rounded down to the nearest page boundary?
Do you need some extra space below sp (same as +512 in the common case below)? I.e. is it possible that the true fault address is below sp and falls into a different page, and then sig.addr would be ((sig.sp & 0xFFF) - 0x1000) ?

This revision is now accepted and ready to land.Apr 14 2016, 12:18 PM
• koriakin added inline comments.Apr 14 2016, 2:06 PM
lib/asan/asan_posix.cc
49

sig.addr is rounded down to nearest page bounduary (because s390 gives you only the page index on a fault, not the full address). However, there is no redzone - accesses below sp are verboten. I'll try to think of a better wording for the comment before commiting.

• koriakin updated this revision to Diff 53896.Apr 15 2016, 8:36 AM
• koriakin edited edge metadata.

Changed the wording in the comment. Does it sound clearer now?

• koriakin updated this revision to Diff 53913.Apr 15 2016, 10:39 AM
• koriakin updated this revision to Diff 53928.Apr 15 2016, 12:07 PM
eugenis added a comment.Apr 15 2016, 12:54 PM

Yes, looks great.

Closed by commit rL266593: [asan] [SystemZ] Add slop for stack address detection. (authored by • koriakin). · Explain WhyApr 18 2016, 2:07 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lib/
asan/
7 lines

Diff 53702

lib/asan/asan_posix.cc

Show All 37 Linesvoid AsanOnDeadlySignal(int signo, void *siginfo, void *context) {
int code = (int)((siginfo_t*)siginfo)->si_code; int code = (int)((siginfo_t*)siginfo)->si_code;
// Write the first message using the bullet-proof write. // Write the first message using the bullet-proof write.
if (18 != internal_write(2, "ASAN:DEADLYSIGNAL\n", 18)) Die(); if (18 != internal_write(2, "ASAN:DEADLYSIGNAL\n", 18)) Die();
SignalContext sig = SignalContext::Create(siginfo, context); SignalContext sig = SignalContext::Create(siginfo, context);
// Access at a reasonable offset above SP, or slightly below it (to account // Access at a reasonable offset above SP, or slightly below it (to account
// for x86_64 or PowerPC redzone, ARM push of multiple registers, etc) is // for x86_64 or PowerPC redzone, ARM push of multiple registers, etc) is
// probably a stack overflow. // probably a stack overflow.
#ifdef __s390__
// For s390, allow a full page of slop - the address from siginfo only has
// page granularity.
bool IsStackAccess = sig.addr >= (sig.sp & ~0xFFF) &&
eugenisUnsubmitted
Not Done
ReplyInline Actions

Do you mean that sig.addr is rounded down to the nearest page boundary?
Do you need some extra space below sp (same as +512 in the common case below)? I.e. is it possible that the true fault address is below sp and falls into a different page, and then sig.addr would be ((sig.sp & 0xFFF) - 0x1000) ?

eugenis: Do you mean that sig.addr is rounded down to the nearest page boundary? Do you need some extra…
koriakinAuthorUnsubmitted
Not Done
ReplyInline Actions

sig.addr is rounded down to nearest page bounduary (because s390 gives you only the page index on a fault, not the full address). However, there is no redzone - accesses below sp are verboten. I'll try to think of a better wording for the comment before commiting.

koriakin: sig.addr is rounded down to nearest page bounduary (because s390 gives you only the page index…
sig.addr < sig.sp + 0xFFFF;
#else
bool IsStackAccess = sig.addr + 512 > sig.sp && sig.addr < sig.sp + 0xFFFF; bool IsStackAccess = sig.addr + 512 > sig.sp && sig.addr < sig.sp + 0xFFFF;
#endif
#if __powerpc__ #if __powerpc__
// Large stack frames can be allocated with e.g. // Large stack frames can be allocated with e.g.
// lis r0,-10000 // lis r0,-10000
// stdux r1,r1,r0 # store sp to [sp-10000] and update sp by -10000 // stdux r1,r1,r0 # store sp to [sp-10000] and update sp by -10000
// If the store faults then sp will not have been updated, so test above // If the store faults then sp will not have been updated, so test above
// will not work, becase the fault address will be more than just "slightly" // will not work, becase the fault address will be more than just "slightly"
// below sp. // below sp.
▲ Show 20 Lines • Show All 63 Lines • Show Last 20 Lines