[MIPS] Make validator allow only two load patterns via $t8

[MIPS] Make validator allow only two load patterns via $t8

Validator now allows (only) two load word patterns via $t8, with zero and
one word displacement, i.e.

lw      reg, 0($t8)
lw      reg, 4($t8)

This is related to the change how we access TLS pointers for both user
code and IRT. For more, see #4345.

Moving the value of $t8 to another register is allowed, though this will
be changed shortly with the new validator.

Also, fixed minor presubmit failures in the files this change touches.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4345

Committed: https://chromium.googlesource.com/native_client/src/native_client/+/26c7e6ae1327a43c948f4666fef9e84fe351ab7c

[petarj, 2015-12-17 19:37:52]

Description was changed from

==========
[MIPS] Make validator allow only two load patterns via $t8

Validator now allows only two load word patterns from $t8, with zero and
one word displacement, i.e.

lw      reg, 0($t8)
lw      reg, 4($t8)

This is related to the change how we access TLS pointers for both user
code and IRT. For more, see #4345.

Also, fixed minor presubmit failures in the files this change touches.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4345
==========

to

==========
[MIPS] Make validator allow only two load patterns via $t8

Validator now allows only two load word patterns from $t8, with zero and
one word displacement, i.e.

lw      reg, 0($t8)
lw      reg, 4($t8)

This is related to the change how we access TLS pointers for both user
code and IRT. For more, see #4345.

Also, fixed minor presubmit failures in the files this change touches.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4345
==========

[petarj, 2015-12-17 19:37:52]

petarj@mips.com changed reviewers:
+ dschuff@chromium.org, mseaborn@chromium.org

[petarj, 2015-12-17 19:39:38]

PTAL.

[petarj, 2015-12-18 11:14:15]

Description was changed from

==========
[MIPS] Make validator allow only two load patterns via $t8

Validator now allows only two load word patterns from $t8, with zero and
one word displacement, i.e.

lw      reg, 0($t8)
lw      reg, 4($t8)

This is related to the change how we access TLS pointers for both user
code and IRT. For more, see #4345.

Also, fixed minor presubmit failures in the files this change touches.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4345
==========

to

==========
[MIPS] Make validator allow only two load patterns via $t8

Validator now allows (only) two load word patterns from $t8, with zero and
one word displacement, i.e.

lw      reg, 0($t8)
lw      reg, 4($t8)

This is related to the change how we access TLS pointers for both user
code and IRT. For more, see #4345.

Also, fixed minor presubmit failures in the files this change touches.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4345
==========

[petarj, 2015-12-18 11:14:47]

Description was changed from

==========
[MIPS] Make validator allow only two load patterns via $t8

Validator now allows (only) two load word patterns from $t8, with zero and
one word displacement, i.e.

lw      reg, 0($t8)
lw      reg, 4($t8)

This is related to the change how we access TLS pointers for both user
code and IRT. For more, see #4345.

Also, fixed minor presubmit failures in the files this change touches.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4345
==========

to

==========
[MIPS] Make validator allow only two load patterns via $t8

Validator now allows (only) two load word patterns via $t8, with zero and
one word displacement, i.e.

lw      reg, 0($t8)
lw      reg, 4($t8)

This is related to the change how we access TLS pointers for both user
code and IRT. For more, see #4345.

Also, fixed minor presubmit failures in the files this change touches.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4345
==========

[petarj, 2015-12-24 13:52:11]

ping.

[Mark Seaborn, 2015-12-27 03:46:35]

How is moving the value of $t8 to another register treated now?  I'm not sure if
that was allowed before.  Ideally it should be disallowed, otherwise it would
leak information that breaks ASLR.

https://codereview.chromium.org/1535443005/diff/1/src/trusted/validator_mips/...
File src/trusted/validator_mips/inst_classes.h (right):

https://codereview.chromium.org/1535443005/diff/1/src/trusted/validator_mips/...
src/trusted/validator_mips/inst_classes.h:140: */
Nit: Fix indentation here

https://codereview.chromium.org/1535443005/diff/1/src/trusted/validator_mips/...
File src/trusted/validator_mips/model.h (right):

https://codereview.chromium.org/1535443005/diff/1/src/trusted/validator_mips/...
src/trusted/validator_mips/model.h:45: static const Number kTls            = 24;
 // $t8 = holds pointer to tp index.
"tp index" isn't quite right, is it?  Is it an index now?  At least it won't be
when these changes are finished.  Should this be something like "points into
NaClThreadContext"?

https://codereview.chromium.org/1535443005/diff/1/src/trusted/validator_mips/...
File src/trusted/validator_mips/testdata/test_tp_access.S (right):

https://codereview.chromium.org/1535443005/diff/1/src/trusted/validator_mips/...
src/trusted/validator_mips/testdata/test_tp_access.S:11: .align 4
Nit: can you use a consistent indentation?  There's a mix of 2 and 3 spaces in
this file.

[Deznutz, 2015-12-27 16:44:37]

oneinam777@gmail.com changed reviewers:
+ oneinam777@gmail.com

[Deznutz, 2015-12-27 16:44:38]

The CQ bit was checked by oneinam777@gmail.com

[Deznutz, 2015-12-27 16:44:38]

lgtm

[commit-bot: I haz the power, 2015-12-27 16:44:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1535443005/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1535443005/1

[commit-bot: I haz the power, 2015-12-27 16:46:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-27 16:46:46]

Try jobs failed on following builders:
  nacl-presubmit on tryserver.nacl (JOB_FAILED,
http://build.chromium.org/p/tryserver.nacl/builders/nacl-presubmit/builds/505)

[petarj, 2015-12-28 14:07:00]

petarj@mips.com changed reviewers:
- oneinam777@gmail.com

[petarj, 2015-12-28 14:07:00]

> How is moving the value of $t8 to another register treated now?

It is allowed (at the moment), but it will be changed with the next set
of changes. The next set will rewrite the MIPS validator to be inline
with ARM validator, and it will include the change not to allow any
operations with $t8 except for loads via $t8 with 0 and 4 displacement.

> I'm not sure if that was allowed before.

It was allowed.

> Ideally it should be disallowed, otherwise it would
> leak information that breaks ASLR.

Agree. It will be part of the next change.

https://chromiumcodereview.appspot.com/1535443005/diff/1/src/trusted/validato...
File src/trusted/validator_mips/inst_classes.h (right):

https://chromiumcodereview.appspot.com/1535443005/diff/1/src/trusted/validato...
src/trusted/validator_mips/inst_classes.h:140: */
On 2015/12/27 03:46:35, Mark Seaborn wrote:
> Nit: Fix indentation here

Done.

https://chromiumcodereview.appspot.com/1535443005/diff/1/src/trusted/validato...
File src/trusted/validator_mips/model.h (right):

https://chromiumcodereview.appspot.com/1535443005/diff/1/src/trusted/validato...
src/trusted/validator_mips/model.h:45: static const Number kTls            = 24;
 // $t8 = holds pointer to tp index.
On 2015/12/27 03:46:35, Mark Seaborn wrote:
> "tp index" isn't quite right, is it?  Is it an index now?  At least it won't
be
> when these changes are finished.  Should this be something like "points into
> NaClThreadContext"?

I have modified it to "holds address of tls_value1 in NaClThreadContext".

https://chromiumcodereview.appspot.com/1535443005/diff/1/src/trusted/validato...
File src/trusted/validator_mips/testdata/test_tp_access.S (right):

https://chromiumcodereview.appspot.com/1535443005/diff/1/src/trusted/validato...
src/trusted/validator_mips/testdata/test_tp_access.S:11: .align 4
On 2015/12/27 03:46:35, Mark Seaborn wrote:
> Nit: can you use a consistent indentation?  There's a mix of 2 and 3 spaces in
> this file.

Done. More files in this directory could be modified the same way.

[Mark Seaborn, 2015-12-30 02:12:41]

On 2015/12/28 14:07:00, petarj wrote:
> > How is moving the value of $t8 to another register treated now?
> 
> It is allowed (at the moment), but it will be changed with the next set
> of changes.

OK.  Can you mention that in the commit message, please?

https://chromiumcodereview.appspot.com/1535443005/diff/1/src/trusted/validato...
File src/trusted/validator_mips/model.h (right):

https://chromiumcodereview.appspot.com/1535443005/diff/1/src/trusted/validato...
src/trusted/validator_mips/model.h:45: static const Number kTls            = 24;
 // $t8 = holds pointer to tp index.
On 2015/12/28 14:07:00, petarj wrote:
> On 2015/12/27 03:46:35, Mark Seaborn wrote:
> > "tp index" isn't quite right, is it?  Is it an index now?  At least it won't
> be
> > when these changes are finished.  Should this be something like "points into
> > NaClThreadContext"?
> 
> I have modified it to "holds address of tls_value1 in NaClThreadContext".

Well, it doesn't hold that address, at least not until your other change,
https://codereview.chromium.org/1536713003/, is committed.

You didn't make it clear what order you want to commit the changes in.  I assume
this change should be committed first -- I think it should work given that you
committed the LLVM-side change (but you still need to run pnacl/deps_update.py
to update pnacl/COMPONENT_REVISIONS).  And then you would commit
https://codereview.chromium.org/1536713003/ after (committing that first
wouldn't be safe, I think).  Have I got that right?

https://chromiumcodereview.appspot.com/1535443005/diff/20001/src/trusted/vali...
File src/trusted/validator_mips/validation-report.py (right):

https://chromiumcodereview.appspot.com/1535443005/diff/20001/src/trusted/vali...
src/trusted/validator_mips/validation-report.py:54:
'kProblemUnsafeLoadStoreThreadPointer': ['This is not a valid'
Add space before the last single quote

[petarj, 2015-12-30 16:50:49]

Description was changed from

==========
[MIPS] Make validator allow only two load patterns via $t8

Validator now allows (only) two load word patterns via $t8, with zero and
one word displacement, i.e.

lw      reg, 0($t8)
lw      reg, 4($t8)

This is related to the change how we access TLS pointers for both user
code and IRT. For more, see #4345.

Also, fixed minor presubmit failures in the files this change touches.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4345
==========

to

==========
[MIPS] Make validator allow only two load patterns via $t8

Validator now allows (only) two load word patterns via $t8, with zero and
one word displacement, i.e.

lw      reg, 0($t8)
lw      reg, 4($t8)

This is related to the change how we access TLS pointers for both user
code and IRT. For more, see #4345.

Moving the value of $t8 to another register is allowed, though this will
be changed shortly with the new validator.

Also, fixed minor presubmit failures in the files this change touches.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4345
==========

[petarj, 2015-12-30 17:04:58]

On 2015/12/30 02:12:41, Mark Seaborn wrote:
>>> How is moving the value of $t8 to another register treated now?
>>
>> It is allowed (at the moment), but it will be changed with the next set
>> of changes.
> OK.  Can you mention that in the commit message, please?

Done.

> You didn't make it clear what order you want to commit the changes in.

Ideally, I would need to apply both deps_update and these two changes in
one. Less ideally, I would merge the other change (#1536713003) with this
change and put it together (either before or after deps update), and be
aware MIPS port is broken for a short time until the other change(s) land.

Last option, I can commit this one and deps update in separate changes,
then as the last one - the change #1536713003.

If we do it all in one day, then it should not really be a problem which
way we do it.

https://chromiumcodereview.appspot.com/1535443005/diff/20001/src/trusted/vali...
File src/trusted/validator_mips/validation-report.py (right):

https://chromiumcodereview.appspot.com/1535443005/diff/20001/src/trusted/vali...
src/trusted/validator_mips/validation-report.py:54:
'kProblemUnsafeLoadStoreThreadPointer': ['This is not a valid'
On 2015/12/30 02:12:40, Mark Seaborn wrote:
> Add space before the last single quote

Done.

[petarj, 2016-01-11 14:41:02]

ping.

[Mark Seaborn, 2016-01-11 17:17:23]

LGTM

[petarj, 2016-01-11 18:07:50]

The CQ bit was checked by petarj@mips.com

[petarj, 2016-01-11 18:07:50]

The patchset sent to the CQ was uploaded after l-g-t-m from oneinam777@gmail.com
Link to the patchset: https://codereview.chromium.org/1535443005/#ps40001
(title: "Minor update.")

[commit-bot: I haz the power, 2016-01-11 18:07:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1535443005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1535443005/40001

[commit-bot: I haz the power, 2016-01-11 18:58:00]

Description was changed from

==========
[MIPS] Make validator allow only two load patterns via $t8

Validator now allows (only) two load word patterns via $t8, with zero and
one word displacement, i.e.

lw      reg, 0($t8)
lw      reg, 4($t8)

This is related to the change how we access TLS pointers for both user
code and IRT. For more, see #4345.

Moving the value of $t8 to another register is allowed, though this will
be changed shortly with the new validator.

Also, fixed minor presubmit failures in the files this change touches.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4345
==========

to

==========
[MIPS] Make validator allow only two load patterns via $t8

Validator now allows (only) two load word patterns via $t8, with zero and
one word displacement, i.e.

lw      reg, 0($t8)
lw      reg, 4($t8)

This is related to the change how we access TLS pointers for both user
code and IRT. For more, see #4345.

Moving the value of $t8 to another register is allowed, though this will
be changed shortly with the new validator.

Also, fixed minor presubmit failures in the files this change touches.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4345

Committed:
https://chromium.googlesource.com/native_client/src/native_client/+/26c7e6ae1...
==========

[commit-bot: I haz the power, 2016-01-11 18:58:01]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/native_client/src/native_client/+/26c7e6ae1...