Make validator require read sandboxing on ARM.

Make validator require read sandboxing on ARM.  Added some minor test extensions
for write sandboxing also (SP updates and two-address stores).

BUG= http://code.google.com/p/nativeclient/issues/detail?id=2129
TEST=utman_test.sh
Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=6991

[sehr (please use chromium), 2011-10-13 22:45:12]

One more try.

[jasonwkim, 2011-10-13 23:05:41]

http://codereview.chromium.org/8275008/diff/1/src/trusted/validator_arm/inst_...
File src/trusted/validator_arm/inst_classes.cc (right):

http://codereview.chromium.org/8275008/diff/1/src/trusted/validator_arm/inst_...
src/trusted/validator_arm/inst_classes.cc:219: bool pre_index = i.bit(24);
some comments?

http://codereview.chromium.org/8275008/diff/1/src/trusted/validator_arm/inst_...
src/trusted/validator_arm/inst_classes.cc:281: if (pre_index) {
ditto

http://codereview.chromium.org/8275008/diff/1/src/trusted/validator_arm/valid...
File src/trusted/validator_arm/validator_tests.cc (right):

http://codereview.chromium.org/8275008/diff/1/src/trusted/validator_arm/valid...
src/trusted/validator_arm/validator_tests.cc:536: * load/store used in an unsafe
manner.
Do we have any tests that verify read sandboxing is worjjing?

[sehr, 2011-10-14 21:36:39]

On 2011/10/13 23:05:41, jasonwkim wrote:
>
http://codereview.chromium.org/8275008/diff/1/src/trusted/validator_arm/inst_...
> File src/trusted/validator_arm/inst_classes.cc (right):
> 
>
http://codereview.chromium.org/8275008/diff/1/src/trusted/validator_arm/inst_...
> src/trusted/validator_arm/inst_classes.cc:219: bool pre_index = i.bit(24);
> some comments?
> 
>
http://codereview.chromium.org/8275008/diff/1/src/trusted/validator_arm/inst_...
> src/trusted/validator_arm/inst_classes.cc:281: if (pre_index) {
> ditto
> 
>
http://codereview.chromium.org/8275008/diff/1/src/trusted/validator_arm/valid...
> File src/trusted/validator_arm/validator_tests.cc (right):
> 
>
http://codereview.chromium.org/8275008/diff/1/src/trusted/validator_arm/valid...
> src/trusted/validator_arm/validator_tests.cc:536: * load/store used in an
unsafe
> manner.
> Do we have any tests that verify read sandboxing is worjjing?

We have tests now, and the trybots are again happy.  PTAL.

[sehr (please use chromium), 2011-10-18 18:46:39]

ping.

[robertm, 2011-10-18 21:19:55]

Since it is very likely that we have to revisit the load sandboxing when people
complain about execution speed
or executable size.

Could we make the load  sandbox checking easy to disable
(if it isn't already)

http://codereview.chromium.org/8275008/diff/11004/src/trusted/service_runtime...
File src/trusted/service_runtime/arch/arm/nacl_switch_to_app_arm.c (left):

http://codereview.chromium.org/8275008/diff/11004/src/trusted/service_runtime...
src/trusted/service_runtime/arch/arm/nacl_switch_to_app_arm.c:47: /* Skipping a
2-byte halt brings us to 0 mod 16. */
this change was not mentioned in the description

http://codereview.chromium.org/8275008/diff/11004/src/trusted/validator_arm/t...
File src/trusted/validator_arm/testdata/test_loads.S (right):

http://codereview.chromium.org/8275008/diff/11004/src/trusted/validator_arm/t...
src/trusted/validator_arm/testdata/test_loads.S:17: bundle0:
maybe add some pc and sp loads

http://codereview.chromium.org/8275008/diff/11004/src/trusted/validator_arm/t...
src/trusted/validator_arm/testdata/test_loads.S:21: bic r1, r1, #MASK       @
Confining an address in place
not sure whether those are tested elsewhere but

ldr r0, [r1,r2] 
should also be covered

[sehr (please use chromium), 2011-10-18 22:44:11]

Thanks for the reviews.  Requested tests were added.  PTAL.

http://codereview.chromium.org/8275008/diff/11004/src/trusted/service_runtime...
File src/trusted/service_runtime/arch/arm/nacl_switch_to_app_arm.c (left):

http://codereview.chromium.org/8275008/diff/11004/src/trusted/service_runtime...
src/trusted/service_runtime/arch/arm/nacl_switch_to_app_arm.c:47: /* Skipping a
2-byte halt brings us to 0 mod 16. */
On 2011/10/18 21:19:55, robertm wrote:
> this change was not mentioned in the description

Thanks.  This was not intended for this CL.

http://codereview.chromium.org/8275008/diff/11004/src/trusted/validator_arm/t...
File src/trusted/validator_arm/testdata/test_loads.S (right):

http://codereview.chromium.org/8275008/diff/11004/src/trusted/validator_arm/t...
src/trusted/validator_arm/testdata/test_loads.S:17: bundle0:
On 2011/10/18 21:19:55, robertm wrote:
> maybe add some pc and sp loads

Done.

http://codereview.chromium.org/8275008/diff/11004/src/trusted/validator_arm/t...
src/trusted/validator_arm/testdata/test_loads.S:21: bic r1, r1, #MASK       @
Confining an address in place
On 2011/10/18 21:19:55, robertm wrote:
> not sure whether those are tested elsewhere but
> 
> ldr r0, [r1,r2] 
> should also be covered 

Done.

[robertm, 2011-10-19 14:28:00]

How difficult is it to turn read sandboxing off again if we have to.
is there a flags in the validator or is it hardcoded

On 2011/10/18 22:44:11, sehr wrote:
> Thanks for the reviews.  Requested tests were added.  PTAL.
> 
>
http://codereview.chromium.org/8275008/diff/11004/src/trusted/service_runtime...
> File src/trusted/service_runtime/arch/arm/nacl_switch_to_app_arm.c (left):
> 
>
http://codereview.chromium.org/8275008/diff/11004/src/trusted/service_runtime...
> src/trusted/service_runtime/arch/arm/nacl_switch_to_app_arm.c:47: /* Skipping
a
> 2-byte halt brings us to 0 mod 16. */
> On 2011/10/18 21:19:55, robertm wrote:
> > this change was not mentioned in the description
> 
> Thanks.  This was not intended for this CL.
> 
>
http://codereview.chromium.org/8275008/diff/11004/src/trusted/validator_arm/t...
> File src/trusted/validator_arm/testdata/test_loads.S (right):
> 
>
http://codereview.chromium.org/8275008/diff/11004/src/trusted/validator_arm/t...
> src/trusted/validator_arm/testdata/test_loads.S:17: bundle0:
> On 2011/10/18 21:19:55, robertm wrote:
> > maybe add some pc and sp loads
> 
> Done.
> 
>
http://codereview.chromium.org/8275008/diff/11004/src/trusted/validator_arm/t...
> src/trusted/validator_arm/testdata/test_loads.S:21: bic r1, r1, #MASK       @
> Confining an address in place
> On 2011/10/18 21:19:55, robertm wrote:
> > not sure whether those are tested elsewhere but
> > 
> > ldr r0, [r1,r2] 
> > should also be covered 
> 
> Done.

[sehr (please use chromium), 2011-10-19 19:56:14]

It should be as simple as removing the safety and base_register methods on the
load classes in inst_classes.h, plus removing the related tests.
 On 2011/10/19 14:28:00, robertm wrote:
> How difficult is it to turn read sandboxing off again if we have to.
> is there a flags in the validator or is it hardcoded
> 
> On 2011/10/18 22:44:11, sehr wrote:
> > Thanks for the reviews.  Requested tests were added.  PTAL.
> > 
> >
>
http://codereview.chromium.org/8275008/diff/11004/src/trusted/service_runtime...
> > File src/trusted/service_runtime/arch/arm/nacl_switch_to_app_arm.c (left):
> > 
> >
>
http://codereview.chromium.org/8275008/diff/11004/src/trusted/service_runtime...
> > src/trusted/service_runtime/arch/arm/nacl_switch_to_app_arm.c:47: /*
Skipping
> a
> > 2-byte halt brings us to 0 mod 16. */
> > On 2011/10/18 21:19:55, robertm wrote:
> > > this change was not mentioned in the description
> > 
> > Thanks.  This was not intended for this CL.
> > 
> >
>
http://codereview.chromium.org/8275008/diff/11004/src/trusted/validator_arm/t...
> > File src/trusted/validator_arm/testdata/test_loads.S (right):
> > 
> >
>
http://codereview.chromium.org/8275008/diff/11004/src/trusted/validator_arm/t...
> > src/trusted/validator_arm/testdata/test_loads.S:17: bundle0:
> > On 2011/10/18 21:19:55, robertm wrote:
> > > maybe add some pc and sp loads
> > 
> > Done.
> > 
> >
>
http://codereview.chromium.org/8275008/diff/11004/src/trusted/validator_arm/t...
> > src/trusted/validator_arm/testdata/test_loads.S:21: bic r1, r1, #MASK      
@
> > Confining an address in place
> > On 2011/10/18 21:19:55, robertm wrote:
> > > not sure whether those are tested elsewhere but
> > > 
> > > ldr r0, [r1,r2] 
> > > should also be covered 
> > 
> > Done.

[robertm, 2011-10-19 20:00:12]

LGTM

On 2011/10/19 19:56:14, sehr wrote:
> It should be as simple as removing the safety and base_register methods on the
> load classes in inst_classes.h, plus removing the related tests.
>  On 2011/10/19 14:28:00, robertm wrote:
> > How difficult is it to turn read sandboxing off again if we have to.
> > is there a flags in the validator or is it hardcoded
> > 
> > On 2011/10/18 22:44:11, sehr wrote:
> > > Thanks for the reviews.  Requested tests were added.  PTAL.
> > > 
> > >
> >
>
http://codereview.chromium.org/8275008/diff/11004/src/trusted/service_runtime...
> > > File src/trusted/service_runtime/arch/arm/nacl_switch_to_app_arm.c (left):
> > > 
> > >
> >
>
http://codereview.chromium.org/8275008/diff/11004/src/trusted/service_runtime...
> > > src/trusted/service_runtime/arch/arm/nacl_switch_to_app_arm.c:47: /*
> Skipping
> > a
> > > 2-byte halt brings us to 0 mod 16. */
> > > On 2011/10/18 21:19:55, robertm wrote:
> > > > this change was not mentioned in the description
> > > 
> > > Thanks.  This was not intended for this CL.
> > > 
> > >
> >
>
http://codereview.chromium.org/8275008/diff/11004/src/trusted/validator_arm/t...
> > > File src/trusted/validator_arm/testdata/test_loads.S (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/8275008/diff/11004/src/trusted/validator_arm/t...
> > > src/trusted/validator_arm/testdata/test_loads.S:17: bundle0:
> > > On 2011/10/18 21:19:55, robertm wrote:
> > > > maybe add some pc and sp loads
> > > 
> > > Done.
> > > 
> > >
> >
>
http://codereview.chromium.org/8275008/diff/11004/src/trusted/validator_arm/t...
> > > src/trusted/validator_arm/testdata/test_loads.S:21: bic r1, r1, #MASK     

> @
> > > Confining an address in place
> > > On 2011/10/18 21:19:55, robertm wrote:
> > > > not sure whether those are tested elsewhere but
> > > > 
> > > > ldr r0, [r1,r2] 
> > > > should also be covered 
> > > 
> > > Done.

[jasonwkim, 2011-10-19 20:00:59]

On Wed, Oct 19, 2011 at 12:56 PM, <sehr@google.com> wrote:

> It should be as simple as removing the safety and base_register methods on
> the
> load classes in inst_classes.h, plus removing the related tests.
>
>
Making this switchable on the fly will be a bit of work - probably beyond
scope of this CL
Besides, I am due to face merge from hell once I restart thumb2 stuff again.

In short All your sandboxes and thumbs are belong to me :-)
I say LGTM unless robert has a strong objection


>  On 2011/10/19 14:28:00, robertm wrote:
>
>> How difficult is it to turn read sandboxing off again if we have to.
>> is there a flags in the validator or is it hardcoded
>>
>
>  On 2011/10/18 22:44:11, sehr wrote:
>> > Thanks for the reviews.  Requested tests were added.  PTAL.
>> >
>> >
>>
>
> http://codereview.chromium.**org/8275008/diff/11004/src/**
>
trusted/service_runtime/arch/**arm/nacl_switch_to_app_arm.c<http://codereview.chromium.org/8275008/diff/11004/src/trusted/service_runtime/arch/arm/nacl_switch_to_app_arm.c>
>
>> > File src/trusted/service_runtime/**arch/arm/nacl_switch_to_app_**arm.c
>> (left):
>> >
>> >
>>
>
> http://codereview.chromium.**org/8275008/diff/11004/src/**
>
trusted/service_runtime/arch/**arm/nacl_switch_to_app_arm.c#**oldcode47<http://codereview.chromium.org/8275008/diff/11004/src/trusted/service_runtime/arch/arm/nacl_switch_to_app_arm.c#oldcode47>
>
>> > src/trusted/service_runtime/**arch/arm/nacl_switch_to_app_**arm.c:47:
>> /*
>>
> Skipping
>
>> a
>> > 2-byte halt brings us to 0 mod 16. */
>> > On 2011/10/18 21:19:55, robertm wrote:
>> > > this change was not mentioned in the description
>> >
>> > Thanks.  This was not intended for this CL.
>> >
>> >
>>
>
> http://codereview.chromium.**org/8275008/diff/11004/src/**
>
trusted/validator_arm/**testdata/test_loads.S<http://codereview.chromium.org/8275008/diff/11004/src/trusted/validator_arm/testdata/test_loads.S>
>
>> > File src/trusted/validator_arm/**testdata/test_loads.S (right):
>> >
>> >
>>
>
> http://codereview.chromium.**org/8275008/diff/11004/src/**
>
trusted/validator_arm/**testdata/test_loads.S#**newcode17<http://codereview.chromium.org/8275008/diff/11004/src/trusted/validator_arm/testdata/test_loads.S#newcode17>
>
>> > src/trusted/validator_arm/**testdata/test_loads.S:17: bundle0:
>> > On 2011/10/18 21:19:55, robertm wrote:
>> > > maybe add some pc and sp loads
>> >
>> > Done.
>> >
>> >
>>
>
> http://codereview.chromium.**org/8275008/diff/11004/src/**
>
trusted/validator_arm/**testdata/test_loads.S#**newcode21<http://codereview.chromium.org/8275008/diff/11004/src/trusted/validator_arm/testdata/test_loads.S#newcode21>
>
>> > src/trusted/validator_arm/**testdata/test_loads.S:21: bic r1, r1, #MASK
>>
> @
>
>> > Confining an address in place
>> > On 2011/10/18 21:19:55, robertm wrote:
>> > > not sure whether those are tested elsewhere but
>> > >
>> > > ldr r0, [r1,r2]
>> > > should also be covered
>> >
>> > Done.
>>
>
>
>
>
http://codereview.chromium.**org/8275008/<http://codereview.chromium.org/8275...
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[sehr (please use chromium), 2011-10-20 19:53:14]

Thanks.  Committed as r6991.

On 2011/10/19 20:00:59, jasonwkim wrote:
> On Wed, Oct 19, 2011 at 12:56 PM, <mailto:sehr@google.com> wrote:
> 
> > It should be as simple as removing the safety and base_register methods on
> > the
> > load classes in inst_classes.h, plus removing the related tests.
> >
> >
> Making this switchable on the fly will be a bit of work - probably beyond
> scope of this CL
> Besides, I am due to face merge from hell once I restart thumb2 stuff again.
> 
> In short All your sandboxes and thumbs are belong to me :-)
> I say LGTM unless robert has a strong objection
> 
> 
> >  On 2011/10/19 14:28:00, robertm wrote:
> >
> >> How difficult is it to turn read sandboxing off again if we have to.
> >> is there a flags in the validator or is it hardcoded
> >>
> >
> >  On 2011/10/18 22:44:11, sehr wrote:
> >> > Thanks for the reviews.  Requested tests were added.  PTAL.
> >> >
> >> >
> >>
> >
> > http://codereview.chromium.**org/8275008/diff/11004/src/**
> >
>
trusted/service_runtime/arch/**arm/nacl_switch_to_app_arm.c<http://codereview.chromium.org/8275008/diff/11004/src/trusted/service_runtime/arch/arm/nacl_switch_to_app_arm.c>
> >
> >> > File src/trusted/service_runtime/**arch/arm/nacl_switch_to_app_**arm.c
> >> (left):
> >> >
> >> >
> >>
> >
> > http://codereview.chromium.**org/8275008/diff/11004/src/**
> >
>
trusted/service_runtime/arch/**arm/nacl_switch_to_app_arm.c#**oldcode47<http://codereview.chromium.org/8275008/diff/11004/src/trusted/service_runtime/arch/arm/nacl_switch_to_app_arm.c#oldcode47>
> >
> >> > src/trusted/service_runtime/**arch/arm/nacl_switch_to_app_**arm.c:47:
> >> /*
> >>
> > Skipping
> >
> >> a
> >> > 2-byte halt brings us to 0 mod 16. */
> >> > On 2011/10/18 21:19:55, robertm wrote:
> >> > > this change was not mentioned in the description
> >> >
> >> > Thanks.  This was not intended for this CL.
> >> >
> >> >
> >>
> >
> > http://codereview.chromium.**org/8275008/diff/11004/src/**
> >
>
trusted/validator_arm/**testdata/test_loads.S<http://codereview.chromium.org/8275008/diff/11004/src/trusted/validator_arm/testdata/test_loads.S>
> >
> >> > File src/trusted/validator_arm/**testdata/test_loads.S (right):
> >> >
> >> >
> >>
> >
> > http://codereview.chromium.**org/8275008/diff/11004/src/**
> >
>
trusted/validator_arm/**testdata/test_loads.S#**newcode17<http://codereview.chromium.org/8275008/diff/11004/src/trusted/validator_arm/testdata/test_loads.S#newcode17>
> >
> >> > src/trusted/validator_arm/**testdata/test_loads.S:17: bundle0:
> >> > On 2011/10/18 21:19:55, robertm wrote:
> >> > > maybe add some pc and sp loads
> >> >
> >> > Done.
> >> >
> >> >
> >>
> >
> > http://codereview.chromium.**org/8275008/diff/11004/src/**
> >
>
trusted/validator_arm/**testdata/test_loads.S#**newcode21<http://codereview.chromium.org/8275008/diff/11004/src/trusted/validator_arm/testdata/test_loads.S#newcode21>
> >
> >> > src/trusted/validator_arm/**testdata/test_loads.S:21: bic r1, r1, #MASK
> >>
> > @
> >
> >> > Confining an address in place
> >> > On 2011/10/18 21:19:55, robertm wrote:
> >> > > not sure whether those are tested elsewhere but
> >> > >
> >> > > ldr r0, [r1,r2]
> >> > > should also be covered
> >> >
> >> > Done.
> >>
> >
> >
> >
> >
>
http://codereview.chromium.**org/8275008/%3Chttp://codereview.chromium.org/82...>
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Native-Client-Reviews" group.
> To post to this group, send email to
mailto:native-client-reviews@googlegroups.com.
> To unsubscribe from this group, send email to
> mailto:native-client-reviews+unsubscribe@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/native-client-reviews?hl=en.
>

[sehr (please use chromium), 2011-10-20 21:12:02]

And reverted in r6993.  Broke trusted_mmap_test.

On 2011/10/20 19:53:14, sehr wrote:
> Thanks.  Committed as r6991.
> 
> On 2011/10/19 20:00:59, jasonwkim wrote:
> > On Wed, Oct 19, 2011 at 12:56 PM, <mailto:sehr@google.com> wrote:
> > 
> > > It should be as simple as removing the safety and base_register methods on
> > > the
> > > load classes in inst_classes.h, plus removing the related tests.
> > >
> > >
> > Making this switchable on the fly will be a bit of work - probably beyond
> > scope of this CL
> > Besides, I am due to face merge from hell once I restart thumb2 stuff again.
> > 
> > In short All your sandboxes and thumbs are belong to me :-)
> > I say LGTM unless robert has a strong objection
> > 
> > 
> > >  On 2011/10/19 14:28:00, robertm wrote:
> > >
> > >> How difficult is it to turn read sandboxing off again if we have to.
> > >> is there a flags in the validator or is it hardcoded
> > >>
> > >
> > >  On 2011/10/18 22:44:11, sehr wrote:
> > >> > Thanks for the reviews.  Requested tests were added.  PTAL.
> > >> >
> > >> >
> > >>
> > >
> > > http://codereview.chromium.**org/8275008/diff/11004/src/**
> > >
> >
>
trusted/service_runtime/arch/**arm/nacl_switch_to_app_arm.c<http://codereview.chromium.org/8275008/diff/11004/src/trusted/service_runtime/arch/arm/nacl_switch_to_app_arm.c>
> > >
> > >> > File src/trusted/service_runtime/**arch/arm/nacl_switch_to_app_**arm.c
> > >> (left):
> > >> >
> > >> >
> > >>
> > >
> > > http://codereview.chromium.**org/8275008/diff/11004/src/**
> > >
> >
>
trusted/service_runtime/arch/**arm/nacl_switch_to_app_arm.c#**oldcode47<http://codereview.chromium.org/8275008/diff/11004/src/trusted/service_runtime/arch/arm/nacl_switch_to_app_arm.c#oldcode47>
> > >
> > >> > src/trusted/service_runtime/**arch/arm/nacl_switch_to_app_**arm.c:47:
> > >> /*
> > >>
> > > Skipping
> > >
> > >> a
> > >> > 2-byte halt brings us to 0 mod 16. */
> > >> > On 2011/10/18 21:19:55, robertm wrote:
> > >> > > this change was not mentioned in the description
> > >> >
> > >> > Thanks.  This was not intended for this CL.
> > >> >
> > >> >
> > >>
> > >
> > > http://codereview.chromium.**org/8275008/diff/11004/src/**
> > >
> >
>
trusted/validator_arm/**testdata/test_loads.S<http://codereview.chromium.org/8275008/diff/11004/src/trusted/validator_arm/testdata/test_loads.S>
> > >
> > >> > File src/trusted/validator_arm/**testdata/test_loads.S (right):
> > >> >
> > >> >
> > >>
> > >
> > > http://codereview.chromium.**org/8275008/diff/11004/src/**
> > >
> >
>
trusted/validator_arm/**testdata/test_loads.S#**newcode17<http://codereview.chromium.org/8275008/diff/11004/src/trusted/validator_arm/testdata/test_loads.S#newcode17>
> > >
> > >> > src/trusted/validator_arm/**testdata/test_loads.S:17: bundle0:
> > >> > On 2011/10/18 21:19:55, robertm wrote:
> > >> > > maybe add some pc and sp loads
> > >> >
> > >> > Done.
> > >> >
> > >> >
> > >>
> > >
> > > http://codereview.chromium.**org/8275008/diff/11004/src/**
> > >
> >
>
trusted/validator_arm/**testdata/test_loads.S#**newcode21<http://codereview.chromium.org/8275008/diff/11004/src/trusted/validator_arm/testdata/test_loads.S#newcode21>
> > >
> > >> > src/trusted/validator_arm/**testdata/test_loads.S:21: bic r1, r1, #MASK
> > >>
> > > @
> > >
> > >> > Confining an address in place
> > >> > On 2011/10/18 21:19:55, robertm wrote:
> > >> > > not sure whether those are tested elsewhere but
> > >> > >
> > >> > > ldr r0, [r1,r2]
> > >> > > should also be covered
> > >> >
> > >> > Done.
> > >>
> > >
> > >
> > >
> > >
> >
>
http://codereview.chromium.**org/8275008/%253Chttp://codereview.chromium.org/...>
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Native-Client-Reviews" group.
> > To post to this group, send email to
> mailto:native-client-reviews@googlegroups.com.
> > To unsubscribe from this group, send email to
> > mailto:native-client-reviews+unsubscribe@googlegroups.com.
> > For more options, visit this group at
> > http://groups.google.com/group/native-client-reviews?hl=en.
> >