[unwinder] Add bounds checking to the unwinder API

It's possible that we encounter incorrect SP or FP values while
unwinding the stack. One reason is that third-party code like virus
protection may change the stack. If we encounter values for SP or FP
that don't make sense, we should bail out of unwinding and return false.

Bug: v8:8116, chromium:909957
Change-Id: I630fef3f619382c7035be50b86072be349ed185c
Reviewed-on: https://chromium-review.googlesource.com/c/1358514Reviewed-by: Yang Guo <yangguo@chromium.org>
Commit-Queue: Peter Marshall <petermarshall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58018}

include/v8.h

@@ -9270,7 +9270,11 @@ class V8_EXPORT Unwinder {
    * \param unwind_state Input state for the Isolate that the stack comes from.
    * \param register_state The current registers. This is an in-out param that
    * will be overwritten with the register values after unwinding, on success.
-   * \param stack_base Currently unused.
+   * \param stack_base The resulting stack pointer and frame pointer values are
+   * bounds-checked against the stack_base and the original stack pointer value
+   * to ensure that they are valid locations in the given stack. If these values
+   * or any intermediate frame pointer values used during unwinding are ever out
+   * of these bounds, unwinding will fail.
    *
    * \return True on success.
    */

src/unwinder.cc

@@ -49,26 +49,41 @@ void* GetCallerSPFromFP(void* fp) {
                                  i::CommonFrameConstants::kCallerSPOffset);
 }
 
+bool AddressIsInStack(const void* address, const void* stack_base,
+                      const void* stack_top) {
+  return address <= stack_base && address >= stack_top;
+}
+
 }  // namespace
 
 bool Unwinder::TryUnwindV8Frames(const UnwindState& unwind_state,
                                  RegisterState* register_state,
                                  const void* stack_base) {
+  const void* stack_top = register_state->sp;
+
   void* pc = register_state->pc;
   if (PCIsInV8(unwind_state, pc) &&
       !IsInUnsafeJSEntryRange(unwind_state.js_entry_stub, pc)) {
     void* current_fp = register_state->fp;
+    if (!AddressIsInStack(current_fp, stack_base, stack_top)) return false;
 
     // Peek at the return address that the caller pushed. If it's in V8, then we
     // assume the caller frame is a JS frame and continue to unwind.
     void* next_pc = GetReturnAddressFromFP(current_fp);
     while (PCIsInV8(unwind_state, next_pc)) {
       current_fp = GetCallerFPFromFP(current_fp);
+      if (!AddressIsInStack(current_fp, stack_base, stack_top)) return false;
       next_pc = GetReturnAddressFromFP(current_fp);
     }
 
-    register_state->sp = GetCallerSPFromFP(current_fp);
-    register_state->fp = GetCallerFPFromFP(current_fp);
+    void* final_sp = GetCallerSPFromFP(current_fp);
+    if (!AddressIsInStack(final_sp, stack_base, stack_top)) return false;
+    register_state->sp = final_sp;
+
+    void* final_fp = GetCallerFPFromFP(current_fp);
+    if (!AddressIsInStack(final_fp, stack_base, stack_top)) return false;
+    register_state->fp = final_fp;
+
     register_state->pc = next_pc;
     return true;
   }

test/cctest/test-unwinder.cc

@@ -347,6 +347,88 @@ TEST(Unwind_JSEntryStub_Fail) {
   CHECK_EQ(start + 10, register_state.pc);
 }
 
+TEST(Unwind_StackBounds_Basic) {
+  LocalContext env;
+  v8::Isolate* isolate = env->GetIsolate();
+
+  UnwindState unwind_state = isolate->GetUnwindState();
+  RegisterState register_state;
+
+  const size_t code_length = 10;
+  uintptr_t code[code_length] = {0};
+  unwind_state.code_range.start = code;
+  unwind_state.code_range.length_in_bytes = code_length * sizeof(uintptr_t);
+
+  uintptr_t stack[3];
+  stack[0] = reinterpret_cast<uintptr_t>(stack + 2);  // saved FP (rbp).
+  stack[1] = 202;  // Return address into C++ code.
+  stack[2] = 303;  // The SP points here in the caller's frame.
+
+  register_state.sp = stack;
+  register_state.fp = stack;
+  register_state.pc = code;
+
+  void* wrong_stack_base = reinterpret_cast<void*>(
+      reinterpret_cast<uintptr_t>(stack) - sizeof(uintptr_t));
+  bool unwound = v8::Unwinder::TryUnwindV8Frames(unwind_state, &register_state,
+                                                 wrong_stack_base);
+  CHECK(!unwound);
+
+  // Correct the stack base and unwinding should succeed.
+  void* correct_stack_base = stack + arraysize(stack);
+  unwound = v8::Unwinder::TryUnwindV8Frames(unwind_state, &register_state,
+                                            correct_stack_base);
+  CHECK(unwound);
+}
+
+TEST(Unwind_StackBounds_WithUnwinding) {
+  LocalContext env;
+  v8::Isolate* isolate = env->GetIsolate();
+
+  UnwindState unwind_state = isolate->GetUnwindState();
+  RegisterState register_state;
+
+  // Use a fake code range so that we can initialize it to 0s.
+  const size_t code_length = 40;
+  uintptr_t code[code_length] = {0};
+  unwind_state.code_range.start = code;
+  unwind_state.code_range.length_in_bytes = code_length * sizeof(uintptr_t);
+
+  // Our fake stack has two frames - one C++ frame and one JS frame (on top).
+  // The stack grows from high addresses to low addresses.
+  uintptr_t stack[11];
+  void* stack_base = stack + arraysize(stack);
+  stack[0] = 101;
+  stack[1] = 111;
+  stack[2] = 121;
+  stack[3] = 131;
+  stack[4] = 141;
+  stack[5] = reinterpret_cast<uintptr_t>(stack + 9);  // saved FP (rbp).
+  stack[6] = reinterpret_cast<uintptr_t>(code + 20);  // JS code.
+  stack[7] = 303;  // The SP points here in the caller's frame.
+  stack[8] = 404;
+  stack[9] = reinterpret_cast<uintptr_t>(stack) +
+             (12 * sizeof(uintptr_t));                 // saved FP (OOB).
+  stack[10] = reinterpret_cast<uintptr_t>(code + 20);  // JS code.
+
+  register_state.sp = stack;
+  register_state.fp = stack + 5;
+
+  // Put the current PC inside of the code range so it looks valid.
+  register_state.pc = code + 30;
+
+  // Unwind will fail because stack[9] FP points outside of the stack.
+  bool unwound = v8::Unwinder::TryUnwindV8Frames(unwind_state, &register_state,
+                                                 stack_base);
+  CHECK(!unwound);
+
+  // Change the return address so that it is not in range.
+  stack[10] = 202;
+  unwound = v8::Unwinder::TryUnwindV8Frames(unwind_state, &register_state,
+                                            stack_base);
+  CHECK(!unwound);
+}
+
 TEST(PCIsInV8_BadState_Fail) {
   UnwindState unwind_state;
   void* pc = nullptr;