Bug 1102209 - Remove use of CodeGen::JoinInstructions in the Linux sandboxing code. r=kang

This reorganizes SandboxAssembler to stack up the policy rules and
traverse them in reverse order to build the filter DAG from tail to head
(i.e., starting with "deny all" and prepending allow and return-errno
rules).  Thus, this code will continue to work (perhaps with minor
changes, such as to the NodePtr typedef) with future versions of the
Chromium sandbox code that don't allow mutating the filter program with
the JoinInstructions method.

security/sandbox/linux/SandboxAssembler.cpp

@@ -5,138 +5,155 @@
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "SandboxAssembler.h"
+
+#include <errno.h>
+#include <utility>
+
 #include "linux_seccomp.h"
 #include "mozilla/NullPtr.h"
-#include <errno.h>
-
-using namespace sandbox;
+#include "sandbox/linux/seccomp-bpf/codegen.h"
 
 namespace mozilla {
 
-SandboxAssembler::SandboxAssembler()
-{
-  mTail = LoadSyscall(nullptr);
-  mTailAlt = nullptr;
+class SandboxAssemblerImpl {
+  typedef sandbox::Instruction* NodePtr;
 
-  mHead = LoadArch(JmpEq(SECCOMP_ARCH, mTail, RetKill()));
-}
+  sandbox::CodeGen mCode;
+  NodePtr mHead;
+
+  NodePtr RetAllow();
+  NodePtr RetDeny(int aErrno);
+  NodePtr RetKill();
+  NodePtr LoadArch(NodePtr aNext);
+  NodePtr LoadSyscall(NodePtr aNext);
+  NodePtr LoadArgHi(int aArg, NodePtr aNext);
+  NodePtr LoadArgLo(int aArg, NodePtr aNext);
+  NodePtr JmpEq(uint32_t aValue, NodePtr aThen, NodePtr aElse);
+public:
+  SandboxAssemblerImpl();
+  void PrependCheck(int aMaybeError, int aSyscallNr);
+  void PrependCheck(int aMaybeError, int aSyscallNr, int aArgChecked,
+                    const std::vector<uint32_t>& aArgValues);
+  void Compile(std::vector<sock_filter>* aProgram, bool aPrint);
+};
 
 void
-SandboxAssembler::AppendCheck(Instruction *aCheck,
-                              Instruction *aNewTail,
-                              Instruction *aNewTailAlt)
+SandboxAssemblerImpl::Compile(std::vector<sock_filter>* aProgram, bool aPrint)
 {
-  mCode.JoinInstructions(mTail, aCheck);
-  if (mTailAlt != nullptr) {
-    mCode.JoinInstructions(mTailAlt, aCheck);
-  }
-  mTail = aNewTail;
-  mTailAlt = aNewTailAlt;
-}
+  NodePtr prog = LoadSyscall(mHead);
+  prog = LoadArch(JmpEq(SECCOMP_ARCH, prog, RetKill()));
 
-void
-SandboxAssembler::Handle(const Condition &aCond, Instruction *aResult)
-{
-  Instruction *checkArg, *noMatch;
-
-  if (!aCond.mCheckingArg) {
-    checkArg = aResult;
-    noMatch = nullptr;
-  } else {
-    const int8_t arg = aCond.mArgChecked;
-    noMatch = LoadSyscall(nullptr);
-    Instruction *checkArgLo = noMatch;
-
-    // Loop backwards, prepending checks onto the no-match base case.
-    for (size_t i = aCond.mArgValues.size(); i > 0; --i) {
-      checkArgLo = JmpEq(aCond.mArgValues[i - 1], aResult, checkArgLo);
-    }
-    checkArgLo = LoadArgLo(arg, checkArgLo);
-
-    checkArg = LoadArgHi(arg, JmpEq(0, checkArgLo, RetKill()));
-  }
-  Instruction *check = JmpEq(aCond.mSyscallNr, checkArg, nullptr);
-  AppendCheck(check, check, noMatch);
-}
-
-void
-SandboxAssembler::Finish()
-{
-  AppendCheck(RetKill(), nullptr, nullptr);
-}
-
-void
-SandboxAssembler::Compile(std::vector<struct sock_filter> *aProgram,
-                          bool aPrint)
-{
-  mCode.Compile(mHead, aProgram);
+  mCode.Compile(prog, aProgram);
   if (aPrint) {
     mCode.PrintProgram(*aProgram);
   }
 }
 
-SandboxAssembler::~SandboxAssembler()
+void
+SandboxAssemblerImpl::PrependCheck(int aMaybeError, int aSyscallNr)
 {
-  // The CodeGen destructor will clean up the Instruction graph.
+  NodePtr ret = aMaybeError ? RetDeny(aMaybeError) : RetAllow();
+  mHead = JmpEq(aSyscallNr, ret, mHead);
 }
 
-Instruction *
-SandboxAssembler::LoadArch(Instruction *aNext)
+void
+SandboxAssemblerImpl::PrependCheck(int aMaybeError, int aSyscallNr,
+                                   int aArgChecked,
+                                   const std::vector<uint32_t>& aArgValues)
+{
+  NodePtr ret = aMaybeError ? RetDeny(aMaybeError) : RetAllow();
+  NodePtr noMatch = mHead;
+  NodePtr checkArg = LoadSyscall(noMatch);
+
+  // Loop backwards, prepending checks onto the no-match base case.
+  for (auto i = aArgValues.rbegin(); i != aArgValues.rend(); ++i) {
+    checkArg = JmpEq(*i, ret, checkArg);
+  }
+  checkArg = LoadArgLo(aArgChecked, checkArg);
+  checkArg = LoadArgHi(aArgChecked, JmpEq(0, checkArg, RetKill()));
+
+  mHead = JmpEq(aSyscallNr, checkArg, noMatch);
+}
+
+SandboxAssemblerImpl::SandboxAssemblerImpl() {
+  mHead = RetKill();
+}
+
+void
+SandboxAssembler::Compile(std::vector<sock_filter>* aProgram, bool aPrint)
+{
+  SandboxAssemblerImpl impl;
+
+  for (auto i = mRuleStack.rbegin(); i != mRuleStack.rend(); ++i) {
+    if (i->second.mCheckingArg) {
+      impl.PrependCheck(i->first, i->second.mSyscallNr, i->second.mArgChecked,
+                        i->second.mArgValues);
+    } else {
+      impl.PrependCheck(i->first, i->second.mSyscallNr);
+    }
+  }
+
+  impl.Compile(aProgram, aPrint);
+}
+
+
+SandboxAssemblerImpl::NodePtr
+SandboxAssemblerImpl::LoadArch(NodePtr aNext)
 {
   return mCode.MakeInstruction(BPF_LD + BPF_W + BPF_ABS,
                                SECCOMP_ARCH_IDX,
                                aNext);
 }
 
-Instruction *
-SandboxAssembler::LoadSyscall(Instruction *aNext)
+SandboxAssemblerImpl::NodePtr
+SandboxAssemblerImpl::LoadSyscall(NodePtr aNext)
 {
   return mCode.MakeInstruction(BPF_LD + BPF_W + BPF_ABS,
                                SECCOMP_NR_IDX,
                                aNext);
 }
 
-Instruction *
-SandboxAssembler::LoadArgHi(int aArg, Instruction *aNext)
+SandboxAssemblerImpl::NodePtr
+SandboxAssemblerImpl::LoadArgHi(int aArg, NodePtr aNext)
 {
   return mCode.MakeInstruction(BPF_LD + BPF_W + BPF_ABS,
                                SECCOMP_ARG_MSB_IDX(aArg),
                                aNext);
 }
 
-Instruction *
-SandboxAssembler::LoadArgLo(int aArg, Instruction *aNext)
+SandboxAssemblerImpl::NodePtr
+SandboxAssemblerImpl::LoadArgLo(int aArg, NodePtr aNext)
 {
   return mCode.MakeInstruction(BPF_LD + BPF_W + BPF_ABS,
                                SECCOMP_ARG_LSB_IDX(aArg),
                                aNext);
 }
 
-Instruction *
-SandboxAssembler::JmpEq(uint32_t aValue, Instruction *aThen, Instruction *aElse)
+SandboxAssemblerImpl::NodePtr
+SandboxAssemblerImpl::JmpEq(uint32_t aValue, NodePtr aThen, NodePtr aElse)
 {
   return mCode.MakeInstruction(BPF_JMP + BPF_JEQ + BPF_K,
                                aValue, aThen, aElse);
 }
 
-Instruction *
-SandboxAssembler::RetAllow()
+SandboxAssemblerImpl::NodePtr
+SandboxAssemblerImpl::RetAllow()
 {
   return mCode.MakeInstruction(BPF_RET + BPF_K,
                                SECCOMP_RET_ALLOW,
                                nullptr);
 }
 
-Instruction *
-SandboxAssembler::RetDeny(int aErrno)
+SandboxAssemblerImpl::NodePtr
+SandboxAssemblerImpl::RetDeny(int aErrno)
 {
   return mCode.MakeInstruction(BPF_RET + BPF_K,
                                SECCOMP_RET_ERRNO + aErrno,
                                nullptr);
 }
 
-Instruction *
-SandboxAssembler::RetKill()
+SandboxAssemblerImpl::NodePtr
+SandboxAssemblerImpl::RetKill()
 {
   return mCode.MakeInstruction(BPF_RET + BPF_K,
                                SECCOMP_RET_TRAP,

security/sandbox/linux/SandboxAssembler.h

@@ -7,20 +7,16 @@
 #ifndef mozilla_SandboxAssembler_h
 #define mozilla_SandboxAssembler_h
 
-#include "sandbox/linux/seccomp-bpf/codegen.h"
-
 #include <vector>
+
 #include "mozilla/Assertions.h"
 
-using namespace sandbox;
+struct sock_filter;
 
 namespace mozilla {
 
 class SandboxAssembler {
 public:
-  SandboxAssembler();
-  ~SandboxAssembler();
-
   class Condition {
     friend class SandboxAssembler;
     uint32_t mSyscallNr;
@@ -49,45 +45,29 @@ public:
     {
       MOZ_ASSERT(aArgChecked < sNumArgs);
     }
+    // This isn't perf-critical, so a naive copy ctor is fine.
+    Condition(const Condition& aOther)
+    : mSyscallNr(aOther.mSyscallNr)
+    , mCheckingArg(aOther.mCheckingArg)
+    , mArgChecked(aOther.mArgChecked)
+    , mArgValues(aOther.mArgValues)
+    { }
   };
 
   // Allow syscalls matching this condition, if no earlier condition matched.
-  void Allow(const Condition &aCond) {
-    Handle(aCond, RetAllow());
+  void Allow(const Condition& aCond) {
+    mRuleStack.push_back(std::make_pair(0, aCond));
   }
   // Cause syscalls matching this condition to fail with the given error, if
   // no earlier condition matched.
-  void Deny(int aErrno, const Condition &aCond) {
-    Handle(aCond, RetDeny(aErrno));
+  void Deny(int aErrno, const Condition& aCond) {
+    MOZ_ASSERT(aErrno != 0);
+    mRuleStack.push_back(std::make_pair(aErrno, aCond));
   }
 
-  void Finish();
-  void Compile(std::vector<struct sock_filter> *aProgram,
-               bool aPrint = false);
+  void Compile(std::vector<sock_filter>* aProgram, bool aPrint = false);
 private:
-  CodeGen mCode;
-  // The entry point of the filter program.
-  Instruction *mHead;
-  // Pointer to an instruction with a null successor which needs to be filled
-  // in with the rest of the program; see CodeGen::JoinInstructions.
-  Instruction *mTail;
-  // In some cases we will have two such instructions; this, if not null, is
-  // that.  (If we have more complicated conditions in the future, this may
-  // need to be generalized into a vector<Instruction*>.)
-  Instruction *mTailAlt;
-
-  Instruction *RetAllow();
-  Instruction *RetDeny(int aErrno);
-  Instruction *RetKill();
-  Instruction *LoadArch(Instruction *aNext);
-  Instruction *LoadSyscall(Instruction *aNext);
-  Instruction *LoadArgHi(int aArg, Instruction *aNext);
-  Instruction *LoadArgLo(int aArg, Instruction *aNext);
-  Instruction *JmpEq(uint32_t aValue, Instruction *aThen, Instruction *aElse);
-  void AppendCheck(Instruction *aCheck,
-                   Instruction *aNewTail,
-                   Instruction *aNewTailAlt);
-  void Handle(const Condition &aCond, Instruction* aResult);
+  std::vector<std::pair<int, Condition>> mRuleStack;
 
   static const uint8_t sNumArgs = 6;
 };

security/sandbox/linux/SandboxFilter.cpp

@@ -471,7 +471,6 @@ SandboxFilter::SandboxFilter(const sock_fprog** aStored, SandboxType aType,
     MOZ_CRASH("Nonexistent sandbox type!");
   }
   impl->Build();
-  impl->Finish();
   impl->Compile(&filterVec, aVerbose);
   delete impl;