ARM validator: fix sandbox escape with SP update at the end of the last bundle

src/trusted/validator_arm/validator_small_tests.cc

 /*
  * Copyright (c) 2012 The Native Client Authors. All rights reserved.
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #ifndef NACL_TRUSTED_BUT_NOT_TCB
 #error This file is not meant for use in the TCB
 #endif
 
 /*
  * Small unit tests for the ARM validator
  *
  * Also see validator_large_tests.cc,
  * and validator_tests.h for the testing infrastructure.
  */
 
 #include "native_client/src/trusted/validator_arm/validator_tests.h"
 
 using nacl_val_arm_test::ProblemRecord;
 using nacl_val_arm_test::ProblemSpy;
 using nacl_val_arm_test::ValidatorTests;
 using nacl_val_arm_test::kDefaultBaseAddr;
 using nacl_val_arm_test::kCodeRegionSize;
 using nacl_val_arm_test::kDataRegionSize;
 using nacl_val_arm_test::kAbiReadOnlyRegisters;
 using nacl_val_arm_test::kAbiDataAddrRegisters;
 using nacl_val_arm_test::arm_inst;
+using nacl_arm_dec::kNop;
 using nacl_arm_dec::Instruction;
 using nacl_arm_dec::ClassDecoder;
 
 namespace {
 
 // Holds instruction and message to print if there is an issue when it
 // is tested.
 struct AnnotatedInstruction {
   arm_inst inst;
   const char *about;
@@ skipping 50 matching lines @@
       Instruction test(ChangeInstCond(test_inst, cond));
       const ClassDecoder& decoder = tester->decode(test);
       Instruction sentinel(decoder.dynamic_code_replacement_sentinel(test));
       EXPECT_TRUE(test.Equals(sentinel)) <<
           std::hex << test.Bits() << "->" <<
           std::hex << sentinel.Bits() << ": " << insts[i].about;
     }
   }
 }
 
+TEST_F(ValidatorTests, NopBundle) {
+  vector<arm_inst> code(_validator.InstructionsPerBundle(), kNop);
+  validation_should_pass(&code[0], code.size(), kDefaultBaseAddr,
+                         "NOP bundle");
+}
+
 /*
  * Primitive tests checking various constructor properties.  Any of these
  * failing would be a very bad sign indeed.
  */
 
 TEST_F(ValidatorTests, RecognizesDataAddressRegisters) {
   // Note that the logic below needs to be kept in sync with the definition
   // of kAbiDataAddrRegisters at the top of this file.
   //
   // This test is pretty trivial -- we can exercise the data_address_register
@@ skipping 408 matching lines @@
     EXPECT_EQ(nacl_arm_val::kProblemUnsafe, first.problem())
         << "Instruction must be marked unsafe: "
         << undefined_insts[i].about;
   }
 }
 
 TEST_F(ValidatorTests, PcRelativeFirstInst) {
   // Note: This tests the fix for issue 2771.
   static const arm_inst pcrel_boundary_tests[] = {
     0xe59f0000,  // ldr     r0, [pc, #0]
-    0xe320f000,  // nop     {0}
-    0xe320f000,  // nop     {0}
-    0xe320f000,  // nop     {0}"
+    kNop,
+    kNop,
+    kNop,
   };
   validation_should_pass(pcrel_boundary_tests,
                          NACL_ARRAY_SIZE(pcrel_boundary_tests),
                          kDefaultBaseAddr,
                          "pc relative first instruction in first bundle");
 }
 
 TEST_F(ValidatorTests, PcRelativeFirst2ndBundle) {
   // Note: This tests the fix for issue 2771.
   static const arm_inst pcrel_boundary_tests[] = {
-    0xe320f000,  // nop     {0}
-    0xe320f000,  // nop     {0}
-    0xe320f000,  // nop     {0}
-    0xe320f000,  // nop     {0}
+    kNop,
+    kNop,
+    kNop,
+    kNop,
     0xe59f0000,  // ldr     r0, [pc, #0]
   };
   validation_should_pass(pcrel_boundary_tests,
                          NACL_ARRAY_SIZE(pcrel_boundary_tests),
                          kDefaultBaseAddr,
                          "pc relative first instruction in 2nd bundle");
 }
 
 TEST_F(ValidatorTests, SafeConditionalBicLdrTest) {
   // Test if we fixed bug with conditional Bic Loads (issue 2769).
@@ skipping 368 matching lines @@
                                    "disallow.");
           }
         }
       }
     }
   }
 }
 
 // TODO(karl): Add pattern rules and test cases for using bfc to update SP.
 
+TEST_F(ValidatorTests, UnmaskedSpUpdate) {
+  vector<arm_inst> code(_validator.InstructionsPerBundle(), kNop);
+  for (vector<arm_inst>::size_type i = 0; i < code.size(); ++i) {
+    std::fill(code.begin(), code.end(), kNop);
+    code[i] = 0xE1A0D000;  // MOV SP, R0
+    validation_should_fail(&code[0], code.size(), kDefaultBaseAddr,
+                           "unmasked SP update");
+  }
+}
+
+TEST_F(ValidatorTests, MaskedSpUpdate) {
+  vector<arm_inst> code(_validator.InstructionsPerBundle() * 2, kNop);
+  for (vector<arm_inst>::size_type i = 0; i < code.size() - 1; ++i) {
+    std::fill(code.begin(), code.end(), kNop);
+    code[i] = 0xE1A0D000;      // MOV SP, R0
+    code[i + 1] = 0xE3CDD2FF;  // BIC SP, SP, #0xf000000f

[Mark Seaborn, 2012/11/13 00:30:18]

Why not 0xc0000000 for the mask?  Bear in mind
https://code.google.com/p/nativeclient/issues/detail?id=1137: I think we should
limit which masks we accept.

[JF, 2012/11/13 00:49:42]

There's a bunch of other code below that does the same, I'd rather fix this
issue in one CL instead of diverging for this one case: it'll make the fix
easier and prevent us from forgetting this one instance if the fix isn't what we
decide to do today.

+    if (i == _validator.InstructionsPerBundle() - 1) {
+      validation_should_fail(&code[0], code.size(), kDefaultBaseAddr,

[Mark Seaborn, 2012/11/13 00:30:18]

Can you comment here that this is not actually unsafe -- "bic sp, sp,
#0xc0000000" is safe on its own -- but rejected so that superinstructions are
generally handled consistently?

[JF, 2012/11/13 00:49:42]

Done.

+                             "masked SP update straddling a bundle boundary");
+    } else {
+      validation_should_pass(&code[0], code.size(), kDefaultBaseAddr,
+                             "masked SP update");
+    }
+  }
+}
+
 TEST_F(ValidatorTests, AddConstToSpTest) {
   // Show that we can add a constant to the stack pointer is fine,
   // so long as we follow it with a mask instruction.
   static const arm_inst sp_inst[] = {
     0xe28dd00c,  // add sp, sp, #12
     0xe3cdd2ff,  // bic     sp, sp, #-268435441     ; 0xf000000f
   };
   validation_should_pass(sp_inst,
                          NACL_ARRAY_SIZE(sp_inst),
                          kDefaultBaseAddr,
@@ skipping 209 matching lines @@
   // when pointing at the head.
   // Note that we don't actually put anything malicious in the literal pool,
   // and we still shouldn't be able to jump in the middle of it.
   const size_t bundle_pos = _validator.InstructionsPerBundle();
   vector<arm_inst> code(3 * bundle_pos);
   for (size_t b_pos = 0; b_pos < code.size(); ++b_pos) {
     if ((bundle_pos <= b_pos) && (b_pos < bundle_pos * 2)) {
       // Don't try putting the branch in the middle of the literal pool.
       continue;
     }
-    std::fill(code.begin(), code.end(), 0xE320F000);  // NOP
+    std::fill(code.begin(), code.end(), kNop);
     code[bundle_pos] = nacl_arm_dec::kLiteralPoolHead;
     for (size_t b_target = 0; b_target < code.size(); ++b_target) {
       // PC reads as current instruction address plus 8 (e.g. two instructions
       // ahead of b_pos).
       // imm24 is encoded with the bottom two bits zeroed out, which we
       // implicitly do by working with instructions instead of bytes.
       uint32_t imm24 = (b_target - b_pos - 2) & 0x00FFFFFF;
       code[b_pos] = 0xEA000000 | imm24;  // B #imm
       bool target_in_pool = (bundle_pos < b_target) &&
           (b_target < bundle_pos * 2);  // Excluding head.
@@ skipping 120 matching lines @@
   }
 }
 
 };  // anonymous namespace
 
 // Test driver function.
 int main(int argc, char *argv[]) {
   testing::InitGoogleTest(&argc, argv);
   return RUN_ALL_TESTS();
 }