Make command buffer commands and immediate data volatile

Make command buffer commands and immediate data volatile

Because command buffer commands and immediate data live in shared memory that
can be modified by an untrusted process, the data must be tagged as volatile to
ensure that compiler does not optimize the service-side checks in a way that
could cause double reads / TOCTOU issues.

Additionally, this provides a good indication that pointers reference
attacker-controlled memory.

BUG=597625, 597636
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/a728ad1c1b56c5c81c516ad11fead19e569b046e
Cr-Commit-Position: refs/heads/master@{#416646}

[piman, 2016-08-25 03:35:32]

Description was changed from

==========
Make command buffer commands and immediate data volatile

Because command buffer commands and immediate data live in shared memory that
can be modified by an untrusted process, the data must be tagged as volatile to
ensure that compiler does not optimize the service-side checks in a way that
could cause double reads / TOCTOU issues.

Additionally, this provides a good indication that pointers reference
attacker-controlled memory.

BUG=597625, 597636
==========

to

==========
Make command buffer commands and immediate data volatile

Because command buffer commands and immediate data live in shared memory that
can be modified by an untrusted process, the data must be tagged as volatile to
ensure that compiler does not optimize the service-side checks in a way that
could cause double reads / TOCTOU issues.

Additionally, this provides a good indication that pointers reference
attacker-controlled memory.

BUG=597625, 597636
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[piman, 2016-08-25 03:35:41]

The CQ bit was checked by piman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 03:36:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 03:38:22]

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

[commit-bot: I haz the power, 2016-08-25 03:38:23]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[piman, 2016-08-29 23:14:36]

The CQ bit was checked by piman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 23:51:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[piman, 2016-08-30 01:17:10]

The CQ bit was checked by piman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 01:55:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 03:50:00]

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

[commit-bot: I haz the power, 2016-08-30 03:50:01]

Dry run: This issue passed the CQ dry run.

[piman, 2016-08-31 22:29:16]

The CQ bit was checked by piman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 22:29:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 00:31:17]

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

[commit-bot: I haz the power, 2016-09-01 00:31:18]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[piman, 2016-09-01 00:40:37]

piman@chromium.org changed reviewers:
+ sievers@chromium.org

[piman, 2016-09-01 00:40:38]

So, I made this CL, it works, but I don't know if we want to land it.


The good:
Basically this tags all commands and immediate data as volatile, to prevent the
compiler from "optimizing" and causing it to read shared memory twice (which is
a transformation that it's allowed to do - e.g. under register pressure, it
could decide to do that as opposed to spill to the stack).
Furthermore, it helps recognise when data is untrusted, so that we can pay
attention to it and avoid double-reads (unless we can assume it's harmless, e.g.
glUniform* functions) - the exercise was useful to produce
https://codereview.chromium.org/2264253003 , which fixed a few of those.


The slightly bad but mostly neutral:
I looked at the generated code, on both x86-64, x86 and arm. Overall the code is
a tiny bit bigger (a couple of kb), most functions are unchanged, a bunch
increase by 1 or 2 bytes (not generating obviously "worse" code, just picks
different instructions), a handful get shorter ('volatile' prevents some
auto-vectorization, such as in DoUniform4fv for the path that transforms into
bool vecs - I'm not convinced the vectorized implementation helps in the general
case though), and a handful of "large" functions get a bigger hit (e.g.
Tex*Image* functions, 10-20ish bytes, seems to come from extra register
pressure), but they're already expensive, so it's in the noise.
Looking at performance, I tried a few things (among others running the fuzzer on
the entire corpus) but could not detect any measurable impact.


The ugly:
I couldn't find any obvious instance of a double-read in the currently generated
command buffer code, at least not one that was fixed by this CL (I diffed the
before- and after- objects). I did a manual test case that showed that gcc does
emit double-reads under register pressure (interestingly, clang doesn't), but
for chrome command buffer code, I saw the compiler generally preferring to save
things on the stack.


So the bottom line is that this CL may not fix any "real" problem currently, and
arguably makes the generated code ever-so-slightly worse, hence why I'm
hesitant. It does gives some future-proofness for when compilers evolve and may
generate problematic code. The other benefit is as stated, to tag the data in
the code (with /some/ compiler enforcement), though nothing prevents us from
writing code that does double-reads explicitly. Thoughts?

(Side note: this doesn't address other cases where we used shared memory, e.g.
shm buffers, but I will do that next if we decide to take this).

[no sievers, 2016-09-01 21:54:46]

On 2016/09/01 00:40:38, piman wrote:
> So, I made this CL, it works, but I don't know if we want to land it.
> 
> 
> The good:
> Basically this tags all commands and immediate data as volatile, to prevent
the
> compiler from "optimizing" and causing it to read shared memory twice (which
is
> a transformation that it's allowed to do - e.g. under register pressure, it
> could decide to do that as opposed to spill to the stack).
> Furthermore, it helps recognise when data is untrusted, so that we can pay
> attention to it and avoid double-reads (unless we can assume it's harmless,
e.g.
> glUniform* functions) - the exercise was useful to produce
> https://codereview.chromium.org/2264253003 , which fixed a few of those.
> 
> 
> The slightly bad but mostly neutral:
> I looked at the generated code, on both x86-64, x86 and arm. Overall the code
is
> a tiny bit bigger (a couple of kb), most functions are unchanged, a bunch
> increase by 1 or 2 bytes (not generating obviously "worse" code, just picks
> different instructions), a handful get shorter ('volatile' prevents some
> auto-vectorization, such as in DoUniform4fv for the path that transforms into
> bool vecs - I'm not convinced the vectorized implementation helps in the
general
> case though), and a handful of "large" functions get a bigger hit (e.g.
> Tex*Image* functions, 10-20ish bytes, seems to come from extra register
> pressure), but they're already expensive, so it's in the noise.
> Looking at performance, I tried a few things (among others running the fuzzer
on
> the entire corpus) but could not detect any measurable impact.
> 
> 
> The ugly:
> I couldn't find any obvious instance of a double-read in the currently
generated
> command buffer code, at least not one that was fixed by this CL (I diffed the
> before- and after- objects). I did a manual test case that showed that gcc
does
> emit double-reads under register pressure (interestingly, clang doesn't), but
> for chrome command buffer code, I saw the compiler generally preferring to
save
> things on the stack.
> 
> 
> So the bottom line is that this CL may not fix any "real" problem currently,
and
> arguably makes the generated code ever-so-slightly worse, hence why I'm
> hesitant. It does gives some future-proofness for when compilers evolve and
may
> generate problematic code. The other benefit is as stated, to tag the data in
> the code (with /some/ compiler enforcement), though nothing prevents us from
> writing code that does double-reads explicitly. Thoughts?
> 

I don't know if I'd classify the last part as 'ugly' though.

TBH I was also expecting that it'd be very unlikely for exploitable to be
generated.
But it does seem to just be the right thing to do esp. like you pointed out
there is no way for us to convince ourselves that it won't ever happen.
And at the end of the day one argument somewhere in one function call might be
enough.

So maybe just land it and make the code right and show that we mean business?
After all we already made this architecture quite complex with the best of
intentions, so I don't think the extra verbosity hurts anything (significant
performance decreases maybe would have been something else to think about).

[piman, 2016-09-01 22:30:32]

On 2016/09/01 21:54:46, sievers wrote:
> On 2016/09/01 00:40:38, piman wrote:
> > So, I made this CL, it works, but I don't know if we want to land it.
> > 
> > 
> > The good:
> > Basically this tags all commands and immediate data as volatile, to prevent
> the
> > compiler from "optimizing" and causing it to read shared memory twice (which
> is
> > a transformation that it's allowed to do - e.g. under register pressure, it
> > could decide to do that as opposed to spill to the stack).
> > Furthermore, it helps recognise when data is untrusted, so that we can pay
> > attention to it and avoid double-reads (unless we can assume it's harmless,
> e.g.
> > glUniform* functions) - the exercise was useful to produce
> > https://codereview.chromium.org/2264253003 , which fixed a few of those.
> > 
> > 
> > The slightly bad but mostly neutral:
> > I looked at the generated code, on both x86-64, x86 and arm. Overall the
code
> is
> > a tiny bit bigger (a couple of kb), most functions are unchanged, a bunch
> > increase by 1 or 2 bytes (not generating obviously "worse" code, just picks
> > different instructions), a handful get shorter ('volatile' prevents some
> > auto-vectorization, such as in DoUniform4fv for the path that transforms
into
> > bool vecs - I'm not convinced the vectorized implementation helps in the
> general
> > case though), and a handful of "large" functions get a bigger hit (e.g.
> > Tex*Image* functions, 10-20ish bytes, seems to come from extra register
> > pressure), but they're already expensive, so it's in the noise.
> > Looking at performance, I tried a few things (among others running the
fuzzer
> on
> > the entire corpus) but could not detect any measurable impact.
> > 
> > 
> > The ugly:
> > I couldn't find any obvious instance of a double-read in the currently
> generated
> > command buffer code, at least not one that was fixed by this CL (I diffed
the
> > before- and after- objects). I did a manual test case that showed that gcc
> does
> > emit double-reads under register pressure (interestingly, clang doesn't),
but
> > for chrome command buffer code, I saw the compiler generally preferring to
> save
> > things on the stack.
> > 
> > 
> > So the bottom line is that this CL may not fix any "real" problem currently,
> and
> > arguably makes the generated code ever-so-slightly worse, hence why I'm
> > hesitant. It does gives some future-proofness for when compilers evolve and
> may
> > generate problematic code. The other benefit is as stated, to tag the data
in
> > the code (with /some/ compiler enforcement), though nothing prevents us from
> > writing code that does double-reads explicitly. Thoughts?
> > 
> 
> I don't know if I'd classify the last part as 'ugly' though.
> 
> TBH I was also expecting that it'd be very unlikely for exploitable to be
> generated.
> But it does seem to just be the right thing to do esp. like you pointed out
> there is no way for us to convince ourselves that it won't ever happen.
> And at the end of the day one argument somewhere in one function call might be
> enough.
> 
> So maybe just land it and make the code right and show that we mean business?
> After all we already made this architecture quite complex with the best of
> intentions, so I don't think the extra verbosity hurts anything (significant
> performance decreases maybe would have been something else to think about).

Ok, thanks for the feedback. We can then try to land it and watch for
regressions - hopefully if any they'll show up quickly because a straight revert
will probably be soon impossible (though s/volatile// should be easy).

Sorry about the huge CL, it's just not something that was easy to do piecemeal.
If you want to go through it, a large portion is just autogenerated code which
just adds 'volatile' in the right places, and so is the passthrough code. The
rest, in particular gles2_cmd_decoder.cc, has some changes worth looking a bit
more closely at, in particular using const_casts to remove volatile, in places
where we assume the code isn't sensitive to potential double-read (e.g.
glUniform*, straight up copies, etc.).

[piman, 2016-09-01 22:48:10]

The CQ bit was checked by piman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 22:48:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 01:49:24]

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

[commit-bot: I haz the power, 2016-09-02 01:49:25]

Dry run: This issue passed the CQ dry run.

[no sievers, 2016-09-02 21:17:34]

sorry for the delay, lgtm!

https://codereview.chromium.org/2275203002/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2275203002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8662: // an extra copy?
Hmm.. maybe we shouldn't use gfx::Transform then and just an internal helper
that takes a volatile pointer. (Would be ridiculous to qualify with volatile in
random other external classes.)

https://codereview.chromium.org/2275203002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8670: std::copy(transform,
transform + arraysize(gl_matrix), gl_matrix);
This array is small, but was wondering about std::copy() specialization and
volatile ptrs:

http://stackoverflow.com/questions/36098055/stdis-trivially-copyable-why-are-...

" The problem with this is that a volatile qualified type may need to be copied
in a specific way (by copying using only atomic operations on multithreaded
platforms, for example) in order to avoid the “memory tearing” that may occur
with a byte-by-byte copy."

So if we were doing this a lot, maybe better to cast the volatile away.

https://codereview.chromium.org/2275203002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16374: std::copy(matrix, matrix
+ 16, target_matrix);
likewise

[piman, 2016-09-02 22:59:47]

The CQ bit was checked by piman@chromium.org

[piman, 2016-09-02 22:59:48]

The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/2275203002/#ps100001
(title: "std::copy->const_cast+memcpy")

[commit-bot: I haz the power, 2016-09-02 23:00:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[piman, 2016-09-02 23:11:06]

https://codereview.chromium.org/2275203002/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2275203002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8662: // an extra copy?
On 2016/09/02 21:17:34, sievers_OOOtil_Sep12 wrote:
> Hmm.. maybe we shouldn't use gfx::Transform then and just an internal helper
> that takes a volatile pointer. (Would be ridiculous to qualify with volatile
in
> random other external classes.)

Yes, agreed it would be ridiculous...
gfx::Transform (SkMatrix44) is convenient though, because of the matrix multiply
(with fast paths if one or the other is identity, or just a scale+translate,
which is likely), so I'm not sure it's worth duplicating the logic.
Maybe without introducing volatile, a helper that directly exposes the float
array (and resets the type mask), so that we can just copy into it. I'll think
about options.

https://codereview.chromium.org/2275203002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8670: std::copy(transform,
transform + arraysize(gl_matrix), gl_matrix);
On 2016/09/02 21:17:34, sievers_OOOtil_Sep12 wrote:
> This array is small, but was wondering about std::copy() specialization and
> volatile ptrs:
> 
>
http://stackoverflow.com/questions/36098055/stdis-trivially-copyable-why-are-...
> 
> " The problem with this is that a volatile qualified type may need to be
copied
> in a specific way (by copying using only atomic operations on multithreaded
> platforms, for example) in order to avoid the “memory tearing” that may occur
> with a byte-by-byte copy."
> 
> So if we were doing this a lot, maybe better to cast the volatile away.

I went a bit back and forth on this. The const_cast is a theoretical problem,
especially since the compiler will inline the copy (fixed size), but looking at
the generated code, it is both safe and a lot better, and since this could be in
the fast path, I went back to const_cast + memcpy.

https://codereview.chromium.org/2275203002/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16374: std::copy(matrix, matrix
+ 16, target_matrix);
On 2016/09/02 21:17:34, sievers_OOOtil_Sep12 wrote:
> likewise

ditto.

[commit-bot: I haz the power, 2016-09-03 00:19:04]

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

[commit-bot: I haz the power, 2016-09-03 00:19:06]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[piman, 2016-09-06 15:45:48]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2016-09-06 15:46:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 16:48:25]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-06 16:50:56]

Description was changed from

==========
Make command buffer commands and immediate data volatile

Because command buffer commands and immediate data live in shared memory that
can be modified by an untrusted process, the data must be tagged as volatile to
ensure that compiler does not optimize the service-side checks in a way that
could cause double reads / TOCTOU issues.

Additionally, this provides a good indication that pointers reference
attacker-controlled memory.

BUG=597625, 597636
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Make command buffer commands and immediate data volatile

Because command buffer commands and immediate data live in shared memory that
can be modified by an untrusted process, the data must be tagged as volatile to
ensure that compiler does not optimize the service-side checks in a way that
could cause double reads / TOCTOU issues.

Additionally, this provides a good indication that pointers reference
attacker-controlled memory.

BUG=597625, 597636
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/a728ad1c1b56c5c81c516ad11fead19e569b046e
Cr-Commit-Position: refs/heads/master@{#416646}
==========

[commit-bot: I haz the power, 2016-09-06 16:50:58]

Patchset 6 (id:??) landed as
https://crrev.com/a728ad1c1b56c5c81c516ad11fead19e569b046e
Cr-Commit-Position: refs/heads/master@{#416646}