Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13974008:
ARM GPU process Seccomp-BPF policy. (Closed)
|
Created:
7 years, 8 months ago by Jorge Lucangeli Obes Modified:
7 years, 8 months ago Reviewers:
jln (very slow on Chromium) CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, jln+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionARM GPU process Seccomp-BPF policy.
BUG=232068
TEST=Together with a change that enables the policy:
TEST=daisy boots and Chrome works.
TEST=Tests in www/~jorgelo/no_crawl/gpu/ pass.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195986
Patch Set 1 #
Total comments: 5
Patch Set 2 : Refactor ARM GPU sandbox into its own functions. #
Total comments: 14
Patch Set 3 : Address comments. #Patch Set 4 : Don't enable the policy yet. #Messages
Total messages: 9 (0 generated)
Together with the Broker changes (and a separate change actually enabling it), this policy works on daisy.
Let's try to split things a little bit more cleanly (and this is overdue for a large refactor!). https://chromiumcodereview.appspot.com/13974008/diff/1/content/common/sandbox... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://chromiumcodereview.appspot.com/13974008/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:1283: #if defined(__arm__) Let's make another GPU process policy instead. Name GpuArmIntelDriverPolicy or something along these lines. Make it clear in a comment that the if (IsArchitectureArm()) is just a convenient shortcut to detect that configuration, but that the policy is specific to one driver. From that policy, you can inherit the current, generic GPU policy. I also think you should just allow all of IsAdvancedScheduler(), it's not much more attack surface than what this allows. If we're concerned about the IO Scheduler, let's split it up in its own function. For access(), if we need to allow it, we should add it to the broker process that allows open(). Add it as a TODO on the existing bug for NONBLOCK / CLOEXEC. Also remember: use IseArchitectureArm() for anything that doesn't trigger a compile error, and #ifdef as last resort. https://chromiumcodereview.appspot.com/13974008/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:1482: // On ARM we're enabling the sandbox before the X connection is made, Similarly, let's cleanly split this as its own ARM-with-intel-GPU function. You could make helper functions such as: AddX86IntGpuWhiteListedFiled(std::.....* read_whitelist,...* write_whitelist); that you call from here. InitGpuBrokerProcess() can take the policy as an argument to determine which one of the helpers to call.
https://chromiumcodereview.appspot.com/13974008/diff/1/content/common/sandbox... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://chromiumcodereview.appspot.com/13974008/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:1283: #if defined(__arm__) Thinking about it a bit more, perhaps the policies should just be fully separate (and inherit on the baseline policy). Inheritance from an intermediate GPU policy would create more complexity for little gain. Inheritance from the generic GPU policy is probably just wrong. Let me know what you think.
PTAL https://codereview.chromium.org/13974008/diff/1/content/common/sandbox_seccom... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://codereview.chromium.org/13974008/diff/1/content/common/sandbox_seccom... content/common/sandbox_seccomp_bpf_linux.cc:1283: #if defined(__arm__) On 2013/04/20 15:05:00, Julien Tinnes wrote: > Thinking about it a bit more, perhaps the policies should just be fully separate > (and inherit on the baseline policy). > > Inheritance from an intermediate GPU policy would create more complexity for > little gain. Inheritance from the generic GPU policy is probably just wrong. > > Let me know what you think. Done by inheriting from Baseline, agreed that makes more sense. https://codereview.chromium.org/13974008/diff/1/content/common/sandbox_seccom... content/common/sandbox_seccomp_bpf_linux.cc:1482: // On ARM we're enabling the sandbox before the X connection is made, On 2013/04/19 21:28:27, Julien Tinnes wrote: > Similarly, let's cleanly split this as its own ARM-with-intel-GPU function. > > You could make helper functions such as: > AddX86IntGpuWhiteListedFiled(std::.....* read_whitelist,...* write_whitelist); > that you call from here. > > InitGpuBrokerProcess() can take the policy as an argument to determine which one > of the helpers to call. Split ARM portion into its own function. I ended up needing different "sandbox enabling" callbacks. I could have added a |policy| parameter to BrokerProcess::Init, but decided against it to keep coupling to a minimum. We could eventually do some base::Bind magic to have proper currying in Enable*GpuBrokerPolicyCallback.
Looks pretty good! We're almost there. access() support in the broker process should land soon. I'll send NONBLOCK / CLOEXEC support tomorrow. https://chromiumcodereview.appspot.com/13974008/diff/6001/content/common/sand... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://chromiumcodereview.appspot.com/13974008/diff/6001/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:1321: // ARM GPU sandbox is started earlier so we need to allow more stuff. Maybe add a clear comment: this means that network access will be allowed in the GPU sandbox. It's clearly not ideal, but we can take care of this later. Do you know what's happening ? Does it need actual network access or just local sockets? No way we could restrict with parameter inspection ? https://chromiumcodereview.appspot.com/13974008/diff/6001/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:1322: case __NR_access: As discussed, let's get rid of that once you can ;) https://chromiumcodereview.appspot.com/13974008/diff/6001/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:1326: case __NR_getpeername: Please sort! https://chromiumcodereview.appspot.com/13974008/diff/6001/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:1340: if (IsAdvancedScheduler(sysno)) This one should compile on all architectures (I'm pretty adamant about avoiding #ifdef like the plague). Added benefit: you can put this one and the next one in a single if. https://chromiumcodereview.appspot.com/13974008/diff/6001/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:1553: bool (*sandbox_callback)(void) = EnableGpuBrokerPolicyCallback; It's confusing, I'd just initialize it to NULL, and there should be a clear switch-like if statement that sets it to right value depending on the policy. https://chromiumcodereview.appspot.com/13974008/diff/6001/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:1556: read_whitelist.push_back(kDriCard0Path); Are these needed for Mali ? If not, put them clearly in the if gpu_policy == GpuProcessPolicy (see comment below). https://chromiumcodereview.appspot.com/13974008/diff/6001/content/common/sand... content/common/sandbox_seccomp_bpf_linux.cc:1562: if (IsArchitectureArm() && gpu_policy == ArmMaliGpuProcessPolicy) { I think the right choice is if(gpu_policy == XX) { CHECK(IsArchitecture..()); ... } I would put an else if (gpu_policy == GpuProcessPolicy) right after (or maybe better: before), so that it's clear and in one place how things diverge.
PTAL. https://codereview.chromium.org/13974008/diff/6001/content/common/sandbox_sec... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://codereview.chromium.org/13974008/diff/6001/content/common/sandbox_sec... content/common/sandbox_seccomp_bpf_linux.cc:1321: // ARM GPU sandbox is started earlier so we need to allow more stuff. On 2013/04/23 02:22:47, Julien Tinnes wrote: > Maybe add a clear comment: this means that network access will be allowed in the > GPU sandbox. > > It's clearly not ideal, but we can take care of this later. Do you know what's > happening ? > > Does it need actual network access or just local sockets? No way we could > restrict with parameter inspection ? I think we might be able to, but I'd rather get FS lockdown first and then tighten this up. I'll update the comment. https://codereview.chromium.org/13974008/diff/6001/content/common/sandbox_sec... content/common/sandbox_seccomp_bpf_linux.cc:1322: case __NR_access: On 2013/04/23 02:22:47, Julien Tinnes wrote: > As discussed, let's get rid of that once you can ;) Added TODO. Since this CL does *not* enable the sandbox, I can make sure I only enable the sandbox once the broker changes land. https://codereview.chromium.org/13974008/diff/6001/content/common/sandbox_sec... content/common/sandbox_seccomp_bpf_linux.cc:1326: case __NR_getpeername: On 2013/04/23 02:22:47, Julien Tinnes wrote: > Please sort! Done. https://codereview.chromium.org/13974008/diff/6001/content/common/sandbox_sec... content/common/sandbox_seccomp_bpf_linux.cc:1340: if (IsAdvancedScheduler(sysno)) On 2013/04/23 02:22:47, Julien Tinnes wrote: > This one should compile on all architectures (I'm pretty adamant about avoiding > #ifdef like the plague). > > Added benefit: you can put this one and the next one in a single if. Done. https://codereview.chromium.org/13974008/diff/6001/content/common/sandbox_sec... content/common/sandbox_seccomp_bpf_linux.cc:1553: bool (*sandbox_callback)(void) = EnableGpuBrokerPolicyCallback; On 2013/04/23 02:22:47, Julien Tinnes wrote: > It's confusing, I'd just initialize it to NULL, and there should be a clear > switch-like if statement that sets it to right value depending on the policy. Done. https://codereview.chromium.org/13974008/diff/6001/content/common/sandbox_sec... content/common/sandbox_seccomp_bpf_linux.cc:1556: read_whitelist.push_back(kDriCard0Path); On 2013/04/23 02:22:47, Julien Tinnes wrote: > Are these needed for Mali ? If not, put them clearly in the if gpu_policy == > GpuProcessPolicy (see comment below). Yep, they're needed for both. https://codereview.chromium.org/13974008/diff/6001/content/common/sandbox_sec... content/common/sandbox_seccomp_bpf_linux.cc:1562: if (IsArchitectureArm() && gpu_policy == ArmMaliGpuProcessPolicy) { On 2013/04/23 02:22:47, Julien Tinnes wrote: > I think the right choice is if(gpu_policy == XX) { CHECK(IsArchitecture..()); > ... } > > I would put an else if (gpu_policy == GpuProcessPolicy) right after (or maybe > better: before), so that it's clear and in one place how things diverge. Done.
lgtm as something to iterate on, with the understanding that you will lock down socket parameters and access() soon. .Xauthority makes me uneasy as well. Most of these resources probably only need to be acquired once and might be discarded afterwards. We may want to change how the sandbox works in the future and add more stages: - Initialize Sandbox with broker - Initialize process (GPU etc..) - Lock-down broker Then we could also broker out socket etc completely and then lock them down when we lock down the broker.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jorgelo@chromium.org/13974008/16001
Message was sent while issue was closed.
Change committed as 195986 |
