Rewrite non-temporal instructions

Rewrite non-temporal instructions

For security hardening reasons, non-temporal instructions
should not present in validated NaCl applications. They are
either forbidden during validation, as in
https://codereview.chromium.org/1234393005/, or rewritten,
as in this CL.

This CL rewrites non-temporal instructions identified in
the nexes in Web Store. The result of the instruction scan
https://docs.google.com/a/google.com/document/d/1AsQavtUqv3iUAVK5mKNV-B5NHKdHXTCGmZu94mIO_jk/edit?usp=sharing

Some related discussions can be seen in this experimental
CL: https://codereview.chromium.org/1276543006/

BUG=https://code.google.com/p/chromium/issues/detail?id=500026
TEST=trybots

[ruiq, 2015-08-03 23:15:54]

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

[bradn, 2015-08-04 22:03:30]

https://codereview.chromium.org/1269113003/diff/60002/src/trusted/validator/b...
File src/trusted/validator/build.scons (right):

https://codereview.chromium.org/1269113003/diff/60002/src/trusted/validator/b...
src/trusted/validator/build.scons:70: EXTRA_LIBS=['validators', 'nrd_xfer'])
might be overkill to test cross arch, but no harm.

https://codereview.chromium.org/1269113003/diff/60002/src/trusted/validator/v...
File src/trusted/validator/validation_rewrite_32_test.cc (right):

https://codereview.chromium.org/1269113003/diff/60002/src/trusted/validator/v...
src/trusted/validator/validation_rewrite_32_test.cc:52: const char *code =
"\x0f\xe7\x03";
Could you add a .S file with the asm for each of these, but in a data region.
You'd do something like:

.data
.global DisableNonTemporalsNoRewrite_code
DisableNonTemporalsNoRewrite_code:
  movntq %mm0, (%ebx)
  .byte 0
Then use that in place of the code variable.

https://codereview.chromium.org/1269113003/diff/60002/src/trusted/validator/v...
File src/trusted/validator/validation_rewrite_64_test.cc (right):

https://codereview.chromium.org/1269113003/diff/60002/src/trusted/validator/v...
src/trusted/validator/validation_rewrite_64_test.cc:90: const char *expect_code
= "\x89\xff\x49\x89\x44\x3f\x68\x90";
You can also put the expected asm in the .S file.

https://codereview.chromium.org/1269113003/diff/60002/src/trusted/validator_r...
File src/trusted/validator_ragel/dfa_validate_64.c (right):

https://codereview.chromium.org/1269113003/diff/60002/src/trusted/validator_r...
src/trusted/validator_ragel/dfa_validate_64.c:93:
CHECK(reval_callback_data.did_rewrite == 0);
This should return Failure instead of being a check.

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

https://codereview.chromium.org/1269113003/diff/60002/src/trusted/validator_r...
src/trusted/validator_ragel/dfa_validate_common.c:56: } else if
(IsREX(begin[0])) {
Make this arch conditional (#ifdef), so you don't waste time checking for things
from the other arch?

[ruiq, 2015-08-05 20:11:03]

https://codereview.chromium.org/1269113003/diff/60002/src/trusted/validator/v...
File src/trusted/validator/validation_rewrite_32_test.cc (right):

https://codereview.chromium.org/1269113003/diff/60002/src/trusted/validator/v...
src/trusted/validator/validation_rewrite_32_test.cc:52: const char *code =
"\x0f\xe7\x03";
On 2015/08/04 22:03:30, bradn wrote:
> Could you add a .S file with the asm for each of these, but in a data region.
> You'd do something like:
> 
> .data
> .global DisableNonTemporalsNoRewrite_code
> DisableNonTemporalsNoRewrite_code:
>   movntq %mm0, (%ebx)
>   .byte 0
> Then use that in place of the code variable.

Done.

https://codereview.chromium.org/1269113003/diff/60002/src/trusted/validator_r...
File src/trusted/validator_ragel/dfa_validate_64.c (right):

https://codereview.chromium.org/1269113003/diff/60002/src/trusted/validator_r...
src/trusted/validator_ragel/dfa_validate_64.c:93:
CHECK(reval_callback_data.did_rewrite == 0);
On 2015/08/04 22:03:30, bradn wrote:
> This should return Failure instead of being a check.

Done.

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

https://codereview.chromium.org/1269113003/diff/60002/src/trusted/validator_r...
src/trusted/validator_ragel/dfa_validate_common.c:56: } else if
(IsREX(begin[0])) {
On 2015/08/04 22:03:30, bradn wrote:
> Make this arch conditional (#ifdef), so you don't waste time checking for
things
> from the other arch?

Done.

[bradn, 2015-08-05 21:45:53]

lgtm

[bradnelson, 2015-08-05 21:46:04]

lgtm

[Petr Hosek, 2015-08-05 21:51:32]

Is it really necessary to rerun the validation? That doubles the validation
time! We should be able to just restart the validation from the last
instruction.

[Petr Hosek, 2015-08-05 21:59:56]

https://codereview.chromium.org/1269113003/diff/250001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_common.c (right):

https://codereview.chromium.org/1269113003/diff/250001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:64: if (memcmp(begin + 1,
"\x0f\x2b", 2) == 0) {
Couldn't you rewrite this using a switch statement rather than using the
complicated nested if statements?

https://codereview.chromium.org/1269113003/diff/250001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:103: if
(RewriteUnsupportedInstruction(begin, end)) {
You could merge the nested if statement with the outer one to make the code
simpler, i.e.:

if (!(data->flags & NACL_DISABLE_NONTEMPORALS_X86) &&
RewriteUnsupportedInstruction(begin, end)) {
  data->did_rewrite = 1;
  return TRUE;
}
return FALSE;

In fact, you could go even further and merge the `return FALSE;` with the
outermost loop and have just a single `return FALSE;` statement.

[ruiq, 2015-08-06 00:07:12]

On 2015/08/05 21:51:32, Petr Hosek wrote:
> Is it really necessary to rerun the validation? That doubles the validation
> time! We should be able to just restart the validation from the last
> instruction.

Re-validation doubles the validation time, if the supplied code contains
non-temporal instructions. I expect the majority of webstore nexes would
not contain non-temporals. So the questions are:
1) How much performance gain can we get if we use "smart" re-validation?
Does it really matter if we validate twice occasionally?
2) How effective is that optimization? It probably needs more hooks for
the validation, does the benefit outweigh code simplicity?

Considering this is a security fix, I suppose the optimization could be 
implemented in future when collected data suggests necessity?

[ruiq, 2015-08-06 04:53:18]

https://codereview.chromium.org/1269113003/diff/250001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_common.c (right):

https://codereview.chromium.org/1269113003/diff/250001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:64: if (memcmp(begin + 1,
"\x0f\x2b", 2) == 0) {
On 2015/08/05 21:59:56, Petr Hosek wrote:
> Couldn't you rewrite this using a switch statement rather than using the
> complicated nested if statements?

Done.

https://codereview.chromium.org/1269113003/diff/250001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:103: if
(RewriteUnsupportedInstruction(begin, end)) {
On 2015/08/05 21:59:56, Petr Hosek wrote:
> You could merge the nested if statement with the outer one to make the code
> simpler, i.e.:
> 
> if (!(data->flags & NACL_DISABLE_NONTEMPORALS_X86) &&
> RewriteUnsupportedInstruction(begin, end)) {
>   data->did_rewrite = 1;
>   return TRUE;
> }
> return FALSE;
> 
> In fact, you could go even further and merge the `return FALSE;` with the
> outermost loop and have just a single `return FALSE;` statement.

Done.

[khimg, 2015-08-08 00:33:10]

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

[khimg, 2015-08-08 00:33:11]

https://codereview.chromium.org/1269113003/diff/300001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_common.c (right):

https://codereview.chromium.org/1269113003/diff/300001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:54: /* movntq => movq */
Why movntq is not rewritten in 64bit mode?

https://codereview.chromium.org/1269113003/diff/300001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:67: /* movntps => movaps */
Why movntps is not rewritten in 32bit mode?

https://codereview.chromium.org/1269113003/diff/300001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:71: /* movnti => mov */
Why movnti is not rewritten in 32bit moed?

https://codereview.chromium.org/1269113003/diff/300001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:73: memmove(ptr + 2, ptr + 3,
end - begin - 3);
You can't do that. Use the following approach:
  ptr[2] = 0x89;
  ptr[1] = ptr[0];
  ptr[0] = 0x90;

https://codereview.chromium.org/1269113003/diff/300001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:77: /* prefetchnta => nop */
Why prefetchnta is not rewritten in 32bit mode?
You can replace it with a singular nop:
  ptr[2] = 0x1F;
  ptr[3] &= 0xC7;

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

https://codereview.chromium.org/1269113003/diff/300001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_common.c (right):

https://codereview.chromium.org/1269113003/diff/300001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:54: /* movntq => movq */
On 2015/08/08 00:33:11, khimg wrote:
> Why movntq is not rewritten in 64bit mode?

As the CL description says, we only rewrite non-temporal instructions that are
found in current nexes of the webstore so that we do not break them. For future
nexes, they will fail validation if they contain (other) non-temporal
instructions.

https://codereview.chromium.org/1269113003/diff/300001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:67: /* movntps => movaps */
On 2015/08/08 00:33:11, khimg wrote:
> Why movntps is not rewritten in 32bit mode?

ditto.

https://codereview.chromium.org/1269113003/diff/300001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:71: /* movnti => mov */
On 2015/08/08 00:33:11, khimg wrote:
> Why movnti is not rewritten in 32bit moed?

ditto.

https://codereview.chromium.org/1269113003/diff/300001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:73: memmove(ptr + 2, ptr + 3,
end - begin - 3);
On 2015/08/08 00:33:11, khimg wrote:
> You can't do that. Use the following approach:
>   ptr[2] = 0x89;
>   ptr[1] = ptr[0];
>   ptr[0] = 0x90;

The nop has to be put at the end because before the non-temporal store
instruction, there can be an instruction that clears the upper 32-bits of a
register. That instruction followed with this non-temporal store will pass the
validation, but not if they are separated.

https://codereview.chromium.org/1269113003/diff/300001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:77: /* prefetchnta => nop */
On 2015/08/08 00:33:11, khimg wrote:
> Why prefetchnta is not rewritten in 32bit mode?
> You can replace it with a singular nop:
>   ptr[2] = 0x1F;
>   ptr[3] &= 0xC7;

ditto.

[Petr Hosek, 2015-08-11 18:22:36]

https://codereview.chromium.org/1269113003/diff/380001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_common.c (right):

https://codereview.chromium.org/1269113003/diff/380001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:43: /* Similar to
NaClDfaRewriteUnsupportedInstruction(), we don't consider
Nit: the comment body should only start on the second line:

/*
 * ...
 */

https://codereview.chromium.org/1269113003/diff/380001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:95: if (memcmp(begin,
"\x0f\xe7", 2) == 0) {
Since you already have a `ptr`, you could first check the prefix, i.e.:

if (*ptr == 0x66)
  ptr++;

Then you can combine both these cases into one, i.e.:

if (memcmp(ptr, "\x0f\xe7", 2) == 0) {
  ...
}

https://codereview.chromium.org/1269113003/diff/380001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:107: if (IsREX(begin[0]) &&
(begin[1] == 0x0f)) {
The same here, you can use `ptr` to simplify this code (you'll need to use the
`begin` for the actual rewriting or introduce another helper variable).

https://codereview.chromium.org/1269113003/diff/380001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:153: memcmp(begin + 2,
"\x0f\xe7", 2) == 0) {
This could be merged into the `if` case.

https://codereview.chromium.org/1269113003/diff/380001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:170: Bool rc, rc2;
You don't need two different variable for return value.

https://codereview.chromium.org/1269113003/diff/380001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:241: return TRUE;
Nit: you can just return `rc` here.

https://codereview.chromium.org/1269113003/diff/380001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_common.h (right):

https://codereview.chromium.org/1269113003/diff/380001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.h:40: NaClCPUFeaturesX86*
cpu_features;
Nit: * belongs to variable.

[ruiq, 2015-08-11 21:08:34]

https://codereview.chromium.org/1269113003/diff/380001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_common.c (right):

https://codereview.chromium.org/1269113003/diff/380001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:43: /* Similar to
NaClDfaRewriteUnsupportedInstruction(), we don't consider
On 2015/08/11 18:22:35, Petr Hosek wrote:
> Nit: the comment body should only start on the second line:
> 
> /*
>  * ...
>  */

Done.

https://codereview.chromium.org/1269113003/diff/380001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:95: if (memcmp(begin,
"\x0f\xe7", 2) == 0) {
On 2015/08/11 18:22:36, Petr Hosek wrote:
> Since you already have a `ptr`, you could first check the prefix, i.e.:
> 
> if (*ptr == 0x66)
>   ptr++;
> 
> Then you can combine both these cases into one, i.e.:
> 
> if (memcmp(ptr, "\x0f\xe7", 2) == 0) {
>   ...
> }

Separating these two cases were done on purpose. The idea was to make it more
explicit what instruction is rewritten to what (see the comments above each
case).

https://codereview.chromium.org/1269113003/diff/380001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:107: if (IsREX(begin[0]) &&
(begin[1] == 0x0f)) {
On 2015/08/11 18:22:36, Petr Hosek wrote:
> The same here, you can use `ptr` to simplify this code (you'll need to use the
> `begin` for the actual rewriting or introduce another helper variable).

Same here.

https://codereview.chromium.org/1269113003/diff/380001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:153: memcmp(begin + 2,
"\x0f\xe7", 2) == 0) {
On 2015/08/11 18:22:35, Petr Hosek wrote:
> This could be merged into the `if` case.

Same here.

https://codereview.chromium.org/1269113003/diff/380001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:170: Bool rc, rc2;
On 2015/08/11 18:22:36, Petr Hosek wrote:
> You don't need two different variable for return value.

Done.

https://codereview.chromium.org/1269113003/diff/380001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:241: return TRUE;
On 2015/08/11 18:22:36, Petr Hosek wrote:
> Nit: you can just return `rc` here.

Done.

https://codereview.chromium.org/1269113003/diff/380001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_common.h (right):

https://codereview.chromium.org/1269113003/diff/380001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.h:40: NaClCPUFeaturesX86*
cpu_features;
On 2015/08/11 18:22:36, Petr Hosek wrote:
> Nit: * belongs to variable.

Done.

[ruiq, 2015-08-12 05:37:50]

https://codereview.chromium.org/1269113003/diff/380001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_common.c (right):

https://codereview.chromium.org/1269113003/diff/380001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:153: memcmp(begin + 2,
"\x0f\xe7", 2) == 0) {
On 2015/08/11 21:08:34, ruiq wrote:
> On 2015/08/11 18:22:35, Petr Hosek wrote:
> > This could be merged into the `if` case.
> 
> Same here.

The other point is that we don't want to overly generalize the rewriting. For
example, we want to rewrite "66 REX 0f e7", but we don't want to rewrite e.g.,
"66 REX 0f 2b".

[Mark Seaborn, 2015-08-14 21:12:59]

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator/...
File src/trusted/validator/build.scons (right):

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator/...
src/trusted/validator/build.scons:67: validation_rewrite_test_exe=
gtest_env.ComponentProgram(
Nit: there should be spaces before and after "="

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator/...
File src/trusted/validator/validation_rewrite_32_test.cc (right):

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator/...
src/trusted/validator/validation_rewrite_32_test.cc:7: #include <string.h>
Nit: add empty line after this, to separate standard library #includes

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator/...
File src/trusted/validator/validation_rewrite_64_test_data.S (right):

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator/...
src/trusted/validator/validation_rewrite_64_test_data.S:6: 
Does the test have an example of a non-temporal write that the rewriter doesn't
handle?

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator/...
src/trusted/validator/validation_rewrite_64_test_data.S:9: /* This macro is used
to define two global symbols for the same byte sequence:
Nit: Use the NaCl style for multiline comments, with "/*" and "*/" on separate
lines, like this:
/*
 * Blah...
 * blah.
 */

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator/...
src/trusted/validator/validation_rewrite_64_test_data.S:10: * 'name' and
'_name'. This is to work around the linkage issue across OS and
There's already a macro for doing this, "IDENTIFIER", in src/include/nacl_asm.h.

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator/...
src/trusted/validator/validation_rewrite_64_test_data.S:22: .byte 0
What is this for?

Oh, you're using strlen() on x86 code?  That's not a good idea, because there
might be zero bytes inside the code.

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_32.c (right):

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_32.c:74: options &
PROCESS_CHUNK_AS_A_CONTIGUOUS_STREAM;
Use "(x & b) != 0".  NaCl style is don't implicitly convert to a boolean. 
*Especially* where you store the result of an "&" as an int. :-)

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_64.c (right):

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_64.c:75:
callback_data.bundle_begin_offset = (uintptr_t) data & kBundleMask;
I'm not sure I understand this parameter.  You're taking a trusted address and
ANDing it with kBundleMask?  That seems like a very hacky thing to do.

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_common.c (right):

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:50: if ((info &
VALIDATION_ERRORS_MASK) == DIRECT_JUMP_OUT_OF_RANGE)
if (X)
  return TRUE;
else
  return FALSE:

can be written more concisely as "return X".

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:62: static Bool
NaClDfaRewriteUnsupportedInstruction(const uint8_t *begin,
Could this ever be called with begin == end?  Should we have a CHECK to prevent
that?

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:63: const uint8_t *end,
Fix indentation to line up

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:96: #if NACL_BUILD_SUBARCH ==
32
I believe you can use if() rather than #if here.

if() is preferred when possible.  See:
http://lackingrhoticity.blogspot.com/2015/01/conditionalising-c-ifdef-vs-if.html

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:110: if (IsREX(begin[0]) &&
(begin[1] == 0x0f)) {
Nit: don't need ()s around "begin[1] == 0x0f"

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:115: ptr[2] = 0x29;
It's surprising that you're using "begin" to read, but "ptr" to write.  Can you
make it consistent?

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:132: ptr[end - begin - 1] =
0x90;
How about commenting this as: 0x90; /* NOP */

Same below.

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:158: memcmp(begin + 2,
"\x0f\xe7", 2) == 0) {
Fix indentation alignment

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:164: #endif
For safety, add:
#else
# error Unknown architecture

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:189:
CHECK(!data->chunk_processed_as_a_contiguous_stream);
I'm not sure I understand the purpose of this parameter if it's always supposed
to be false...

[ruiq, 2015-08-15 04:46:41]

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator/...
File src/trusted/validator/build.scons (right):

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator/...
src/trusted/validator/build.scons:67: validation_rewrite_test_exe=
gtest_env.ComponentProgram(
On 2015/08/14 21:12:59, Mark Seaborn wrote:
> Nit: there should be spaces before and after "="

Done.

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator/...
File src/trusted/validator/validation_rewrite_32_test.cc (right):

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator/...
src/trusted/validator/validation_rewrite_32_test.cc:7: #include <string.h>
On 2015/08/14 21:12:59, Mark Seaborn wrote:
> Nit: add empty line after this, to separate standard library #includes

Done.

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator/...
File src/trusted/validator/validation_rewrite_64_test_data.S (right):

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator/...
src/trusted/validator/validation_rewrite_64_test_data.S:6: 
On 2015/08/14 21:12:59, Mark Seaborn wrote:
> Does the test have an example of a non-temporal write that the rewriter
doesn't
> handle?

Done.

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator/...
src/trusted/validator/validation_rewrite_64_test_data.S:9: /* This macro is used
to define two global symbols for the same byte sequence:
On 2015/08/14 21:12:59, Mark Seaborn wrote:
> Nit: Use the NaCl style for multiline comments, with "/*" and "*/" on separate
> lines, like this:
> /*
>  * Blah...
>  * blah.
>  */

Done.

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator/...
src/trusted/validator/validation_rewrite_64_test_data.S:10: * 'name' and
'_name'. This is to work around the linkage issue across OS and
On 2015/08/14 21:12:59, Mark Seaborn wrote:
> There's already a macro for doing this, "IDENTIFIER", in
src/include/nacl_asm.h.

Done.

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator/...
src/trusted/validator/validation_rewrite_64_test_data.S:22: .byte 0
On 2015/08/14 21:12:59, Mark Seaborn wrote:
> What is this for?
> 
> Oh, you're using strlen() on x86 code?  That's not a good idea, because there
> might be zero bytes inside the code.

Now using two labels: xx_code, and xx_code_end.

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_32.c (right):

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_32.c:74: options &
PROCESS_CHUNK_AS_A_CONTIGUOUS_STREAM;
On 2015/08/14 21:12:59, Mark Seaborn wrote:
> Use "(x & b) != 0".  NaCl style is don't implicitly convert to a boolean. 
> *Especially* where you store the result of an "&" as an int. :-)

Done.

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_common.c (right):

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:50: if ((info &
VALIDATION_ERRORS_MASK) == DIRECT_JUMP_OUT_OF_RANGE)
On 2015/08/14 21:12:59, Mark Seaborn wrote:
> if (X)
>   return TRUE;
> else
>   return FALSE:
> 
> can be written more concisely as "return X".

Done.

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:63: const uint8_t *end,
On 2015/08/14 21:12:59, Mark Seaborn wrote:
> Fix indentation to line up

Done.

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:110: if (IsREX(begin[0]) &&
(begin[1] == 0x0f)) {
On 2015/08/14 21:12:59, Mark Seaborn wrote:
> Nit: don't need ()s around "begin[1] == 0x0f"

Done.

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:115: ptr[2] = 0x29;
On 2015/08/14 21:12:59, Mark Seaborn wrote:
> It's surprising that you're using "begin" to read, but "ptr" to write.  Can
you
> make it consistent?

Done.

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:132: ptr[end - begin - 1] =
0x90;
On 2015/08/14 21:12:59, Mark Seaborn wrote:
> How about commenting this as: 0x90; /* NOP */
> 
> Same below.

Done.

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:158: memcmp(begin + 2,
"\x0f\xe7", 2) == 0) {
On 2015/08/14 21:12:59, Mark Seaborn wrote:
> Fix indentation alignment

Done.

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_common.c:164: #endif
On 2015/08/14 21:12:59, Mark Seaborn wrote:
> For safety, add:
> #else
> # error Unknown architecture

Done.

[Mark Seaborn, 2015-08-21 21:13:32]

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_64.c (right):

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_64.c:75:
callback_data.bundle_begin_offset = (uintptr_t) data & kBundleMask;
On 2015/08/14 21:12:59, Mark Seaborn wrote:
> I'm not sure I understand this parameter.  You're taking a trusted address and
> ANDing it with kBundleMask?  That seems like a very hacky thing to do.

This will need to get addressed...

I will take over this change and do some further cleanups in order to get it
committed.

[QiaoRuiwhu, 2015-08-24 19:48:14]

qiaoruiwhu@gmail.com changed reviewers:
+ QiaoRuiwhu@gmail.com

[QiaoRuiwhu, 2015-08-24 19:48:15]

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
File src/trusted/validator_ragel/dfa_validate_64.c (right):

https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
src/trusted/validator_ragel/dfa_validate_64.c:75:
callback_data.bundle_begin_offset = (uintptr_t) data & kBundleMask;
On 2015/08/21 21:13:32, Mark Seaborn wrote:
> On 2015/08/14 21:12:59, Mark Seaborn wrote:
> > I'm not sure I understand this parameter.  You're taking a trusted address
and
> > ANDing it with kBundleMask?  That seems like a very hacky thing to do.
> 
> This will need to get addressed...
> 
> I will take over this change and do some further cleanups in order to get it
> committed.

This is because the 'data' argument, which is the pointer to code_to_verify, is
not necessarily kBundleSize aligned. At least some callers of
ApplyDfaValidator_x86_64() function do not pass in an aligned pointer. Note that
ValidateChunkAMD64() does not enforce 'data' argument to be kBundleSize aligned
either, although it validates bundle by bundle, starting from 'data'.

I suspect the decision of not enforcing 'data' to be kBundleSize aligned was
made on purpose. This is useful, for example, when user supplies some code
buffer (not aligned) which is used only for validation, and later the code can
be copied to code region (aligned) for execution.

If this is not true, we could potentially change all callers to align 'data'
argument before passing in.

An alternative of computing the offset inside a bundle (current approach) is to
pass the offset in as an additional argument. However, this requires function
interface change.

Hopefuly above explanation clarifies a little bit.

[Mark Seaborn, 2015-08-25 19:01:05]

On 24 August 2015 at 12:48, <QiaoRuiwhu@gmail.com> wrote:

>
>
>
https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
> File src/trusted/validator_ragel/dfa_validate_64.c (right):
>
>
>
https://codereview.chromium.org/1269113003/diff/450001/src/trusted/validator_...
> src/trusted/validator_ragel/dfa_validate_64.c:75:
> callback_data.bundle_begin_offset = (uintptr_t) data & kBundleMask;
> On 2015/08/21 21:13:32, Mark Seaborn wrote:
>
>> On 2015/08/14 21:12:59, Mark Seaborn wrote:
>> > I'm not sure I understand this parameter.  You're taking a trusted
>> address and
>
> > ANDing it with kBundleMask?  That seems like a very hacky thing to do.
>
>
> This will need to get addressed...
>>
>
> I will take over this change and do some further cleanups in order to get
>> it
>
> committed.
>>
>
> This is because the 'data' argument, which is the pointer to
> code_to_verify, is not necessarily kBundleSize aligned. At least some
> callers of ApplyDfaValidator_x86_64() function do not pass in an aligned
> pointer. Note that ValidateChunkAMD64() does not enforce 'data' argument
> to be kBundleSize aligned either, although it validates bundle by
> bundle, starting from 'data'.
>
> I suspect the decision of not enforcing 'data' to be kBundleSize aligned
> was made on purpose. This is useful, for example, when user supplies
> some code buffer (not aligned) which is used only for validation, and
> later the code can be copied to code region (aligned) for execution.
>
> If this is not true, we could potentially change all callers to align
> 'data' argument before passing in.
>
> An alternative of computing the offset inside a bundle (current
> approach) is to pass the offset in as an additional argument. However,
> this requires function interface change.
>
> Hopefuly above explanation clarifies a little bit.


Right, I understand the reason is to figure out where the bundle starts.

Here's a cleaner way to do it:
https://codereview.chromium.org/1309953002/diff2/1:20001/src/trusted/validato...
-- Pass in a pointer to the start of the code chunk being validated
(data->chunk_begin), and calculate:
bundle_begin = data->chunk_begin + ((begin - data->chunk_begin) &
~kBundleMask);

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.