bpf gpu policy: Change GPU bpf policy to support DRI3

bpf gpu policy: Change GPU bpf policy to support DRI3

DRI3 creates files in /dev/shm/. This change adds /dev/shm to
the whitelist for non-ChromeOS Linux platforms.

These files are unlinked and truncated so the following
policy changes have also been made.

unlink is allowed in the broker process policy.
ftruncate is allowed in the gpu process policy

Now DRI3 is supported this change also reverts the
temporary fix to set env var LIBGL_DRI3_DISABLE
https://codereview.chromium.org/708043002

BUG=415681
TEST=Ubuntu 14.10 configured with DRI3

Committed: https://crrev.com/4e3c98a7788ba99f5dd72c87c9ba84e69fbd1bbf
Cr-Commit-Position: refs/heads/master@{#306698}

[leecam, 2014-12-02 02:00:30]

leecam@chromium.org changed reviewers:
+ jln@chromium.org, jorgelo@chromium.org

[leecam, 2014-12-02 02:00:45]

PTAL

[Jorge Lucangeli Obes, 2014-12-02 21:09:06]

https://codereview.chromium.org/759613008/diff/1/content/common/sandbox_linux...
File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):

https://codereview.chromium.org/759613008/diff/1/content/common/sandbox_linux...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:281: }
Let's keep the empty line below.

[leecam, 2014-12-03 17:08:51]

https://codereview.chromium.org/759613008/diff/1/content/common/sandbox_linux...
File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):

https://codereview.chromium.org/759613008/diff/1/content/common/sandbox_linux...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:281: }
On 2014/12/02 21:09:06, Jorge Lucangeli Obes wrote:
> Let's keep the empty line below.

Done.

[leecam, 2014-12-03 17:09:47]

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

[Jorge Lucangeli Obes, 2014-12-03 17:10:33]

lgtm

[leecam, 2014-12-03 17:10:50]

Adding piman as OWNER for gpu_main.cc revert

[leecam, 2014-12-03 17:12:13]

leecam@chromium.org changed reviewers:
+ kbr@chromium.org

[Jorge Lucangeli Obes, 2014-12-03 17:12:24]

Alternatively, Ken for OWNERS since Antoine might be busy.

[Ken Russell (switch to Gerrit), 2014-12-03 19:48:15]

LGTM overall, but a couple of comments / questions.

https://codereview.chromium.org/759613008/diff/20001/content/common/sandbox_l...
File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):

https://codereview.chromium.org/759613008/diff/20001/content/common/sandbox_l...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:138: // openat allowed.
Comment should be updated for the current state of the code.

https://codereview.chromium.org/759613008/diff/20001/content/common/sandbox_l...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:145: case __NR_unlink:
This doesn't allow arbitrary files to be unlinked, does it? There's a filter
elsewhere in the sandbox which evaluates the arguments? Is it worth adding a
comment and perhaps a pointer to the other code?

[jln (very slow on Chromium), 2014-12-03 19:57:59]

Let's discuss unlink(). It's very problematic, I don't think that was part of
the plan. Why is it needed?

https://codereview.chromium.org/759613008/diff/20001/content/common/sandbox_l...
File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):

https://codereview.chromium.org/759613008/diff/20001/content/common/sandbox_l...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:145: case __NR_unlink:
On 2014/12/03 19:48:15, Ken Russell wrote:
> This doesn't allow arbitrary files to be unlinked, does it? There's a filter
> elsewhere in the sandbox which evaluates the arguments? Is it worth adding a
> comment and perhaps a pointer to the other code?

This would indeed. Lee, what requires that? This would be a major break from the
sandbox: we mandate O_EXCL precisely to prevent taking over files owned by
another process. Allowing unlink would break that (in addition to the obvious
annoyance of attackers being able to delete your files).

Moreover, even if we decided we needed this (which we *really* shouldn't), this
should be brokered away, in order to keep the model of the sandbox whole (We
should be able to entirely dump FS access from the GPU process, which we
hopefully will materialize soon with a chroot to an empty dir or something like
this).

[Jorge Lucangeli Obes, 2014-12-03 21:01:50]

https://codereview.chromium.org/759613008/diff/20001/content/common/sandbox_l...
File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):

https://codereview.chromium.org/759613008/diff/20001/content/common/sandbox_l...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:145: case __NR_unlink:
On 2014/12/03 19:57:59, jln wrote:
> On 2014/12/03 19:48:15, Ken Russell wrote:
> > This doesn't allow arbitrary files to be unlinked, does it? There's a filter
> > elsewhere in the sandbox which evaluates the arguments? Is it worth adding a
> > comment and perhaps a pointer to the other code?
> 
> This would indeed. Lee, what requires that? This would be a major break from
the
> sandbox: we mandate O_EXCL precisely to prevent taking over files owned by
> another process. Allowing unlink would break that (in addition to the obvious
> annoyance of attackers being able to delete your files).
> 
> Moreover, even if we decided we needed this (which we *really* shouldn't),
this
> should be brokered away, in order to keep the model of the sandbox whole (We
> should be able to entirely dump FS access from the GPU process, which we
> hopefully will materialize soon with a chroot to an empty dir or something
like
> this).

This is the broker policy.

[leecam, 2014-12-03 21:03:34]

On 2014/12/03 19:57:59, jln wrote:
> Let's discuss unlink(). It's very problematic, I don't think that was part of
> the plan. Why is it needed?

DRI3 creates files in /dev/shm and we need to able to unlink them otherwise they
will just fill up over time. This was discussed in the bug:
https://code.google.com/p/chromium/issues/detail?id=415681 See #48 bullet 5. Its
also highlighted in the BrokerFilePermission bug:
https://code.google.com/p/chromium/issues/detail?id=432369

jln: We also discussed the need for unlink on chat...

> 
>
https://codereview.chromium.org/759613008/diff/20001/content/common/sandbox_l...
> File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):
> 
>
https://codereview.chromium.org/759613008/diff/20001/content/common/sandbox_l...
> content/common/sandbox_linux/bpf_gpu_policy_linux.cc:145: case __NR_unlink:
> On 2014/12/03 19:48:15, Ken Russell wrote:
> > This doesn't allow arbitrary files to be unlinked, does it? There's a filter
> > elsewhere in the sandbox which evaluates the arguments? Is it worth adding a
> > comment and perhaps a pointer to the other code?
> 
> This would indeed. Lee, what requires that? This would be a major break from
the
> sandbox: we mandate O_EXCL precisely to prevent taking over files owned by
> another process. Allowing unlink would break that (in addition to the obvious
> annoyance of attackers being able to delete your files).

Fear not, you can not unlink arbitrary files. We only allow unlink in the broker
process, not the gpu process. There is not a new Broker IPC to allow the client
to ask for files to be unlinked. All we have is a BrokerFilePermission that will
unlink a file as soon as its created. That permission mandates that the file is
created with O_EXCL.

So we only allow the deletion of files we create and we unlink them immediately
(just passing back the fd).

> 
> Moreover, even if we decided we needed this (which we *really* shouldn't),
this
> should be brokered away, in order to keep the model of the sandbox whole (We
> should be able to entirely dump FS access from the GPU process, which we
> hopefully will materialize soon with a chroot to an empty dir or something
like
> this).

This is brokered away...so we can drop the FS on the GPU process if we wanted
to.

[piman, 2014-12-03 21:09:11]

lgtm

[jln (very slow on Chromium), 2014-12-03 21:15:52]

On 2014/12/03 21:03:34, leecam wrote:

> This is brokered away...so we can drop the FS on the GPU process if we wanted
> to.

Ohh sorry. Jorge got it right, I didn't realize this was the broker policy :)

lgtm

[leecam, 2014-12-03 21:27:46]

The CQ bit was checked by leecam@chromium.org

[leecam, 2014-12-03 21:28:29]

Thanks all...comments fixed up.

https://codereview.chromium.org/759613008/diff/20001/content/common/sandbox_l...
File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):

https://codereview.chromium.org/759613008/diff/20001/content/common/sandbox_l...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:138: // openat allowed.
On 2014/12/03 19:48:15, Ken Russell wrote:
> Comment should be updated for the current state of the code.

Done.

https://codereview.chromium.org/759613008/diff/20001/content/common/sandbox_l...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:145: case __NR_unlink:
On 2014/12/03 19:48:15, Ken Russell wrote:
> This doesn't allow arbitrary files to be unlinked, does it? There's a filter
> elsewhere in the sandbox which evaluates the arguments? Is it worth adding a
> comment and perhaps a pointer to the other code?

Done.

[commit-bot: I haz the power, 2014-12-03 21:29:11]

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

[commit-bot: I haz the power, 2014-12-03 22:46:25]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-03 22:47:50]

Patchset 3 (id:??) landed as
https://crrev.com/4e3c98a7788ba99f5dd72c87c9ba84e69fbd1bbf
Cr-Commit-Position: refs/heads/master@{#306698}