From b2021df620824627f5a8c96615edbd1eb7fdddfc Mon Sep 17 00:00:00 2001
From: Riley Dulin <dulinr@fb.com>
Date: Thu, 1 Oct 2020 09:38:50 -0700
Subject: [PATCH] Fix CVE-2020-1914 by using NEXTINST for SaveGeneratorLong

Summary:
If `SaveGeneratorLong` was emitted, it would accidentally jump to the
wrong next instruction, based on how long SaveGenerator was.

Make a callout function to handle the common case, and handle the dispatch
within each case of the interpreter loop.

Fixes CVE-2020-1914

Reviewed By: neildhar

Differential Revision: D24024242

fbshipit-source-id: 3bcb88daa740f0d50e91771a49eb212551ce8bd8
---
 include/hermes/VM/Interpreter.h  |  8 ++++++++
 lib/VM/Interpreter-slowpaths.cpp | 11 +++++++++++
 lib/VM/Interpreter.cpp           | 33 +++++++++++++++++---------------
 test/hermes/es6/generator.js     | 15 +++++++++++++++
 4 files changed, 52 insertions(+), 15 deletions(-)

diff --git a/include/hermes/VM/Interpreter.h b/include/hermes/VM/Interpreter.h
index 83a86d6ba92..766e4381041 100644
--- a/include/hermes/VM/Interpreter.h
+++ b/include/hermes/VM/Interpreter.h
@@ -37,6 +37,14 @@ class Interpreter {
       Handle<Environment> envHandle,
       NativeArgs args);
 
+  /// Suspend the generator function and yield to the caller.
+  /// \param resumeIP Is the IP where the generator should resume from when it
+  ///   is resumed.
+  static void saveGenerator(
+      Runtime *runtime,
+      PinnedHermesValue *frameRegs,
+      const Inst *resumeIP);
+
   /// Slow path for ReifyArguments resReg, lazyReg
   /// It assumes that he fast path has handled the case when 'lazyReg' is
   /// already initialized. It creates a new 'arguments' object and populates it
diff --git a/lib/VM/Interpreter-slowpaths.cpp b/lib/VM/Interpreter-slowpaths.cpp
index 95f37da76d4..26bcd4327ea 100644
--- a/lib/VM/Interpreter-slowpaths.cpp
+++ b/lib/VM/Interpreter-slowpaths.cpp
@@ -9,6 +9,7 @@
 #include "JSLib/JSLibInternal.h"
 #include "hermes/VM/Casting.h"
 #include "hermes/VM/Interpreter.h"
+#include "hermes/VM/StackFrame-inline.h"
 #include "hermes/VM/StringPrimitive.h"
 
 #include "Interpreter-internal.h"
@@ -18,6 +19,16 @@ using namespace hermes::inst;
 namespace hermes {
 namespace vm {
 
+void Interpreter::saveGenerator(
+    Runtime *runtime,
+    PinnedHermesValue *frameRegs,
+    const Inst *resumeIP) {
+  auto *innerFn = vmcast<GeneratorInnerFunction>(FRAME.getCalleeClosure());
+  innerFn->saveStack(runtime);
+  innerFn->setNextIP(resumeIP);
+  innerFn->setState(GeneratorInnerFunction::State::SuspendedYield);
+}
+
 ExecutionStatus Interpreter::caseDirectEval(
     Runtime *runtime,
     PinnedHermesValue *frameRegs,
diff --git a/lib/VM/Interpreter.cpp b/lib/VM/Interpreter.cpp
index 7886dc7140a..31bc66c35ee 100644
--- a/lib/VM/Interpreter.cpp
+++ b/lib/VM/Interpreter.cpp
@@ -1001,6 +1001,16 @@ CallResult<HermesValue> Interpreter::interpretFunction(
 
 #endif // NDEBUG
 
+/// \def DONT_CAPTURE_IP(expr)
+/// \param expr A call expression to a function external to the interpreter. The
+///   expression should not make any allocations and the IP should be set
+///   immediately following this macro.
+#define DONT_CAPTURE_IP(expr)      \
+  do {                             \
+    NoAllocScope noAlloc(runtime); \
+    (void)expr;                    \
+  } while (false)
+
   LLVM_DEBUG(dbgs() << "interpretFunction() called\n");
 
   ScopedNativeDepthTracker depthTracker{runtime};
@@ -1798,25 +1808,18 @@ CallResult<HermesValue> Interpreter::interpretFunction(
       }
 
       CASE(SaveGenerator) {
-        nextIP = IPADD(ip->iSaveGenerator.op1);
-        goto doSaveGen;
+        DONT_CAPTURE_IP(
+            saveGenerator(runtime, frameRegs, IPADD(ip->iSaveGenerator.op1)));
+        ip = NEXTINST(SaveGenerator);
+        DISPATCH;
       }
       CASE(SaveGeneratorLong) {
-        nextIP = IPADD(ip->iSaveGeneratorLong.op1);
-        goto doSaveGen;
+        DONT_CAPTURE_IP(saveGenerator(
+            runtime, frameRegs, IPADD(ip->iSaveGeneratorLong.op1)));
+        ip = NEXTINST(SaveGeneratorLong);
+        DISPATCH;
       }
 
-    doSaveGen : {
-      auto *innerFn = vmcast<GeneratorInnerFunction>(
-          runtime->getCurrentFrame().getCalleeClosure());
-
-      innerFn->saveStack(runtime);
-      innerFn->setNextIP(nextIP);
-      innerFn->setState(GeneratorInnerFunction::State::SuspendedYield);
-      ip = NEXTINST(SaveGenerator);
-      DISPATCH;
-    }
-
       CASE(StartGenerator) {
         auto *innerFn = vmcast<GeneratorInnerFunction>(
             runtime->getCurrentFrame().getCalleeClosure());
diff --git a/test/hermes/es6/generator.js b/test/hermes/es6/generator.js
index 2cbe3c2fdba..9513d789384 100644
--- a/test/hermes/es6/generator.js
+++ b/test/hermes/es6/generator.js
@@ -354,3 +354,18 @@ print(iterator.next().value);
 iterator.return(123);
 // CHECK-NEXT: 1
 // CHECK-NEXT: get return
+
+// Make sure using SaveGeneratorLong works.
+function* saveGeneratorLong() {
+    yield* [1];
+    // Waste some registers, to change SaveGenerator to SaveGeneratorLong.
+    [].push(0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0);
+}
+print(saveGeneratorLong().next().value);
+// CHECK-NEXT: 1