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

src/trusted/validator_arm/validator_small_tests.cc

Index: src/trusted/validator_arm/validator_small_tests.cc
diff --git a/src/trusted/validator_arm/validator_small_tests.cc b/src/trusted/validator_arm/validator_small_tests.cc
index 67930361d4121e8dfcd61872359502da5363d4b2..e0b150cc8234a54810778acd22b2743195733b1c 100644
--- a/src/trusted/validator_arm/validator_small_tests.cc
+++ b/src/trusted/validator_arm/validator_small_tests.cc
@@ -26,6 +26,7 @@ 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;
 
@@ -96,6 +97,12 @@ void test_if_dynamic_code_replacement_sentinels_unchanged(
   }
 }
 
+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.
@@ -524,9 +531,9 @@ 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),
@@ -537,10 +544,10 @@ TEST_F(ValidatorTests, PcRelativeFirstInst) {
 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,
@@ -929,6 +936,32 @@ TEST_F(ValidatorTests, AlwaysDominatesTest) {
 
 // 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.
@@ -1158,7 +1191,7 @@ TEST_F(ValidatorTests, LiteralPoolBranch) {
       // 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