Page MenuHomePhabricator

[AArch64] Ensure no tagged memory is left in the unallocated portion of the stack
ClosedPublic

Authored by chill on Fri, Oct 4, 9:01 AM.

Details

Summary

The memtag sanitizer may fail to clear the memory tags before a function exits.
This issue was originally noticed when running the libcxx testsuite in memtag configuration. A slightly
reduced testcase is attached (it's not very small but small enough to work with):

.
In the patch, the test case
llvm/test/CodeGen/AArch64/stack-tagging-ex-1.ll is a hand-written one that has basically the same structure.
The test case llvm/test/CodeGen/AArch64/stack-tagging-ex-2.ll is another reproducer, see the comment at the top for the
equivalent C++ source.

This patch makes sure that if we tag some memory, we untag that memory before the function returns/throws via any
exit, reachable from the tag operation. For that we place the untag operation either at:
a) the lifetime end call for the alloca, if that call post-dominates the lifetime start call (where the tag operation is placed), or it (the lifetime end call) dominates all reachable exits, otherwise
b) at the reachable exits

Diff Detail

Event Timeline

chill created this revision.Fri, Oct 4, 9:01 AM
eugenis added inline comments.Fri, Oct 4, 1:47 PM
llvm/lib/Target/AArch64/AArch64StackTagging.cpp
587

Now we compute domtree and postdomtree for all functions, even at -O0, even when building without memory tagging. Is it OK?

This can get resolved by switching to the new pass manager in the backend. Do you know if there are any plans to do so?

592

If we definitely have DT, then just pass it unconditionally.

626
chill added a comment.Sun, Oct 6, 5:40 AM

Just noticed patches lack context. Will upload a new version.

Regarding (post-)dominance analysis, indeed, we shouldn't run them at -O0. I'll look into how to go about requiring them only at -Os, -Oz, -O2, and -O3.

chill updated this revision to Diff 223626.Mon, Oct 7, 10:35 AM
chill edited the summary of this revision. (Show Details)

Updated to not require dominance/post-dominance unconditionally. Removed dependency on the parent patch and will place untag
operations in front of resume, for now.

LGTM modulo the postDominates comment.

llvm/lib/Target/AArch64/AArch64StackTagging.cpp
501

I'm worried about the case when A and B in the same basic block, but in a opposite order - i.e. one lifetime ends when control enters the basic block, and a new one starts before it exits. I've never seen it happen in practice, but it seems to be valid IR.

I think you need to iterate over instructions here, same as DominatorTree::dominates does.

chill updated this revision to Diff 223821.Tue, Oct 8, 4:06 AM
chill marked an inline comment as done.
eugenis accepted this revision.Tue, Oct 8, 4:46 PM

Everything looks great, thanks!

llvm/lib/Target/AArch64/AArch64StackTagging.cpp
511

Remove this line, it's unreachable.

This revision is now accepted and ready to land.Tue, Oct 8, 4:46 PM
This revision was automatically updated to reflect the committed changes.
chill marked an inline comment as done.Wed, Oct 9, 9:33 AM