From ed1f3f36aeeef5e61ecf2b41363e37c53eda61a2 Mon Sep 17 00:00:00 2001 From: Paul Robinson Date: Mon, 30 Sep 2019 15:11:23 +0000 Subject: [SSP] [3/3] cmpxchg and addrspacecast instructions can now trigger stack protectors. Fixes PR42238. Add test coverage for llvm.memset, as proxy for all llvm.mem* intrinsics. There are two issues here: (1) they could be lowered to a libc call, which could be intercepted, and do Bad Stuff; (2) with a non-constant size, they could overwrite the current stack frame. The test was mostly written by Matt Arsenault in r363169, which was later reverted; I tweaked what he had and added the llvm.memset part. Differential Revision: https://reviews.llvm.org/D67845 llvm-svn: 373220 --- llvm/lib/CodeGen/StackProtector.cpp | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) (limited to 'llvm/lib/CodeGen') diff --git a/llvm/lib/CodeGen/StackProtector.cpp b/llvm/lib/CodeGen/StackProtector.cpp index 7cce1c597ff..5683d1db473 100644 --- a/llvm/lib/CodeGen/StackProtector.cpp +++ b/llvm/lib/CodeGen/StackProtector.cpp @@ -164,12 +164,19 @@ bool StackProtector::HasAddressTaken(const Instruction *AI) { if (AI == cast(I)->getValueOperand()) return true; break; + case Instruction::AtomicCmpXchg: + // cmpxchg conceptually includes both a load and store from the same + // location. So, like store, the value being stored is what matters. + if (AI == cast(I)->getNewValOperand()) + return true; + break; case Instruction::PtrToInt: if (AI == cast(I)->getOperand(0)) return true; break; case Instruction::Call: { - // Ignore intrinsics that are not calls. TODO: Use isLoweredToCall(). + // Ignore intrinsics that do not become real instructions. + // TODO: Narrow this to intrinsics that have store-like effects. const auto *CI = cast(I); if (!isa(CI) && !CI->isLifetimeStartOrEnd()) return true; @@ -180,6 +187,7 @@ bool StackProtector::HasAddressTaken(const Instruction *AI) { case Instruction::BitCast: case Instruction::GetElementPtr: case Instruction::Select: + case Instruction::AddrSpaceCast: if (HasAddressTaken(I)) return true; break; @@ -192,8 +200,19 @@ bool StackProtector::HasAddressTaken(const Instruction *AI) { return true; break; } - default: + case Instruction::Load: + case Instruction::AtomicRMW: + case Instruction::Ret: + // These instructions take an address operand, but have load-like or + // other innocuous behavior that should not trigger a stack protector. + // atomicrmw conceptually has both load and store semantics, but the + // value being stored must be integer; so if a pointer is being stored, + // we'll catch it in the PtrToInt case above. break; + default: + // Conservatively return true for any instruction that takes an address + // operand, but is not handled above. + return true; } } return false; -- cgit v1.2.3