Experimental: Bundle revalidation inside user callback

Bundle revalidation inside user callback

This is an alternative approach for doing bundle
revalidation. Instead of sending back information to the
validator to instruct revalidation, the user callback 
directly invokes ValidateChunk[ARCH]() to validate the
bundle containing current instruction. It will also rewrite
the instructions of the bundle. And then ValidateCunk[ARCH]
is called again in this callback to revalidate the
rewritten bundle.

Note that this CL is not completely cleaned up yet.

BUG=None

[ruiq, 2015-08-10 00:15:22]

ruiq@google.com changed reviewers:
+ bradnelson@chromium.org, bradnelson@google.com, gdeepti@chromium.org,
khim@chromium.org, mseaborn@chromium.org, phosek@chromium.org

[ruiq, 2015-08-10 00:26:08]

https://codereview.chromium.org/1276543006/diff/20001/src/trusted/validator_r...
File src/trusted/validator_ragel/dfa_validate_common.c (right):

https://codereview.chromium.org/1276543006/diff/20001/src/trusted/validator_r...
src/trusted/validator_ragel/dfa_validate_common.c:133: bundle_begin = (uint8_t
*) addr;
We need to compute bundle_begin this way because the validator does not enforce
the start of the chunk to be 32-byte aligned (although it validates bundle by
bundle and enforces that the size is a multiple of kBundleSize). Moreover, some
of the tests (e.g., src/trusted/validator/validation_cache_test.cc) pass in a
code chunk address which is not necessarily 32-byte aligned. Therefore, what we
get
from "begin & ~(kBundleMask)" is not necessarily the start of the bundle, and we
have to do the above calculation. Alternatively, the bundle start can be passed
to the user callback by the validator. However, in that case the user callback
interface needs to be changed to add an additional parameter, because
"callback_data" is opaque to validator and cannot be used for this purpose.

[khimg, 2015-08-10 00:51:43]

khim@google.com changed reviewers:
+ khim@google.com

[khimg, 2015-08-10 00:51:43]

I don't think it's good idea to use NaClRewriteUnsupportedInstruction twice. It
just looks strnge and makes logic somewhat unclear.

IMO it'll be simpler and cleaner to intorudce simple helper function which will
just skip DIRECT_JUMP_OUT_OF_RANGE errors and return FALSE for everything else
(CPUID-unsupported instructions must be already replaced with NOPs at this point
and non-temporal instructions should also be processed).

Perhaps it'll be few lines longer, but much simpler: first we rewrite the code,
then we check that everything is correct after rewrite.

https://codereview.chromium.org/1276543006/diff/20001/src/trusted/validator_r...
File src/trusted/validator_ragel/dfa_validate_common.c (right):

https://codereview.chromium.org/1276543006/diff/20001/src/trusted/validator_r...
src/trusted/validator_ragel/dfa_validate_common.c:49: if ((info &
VALIDATION_ERRORS_MASK) == CPUID_UNSUPPORTED_INSTRUCTION) {
Instead of VALIDATION_ERRORS_MASK we must use (VALIDATION_ERRORS_MASK &
~DIRECT_JUMP_OUT_OF_RANGE) - although some symbolic name for it will be good.

Otherwise jumps to unaligned addresses will erroneously trigger error here.
High-level validator will check these addresses for us.

https://codereview.chromium.org/1276543006/diff/20001/src/trusted/validator_r...
src/trusted/validator_ragel/dfa_validate_common.c:88: ptr[end - begin - 1] =
0x90;
Please insert NOP before mov, not after! Think about %rip!