gpu: Generate mailboxes on client side

gpu: Generate mailboxes on client side

Because mailboxes are now just a random number, we don't need to round-trip to
the gpu process to generate them, which lets us get rid of IPCs, layers, etc.

It also means generating a mailbox can't fail any more (even on a lost context),
so, removing some code paths and associated tests.

I'm adding a debug-only verification to ensure the mailboxes are generated from
the crypto-random function (in debug, we burn a byte to compute a mini XOR-check
as a sentinel for "we went through the Generate function"). It's not a secure
check, but should hit incorrect/unsafe usage.

BUG=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251570

[piman, 2014-02-14 05:36:53]

jamesr: please review (also OWNER for mojo)
jschuh: gpu_messages.h
agl: crypto/ OWNER.

[piman, 2014-02-14 05:38:59]

(note: this is on top of https://codereview.chromium.org/138763009/)

[jschuh, 2014-02-14 15:44:27]

I'm just going to rubberstamp lgtm this from the security side. The IPC message
changes are self-explanatory removals and we already discussed the security
design aspects thoroughly.

[agl, 2014-02-14 15:52:48]

https://codereview.chromium.org/165393003/diff/40001/crypto/random.h
File crypto/random.h (right):

https://codereview.chromium.org/165393003/diff/40001/crypto/random.h#newcode21
crypto/random.h:21: base::RandBytes(bytes, length);
I think this makes the compile-time people sad because the function has to be
included in a large number of compilation units that the linker then has to
dedup.

Rather I would call base::RandBytes directly from your code and comment both at
the call site, and in base/, that you depend on cryptographic randomness.

[danakj, 2014-02-14 16:55:09]

cc LGTM

[piman, 2014-02-14 19:28:47]

On 2014/02/14 15:52:48, agl wrote:
> https://codereview.chromium.org/165393003/diff/40001/crypto/random.h
> File crypto/random.h (right):
> 
> https://codereview.chromium.org/165393003/diff/40001/crypto/random.h#newcode21
> crypto/random.h:21: base::RandBytes(bytes, length);
> I think this makes the compile-time people sad because the function has to be
> included in a large number of compilation units that the linker then has to
> dedup.

Because the function is trivial, just passing through arguments, the callsite
will be compiled identically, just referencing a different function
(base::RandBytes instead of crypto::RandBytes). I'll argue that it'll also make
it strictly more efficient, though it's in the noise.

About having it included in each compilation unit, is it just that you want me
to mark it static inline? That way the compiler will just omit it from the CU.

> Rather I would call base::RandBytes directly from your code and comment both
at
> the call site, and in base/, that you depend on cryptographic randomness.

That works for /me/, but I think the security people like crypto-random to be
called from crypto:: not base:: (as the comments imply), so that it can be
found.
I'll defer to Justin as to whether he's happy with that or not.

An alternative is to separate crypto/random.* into its own separate library,
that crypto would depend on, but that can be compiled on nacl. It's mostly a
futile exercise, as the time to generate an extra library will likely outweigh
any savings re: linker de-dup.

[agl, 2014-02-14 22:15:38]

LGTM with qualifications.

If Justin is happy with calling base::RandBytes directly, I think that's better.
If not, then I think this is probably the best answer.

[jschuh, 2014-02-14 22:26:25]

On 2014/02/14 22:15:38, agl wrote:
> LGTM with qualifications.
> 
> If Justin is happy with calling base::RandBytes directly, I think that's
better.
> If not, then I think this is probably the best answer.

I think @rsleevi had reasons for wanting it this way. I'd ask him.

[jamesr, 2014-02-14 22:29:57]

lgtm

[piman, 2014-02-14 22:49:50]

+rsleevi ^^^

[Ryan Sleevi, 2014-02-14 23:20:00]

On 2014/02/14 22:49:50, piman wrote:
> +rsleevi ^^^

I'm fine with calling base::RandBytes directly from this function - as this is a
piece of code that will never be under the scope of FIPS or related security
boundaries (as opposed to other places where people "generate a
cryptographically-strong value for actual crypto")

Note this isn't so much a blanket exception as "In this case, it's fine".

[Ryan Sleevi, 2014-02-14 23:20:49]

https://codereview.chromium.org/165393003/diff/160001/crypto/random.h
File crypto/random.h (right):

https://codereview.chromium.org/165393003/diff/160001/crypto/random.h#newcode21
crypto/random.h:21: base::RandBytes(bytes, length);
Oh, missed this change. Don't do this.

[piman, 2014-02-14 23:36:35]

crypto/ bits gone in latest PS.

[piman, 2014-02-15 00:57:52]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2014-02-15 01:14:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/165393003/240001

[commit-bot: I haz the power, 2014-02-15 02:17:44]

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

[commit-bot: I haz the power, 2014-02-15 02:17:44]

Try jobs failed on following builders: linux_chromium_rel

[piman, 2014-02-15 05:56:24]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2014-02-15 05:56:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/165393003/240001

[commit-bot: I haz the power, 2014-02-15 05:56:53]

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

[commit-bot: I haz the power, 2014-02-15 05:56:53]

Try jobs failed on following builders: linux_chromium_rel

[piman, 2014-02-15 10:08:50]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2014-02-15 10:08:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/165393003/240001

[commit-bot: I haz the power, 2014-02-15 14:22:42]

Change committed as 251570

[awong, 2014-02-26 19:19:21]

drive-by nit...technially, we're using the wrong API for RandBytes().

Also, mdempsky points out that this should just be a "using base::RandBytes"
inside the crypto namespace.

https://codereview.chromium.org/165393003/diff/240001/gpu/command_buffer/comm...
File gpu/command_buffer/common/mailbox.cc (right):

https://codereview.chromium.org/165393003/diff/240001/gpu/command_buffer/comm...
gpu/command_buffer/common/mailbox.cc:38: base::RandBytes(result.name,
sizeof(result.name));
Use crypto::RandBytes() instead?

[Ryan Sleevi, 2014-02-26 19:24:40]

On 2014/02/26 19:19:21, awong wrote:
> drive-by nit...technially, we're using the wrong API for RandBytes().
> 
> Also, mdempsky points out that this should just be a "using base::RandBytes"
> inside the crypto namespace.
> 
>
https://codereview.chromium.org/165393003/diff/240001/gpu/command_buffer/comm...
> File gpu/command_buffer/common/mailbox.cc (right):
> 
>
https://codereview.chromium.org/165393003/diff/240001/gpu/command_buffer/comm...
> gpu/command_buffer/common/mailbox.cc:38: base::RandBytes(result.name,
> sizeof(result.name));
> Use crypto::RandBytes() instead?

No. See my comments #11/#12.