Permalink
Browse files

Make shadow stack use exception resistant.

Tests for same.

R=chrisha@chromium.org
BUG=None
TEST=None

Review URL: http://codereview.appspot.com/4816055

git-svn-id: http://sawbuck.googlecode.com/svn/trunk@387 15e8cca8-e42c-11de-a347-f34a4f72eb7d
  • Loading branch information...
1 parent 978e89b commit 7d35cfd3f16ac048f2b30949be57aade7de798fb siggi@chromium.org committed Jul 27, 2011
@@ -561,7 +561,7 @@ void __declspec(naked) TailRecursiveFunction(int depth) {
}
}
-}
+} // namespace
TEST_F(CallTraceDllTest, EnterExitRecursive) {
ASSERT_NO_FATAL_FAILURE(
@@ -601,3 +601,119 @@ TEST_F(CallTraceDllTest, EnterExitTailRecursive) {
EXPECT_EQ(6, entered_addresses_.size());
EXPECT_EQ(6, exited_addresses_.size());
}
+
+namespace {
+
+// Count the number of entries/exits.
+int bottom_entry = 0;
+int bottom_exit = 0;
+
+// The danger with exceptions is in the shadow stack maintained by the
+// call trace DLL. On exception, some of the entries on the shadow stack
+// may become orphaned, which can cause the call trace DLL to pop the wrong
+// entry, and return to the wrong function.
+__declspec(naked) void ExceptionTestBottom(int depth, int throw_depth) {
+ __asm {
+ call CallTraceDllTest::_penter_
+
+ push ebp
+ mov ebp, esp
+ sub esp, __LOCAL_SIZE
+ push ebx
+ push esi
+ push edi
+ }
+
+ ++bottom_entry;
+
+ if (depth > 0)
+ ExceptionTestBottom(depth - 1, throw_depth);
+
+ ++bottom_exit;
+
+ // When we throw, some of the shadow stack entries are orphaned.
+ if (depth == throw_depth)
+ ::RaiseException(0xBADF00D, 0, 0, NULL);
+
+ __asm {
+ pop edi
+ pop esi
+ pop ebx
+ mov esp, ebp
+ pop ebp
+ ret
+ }
+}
+
+bool ExceptionTestRecurseRaiseAndReturn() {
+ __try {
+ ExceptionTestBottom(10, 4);
+ } __except(EXCEPTION_EXECUTE_HANDLER) {
+ return GetExceptionCode() == 0xBADF00D;
+ }
+
+ return false;
+}
+
+// Count the number of entries/exits.
+int top_entry = 0;
+int top_exit = 0;
+
+__declspec(naked) void ExceptionTestReturnAfterException(int depth) {
+ __asm {
+ call CallTraceDllTest::_penter_
+
+ push ebp
+ mov ebp, esp
+ sub esp, __LOCAL_SIZE
+ push ebx
+ push esi
+ push edi
+ }
+
+ ++top_entry;
+
+ if (depth == 0) {
+ EXPECT_TRUE(ExceptionTestRecurseRaiseAndReturn());
+ } else {
+ ExceptionTestReturnAfterException(depth - 1);
+ }
+
+ ++top_exit;
+
+ __asm {
+ pop edi
+ pop esi
+ pop ebx
+ mov esp, ebp
+ pop ebp
+ ret
+ }
+}
+
+} // namespace
+
+TEST_F(CallTraceDllTest, EnterExitReturnAfterException) {
+ top_entry = 0;
+ top_exit = 0;
+ bottom_entry = 0;
+ bottom_exit = 0;
+
+ ASSERT_NO_FATAL_FAILURE(
+ LoadAndEnableCallTraceDll(TRACE_FLAG_ENTER | TRACE_FLAG_EXIT));
+
+ ExceptionTestReturnAfterException(10);
+
+ // Disable the provider and wait for it to notice,
+ // then make sure we got all the events we expected.
+ ASSERT_HRESULT_SUCCEEDED(controller_.DisableProvider(kCallTraceProvider));
+ ASSERT_TRUE(wait_til_disabled_());
+
+ ASSERT_HRESULT_SUCCEEDED(controller_.Stop(NULL));
+
+ EXPECT_EQ(11, top_entry);
+ EXPECT_EQ(11, top_exit);
+
+ EXPECT_EQ(11, bottom_entry);
+ EXPECT_EQ(5, bottom_exit);
+}
@@ -13,6 +13,39 @@
// limitations under the License.
//
// Implementation of the CallTrace call tracing DLL.
+//
+// A note on the exit hook:
+// The exit hook is implemented by swizzling return addresses on the machine
+// stack while maintaining a per-thread shadow stack of return addresses.
+// If exit logging is requested on entry to a function, the shadow stack is
+// pushed with the current return address, and the return address on the machine
+// stack is replaced with the address of _pexit. On subsequent return to _pexit,
+// the exit event will be recorded, the shadow stack popped, and _pexit will
+// return to the address from the shadow stack.
+//
+// This simple implementation works fine in the absence of nonlocal gotos,
+// exceptions and the like. However, on such events, some portion of the machine
+// stack is discarded, which puts the shadow stack out of synchronization with
+// the machine stack. This in turn will cause a subsequent return to _pexit
+// to pop the wrong entry off the shadow stack, and a return to the wrong
+// address.
+//
+// To avoid this, we note that:
+// * On exit, the stack pointer must be strictly greater than the entry frame
+// that the shadow stack entry was created from (as the return address as well
+// as the arguments - in the case of __stdcall - have been popped off the
+// stack in preparation for the return).
+// Also, the second non-orphaned shadow stack entry's entry frame pointer must
+// be equal or greater than the stack pointer (and its return address must be
+// _pexit).
+// * An exception to the above is multiple entries with the same entry address,
+// which occur in the cases of tail call & recursion elimination.
+// * On entry, any shadow stack entry whose entry frame pointer is less than
+// the current entry frame is orphaned. Note that equal entry frame pointers
+// occur in the case of tail call & recursion elimination.
+//
+// By discarding orphaned shadow stack entries on entry and exit, we can ensure
+// that we never return on an orphaned entry.
#include "syzygy/call_trace/call_trace_main.h"
@@ -68,6 +101,9 @@ void __declspec(naked) pexit() {
// Push the function return value.
push eax
+ // Calculate the stack pointer prior to our entry.
+ lea eax, DWORD PTR[esp + 20]
+ push eax
call TracerModule::TraceExit
popfd
@@ -446,9 +482,20 @@ void TracerModule::TraceEntry(EntryFrame *entry_frame, FuncAddr function) {
// Divert function return to pexit if we're tracing function exit.
if (NULL != data && module.IsTracing(TRACE_FLAG_EXIT)) {
+ // Make sure we trim orphaned shadow stack entries before pushing
+ // a new one. On entry, any shadow stack entry whose entry frame pointer
+ // is less than the current entry frame is orphaned.
+ ReturnStack& stack = data->return_stack_;
+ while (!stack.empty() &&
+ reinterpret_cast<const byte*>(stack.back().entry_frame) <
+ reinterpret_cast<const byte*>(entry_frame)) {
+ stack.pop_back();
+ }
+
// Save the old return address.
- data->return_stack_.push_back(
- std::make_pair(entry_frame->retaddr, function));
+ ReturnStackEntry entry = { entry_frame->retaddr, function, entry_frame };
+ stack.push_back(entry);
+
// And modify the return address in our frame.
entry_frame->retaddr = reinterpret_cast<RetAddr>(pexit);
}
@@ -457,25 +504,60 @@ void TracerModule::TraceEntry(EntryFrame *entry_frame, FuncAddr function) {
::SetLastError(err);
}
-RetAddr TracerModule::TraceExit(RetValueWord retval) {
+RetAddr TracerModule::TraceExit(const void* stack_pointer,
+ RetValueWord retval) {
// Stash the last error for restoring on return.
DWORD err = ::GetLastError();
ThreadLocalData *data = module.GetThreadData();
- if (NULL == data || data->return_stack_.empty()) {
- // Ouch, someone's returning one too many times. There's no recovery
- // possible, so we bugcheck.
- CHECK(FALSE) << "Shadow stack out of whack!";
+ CHECK(NULL != data) << "Shadow stack missing in action";
+
+ // On exit, the stack pointer must be strictly greater than the entry frame
+ // that the shadow stack entry was created from. Also, the second non-orphaned
+ // shadow stack entry's entry frame pointer must be equal or greater than
+ // the stack pointer (and its return address must be _pexit).
+ // An exception to the above is multiple entries with the same entry address,
+ // which occur in the cases of tail call & recursion elimination.
+ ReturnStack& stack = data->return_stack_;
+ CHECK(!stack.empty()) << "Shadow stack out of whack!";
+ CHECK(reinterpret_cast<const byte*>(stack_pointer) >
+ reinterpret_cast<const byte*>(stack.back().entry_frame))
+ << "Invalid entry on shadow stack";
+
+ // Find the first entry (if any) that has an entry pointer greater or equal
+ // to the stack pointer. This entry is the second non-orphaned entry on the
+ // stack, or the Nth entry behind N-1 entries with identical entry_frames in
+ // case of tail call & recursion.
+ ReturnStack::reverse_iterator it(stack.rbegin());
+ ReturnStack::reverse_iterator end(stack.rend());
+ for (; it != end; ++it) {
+ if (reinterpret_cast<const byte*>(it->entry_frame) >=
+ reinterpret_cast<const byte*>(stack_pointer)) {
+ break;
+ }
+ }
+
+ // Now "it" points to the entry preceding the entry to pop, or the first of
+ // many entries with identical entry_frame pointers.
+ ReturnStack::reverse_iterator begin(stack.rbegin());
+ --it;
+ EntryFrame* entry_frame = it->entry_frame;
+ for (; it != begin; --it) {
+ if (it->entry_frame != entry_frame) {
+ // Slice the extra entries off the shadow stack.
+ stack.resize(end - it - 1);
+ break;
+ }
}
// Get the top of the stack, we don't pop it yet, because
// the fixup function needs to see our entry to fixup correctly.
- std::pair<RetAddr, FuncAddr> top(data->return_stack_.back());
+ ReturnStackEntry top = stack.back();
if (module.IsTracing(TRACE_FLAG_EXIT)) {
TraceEnterExitEventData event_data;
event_data.depth = data->return_stack_.size();
- event_data.function = top.second;
+ event_data.function = top.function_address;
event_data.retval = retval;
if (module.enable_flags() & TRACE_FLAG_STACK_TRACES) {
event_data.num_traces = ::RtlCaptureStackBackTrace(
@@ -488,13 +570,13 @@ RetAddr TracerModule::TraceExit(RetValueWord retval) {
}
// Pop the stack.
- data->return_stack_.pop_back();
+ stack.pop_back();
// Restore last error as very last thing.
::SetLastError(err);
// And return the original return address.
- return top.first;
+ return top.return_address;
}
void TracerModule::TraceBatchEnter(FuncAddr function) {
@@ -545,7 +627,7 @@ void TracerModule::FixupBackTrace(const ReturnStack& stack,
ReturnStack::const_reverse_iterator it(stack.rbegin()), end(stack.rend());
for (size_t i = 0; i < data->num_traces && it != end; ++i) {
if (pexit == data->traces[i]) {
- data->traces[i] = it->first;
+ data->traces[i] = it->return_address;
++it;
}
}
@@ -61,17 +61,19 @@ class TracerModule: public base::win::EtwTraceProvider {
static void WINAPI TraceEntry(EntryFrame *entry_frame, FuncAddr function);
// Invoked on function exit.
+ // @param stack the stack pointer prior to entering _pexit.
// @param retval the return value from the function returning, e.g. the
// contents of the eax register.
// @returns the return address this invocation should have returned to.
- static RetAddr WINAPI TraceExit(RetValueWord retval);
+ static RetAddr WINAPI TraceExit(const void* stack, RetValueWord retval);
// Overrides from ETWTraceProvider.
virtual void OnEventsEnabled();
virtual void OnEventsDisabled();
bool WaitTilEnabled();
bool WaitTilDisabled();
+
private:
void OnProcessAttach();
void OnProcessDetach();
@@ -90,7 +92,16 @@ class TracerModule: public base::win::EtwTraceProvider {
const TraceEnterExitEventData& data);
void TraceBatchEnter(FuncAddr function);
- typedef std::vector<std::pair<RetAddr, FuncAddr> > ReturnStack;
+ struct ReturnStackEntry {
+ // The original return address we replaced.
+ RetAddr return_address;
+ // The function address invoked, from which this stack entry returns.
+ FuncAddr function_address;
+ // The address of the entry frame associated with this shadow entry.
+ EntryFrame* entry_frame;
+ };
+
+ typedef std::vector<ReturnStackEntry> ReturnStack;
// The number of trace entries we log in a batch. There is a maximal
// event size which appears to be inclusive of the trace header and
@@ -112,7 +123,7 @@ class TracerModule: public base::win::EtwTraceProvider {
// Each entry in the captured data->traces[] that points to pexit
// is fixed to point to the corresponding trace in stack. This is
// necessary because when exit tracing is enabled, the return address
- // of each entered function is rewritten to penter.
+ // of each entered function is rewritten to _pexit.
static void FixupBackTrace(const ReturnStack& stack,
TraceEnterExitEventData *data);

0 comments on commit 7d35cfd

Please sign in to comment.