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

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

Updating the SP as the last instruction in the last bundle is currently accepted by the validator and shouldn't: SP must be followed by a mask.

Test SP update mask; add NOP constant.

BUG=https://code.google.com/p/nativeclient/issues/detail?id=3137
R=mseaborn@chromium.org,kschimpf@chromium.org
TEST=./scons run_arm_validator_small_tests

Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=10238

[Karl, 2012-11-12 21:55:44]

lgtm

[JF, 2012-11-12 23:09:03]

The failures seem to all be flaky timeouts.

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

LGTM -- though I didn't check the hex encodings.

https://codereview.chromium.org/11361222/diff/1/src/include/arm_sandbox.h
File src/include/arm_sandbox.h (right):

https://codereview.chromium.org/11361222/diff/1/src/include/arm_sandbox.h#new...
src/include/arm_sandbox.h:71: * NOP.
The comment doesn't add any information.  Maybe "No-op: mov r0, r0" (assuming
it's a mov like that?).

https://codereview.chromium.org/11361222/diff/1/src/trusted/validator_arm/mod...
File src/trusted/validator_arm/model.h (right):

https://codereview.chromium.org/11361222/diff/1/src/trusted/validator_arm/mod...
src/trusted/validator_arm/model.h:290: static const uint32_t kNop =
NACL_INSTR_ARM_NOP;
Would it make sense for the code to refer to NACL_INSTR_ARM_NOP rather than
kNop?  Why have an alias?  Having a single name could be better for greppability
of the code.

https://codereview.chromium.org/11361222/diff/1/src/trusted/validator_arm/val...
File src/trusted/validator_arm/validator_small_tests.cc (right):

https://codereview.chromium.org/11361222/diff/1/src/trusted/validator_arm/val...
src/trusted/validator_arm/validator_small_tests.cc:954: code[i + 1] =
0xE3CDD2FF;  // BIC SP, SP, #0xf000000f
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.

https://codereview.chromium.org/11361222/diff/1/src/trusted/validator_arm/val...
src/trusted/validator_arm/validator_small_tests.cc:956:
validation_should_fail(&code[0], code.size(), kDefaultBaseAddr,
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]

https://codereview.chromium.org/11361222/diff/1/src/include/arm_sandbox.h
File src/include/arm_sandbox.h (right):

https://codereview.chromium.org/11361222/diff/1/src/include/arm_sandbox.h#new...
src/include/arm_sandbox.h:71: * NOP.
Comment added.

https://codereview.chromium.org/11361222/diff/1/src/trusted/validator_arm/mod...
File src/trusted/validator_arm/model.h (right):

https://codereview.chromium.org/11361222/diff/1/src/trusted/validator_arm/mod...
src/trusted/validator_arm/model.h:290: static const uint32_t kNop =
NACL_INSTR_ARM_NOP;
It's following the pattern above of having validator names for the macros.

https://codereview.chromium.org/11361222/diff/1/src/trusted/validator_arm/val...
File src/trusted/validator_arm/validator_small_tests.cc (right):

https://codereview.chromium.org/11361222/diff/1/src/trusted/validator_arm/val...
src/trusted/validator_arm/validator_small_tests.cc:954: code[i + 1] =
0xE3CDD2FF;  // BIC SP, SP, #0xf000000f
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.

https://codereview.chromium.org/11361222/diff/1/src/trusted/validator_arm/val...
src/trusted/validator_arm/validator_small_tests.cc:956:
validation_should_fail(&code[0], code.size(), kDefaultBaseAddr,
Done.