Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
784733002:
content: bpf: exclude the syscalls if arm64 (Closed)
Descriptioncontent: bpf: exclude the syscalls if arm64
__NR_open, __NR_access are not defined on arm64. So, I just blocked
the syscall to build properly on arm64.
BUG=None
Committed: https://crrev.com/c37fb8efaecfd3ca87725466de4abdeae86ad343
Cr-Commit-Position: refs/heads/master@{#308281}
Patch Set 1 #
Total comments: 5
Patch Set 2 #
Total comments: 12
Patch Set 3 : content: bpf: exclude the syscalls if arm64 #
Messages
Total messages: 26 (4 generated)
parkch98@gmail.com changed reviewers: + hidehiko@chromium.org, jln@chromium.org, leecam@chromium.org, mdempsky@chromium.org, rickyz@chromium.org
https://codereview.chromium.org/784733002/diff/1/content/common/sandbox_linux... File content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc (right): https://codereview.chromium.org/784733002/diff/1/content/common/sandbox_linux... content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:112: case __NR_access: Do we need to allow __NR_accessat or something else? https://codereview.chromium.org/784733002/diff/1/content/common/sandbox_linux... File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right): https://codereview.chromium.org/784733002/diff/1/content/common/sandbox_linux... content/common/sandbox_linux/bpf_gpu_policy_linux.cc:200: || defined(__aarch64__) Is this actually required? The comment suggests this should be x86 specific, which makes me think even the __mips__ presence is superfluous.
Just out of interest what platform are you building for? The only arm64 device we support at the moment is the Nexus 9 and being Android does not use the gpu broker. Are you building Arm64 Linux? If so what SoC/GPU are you targeting? This area of the code is somewhat gpu driver specific and may not play well with a new platform.
Hi, Actually, I'm developing tizen platform. Crosswalk of tizen platform was derived from blink/chromium project. When I tried to enable arm64 build on tizen, I've got syscall errors in this codes. So, I need this patch to address build problem on tizen arm64. You can find the build result of tizen below: https://build.tizen.org/package/show?package=crosswalk-bin&project=devel%3Aar... I'm trying to land a tizen arm64 platform on the ARM64 juno board. It might has mali-t628 GPU. Later, I also tried to add arm64 specific codes for GPU. On Sun, Dec 7, 2014 at 1:34 PM, <leecam@chromium.org> wrote: > Just out of interest what platform are you building for? > > The only arm64 device we support at the moment is the Nexus 9 and being > Android > does not use the gpu broker. > > Are you building Arm64 Linux? If so what SoC/GPU are you targeting? > > This area of the code is somewhat gpu driver specific and may not play well > with > a new platform. > > > > https://codereview.chromium.org/784733002/ -- Best Regards, Chanho Park To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/784733002/diff/1/content/common/sandbox_linux... File content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc (right): https://codereview.chromium.org/784733002/diff/1/content/common/sandbox_linux... content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:112: case __NR_access: On 2014/12/07 03:46:40, mdempsky wrote: > Do we need to allow __NR_accessat or something else? Hmm. There is no __NR_access on the arm64. Maybe __NR_faccessat can be used instead of it. https://codereview.chromium.org/784733002/diff/1/content/common/sandbox_linux... File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right): https://codereview.chromium.org/784733002/diff/1/content/common/sandbox_linux... content/common/sandbox_linux/bpf_gpu_policy_linux.cc:200: || defined(__aarch64__) On 2014/12/07 03:46:40, mdempsky wrote: > Is this actually required? The comment suggests this should be x86 specific, > which makes me think even the __mips__ presence is superfluous. You're right. I'll remove this inclusion on aarch64.
On 2014/12/07 06:33:49, cometzero wrote: > Hi, Actually, I'm developing tizen platform. Crosswalk of tizen > platform was derived from blink/chromium project. When I tried to > enable arm64 build on tizen, I've got syscall errors in this codes. > So, I need this patch to address build problem on tizen arm64. > > You can find the build result of tizen below: > https://build.tizen.org/package/show?package=crosswalk-bin&project=devel%3Aar... > > I'm trying to land a tizen arm64 platform on the ARM64 juno board. It > might has mali-t628 GPU. > Later, I also tried to add arm64 specific codes for GPU. > > On Sun, Dec 7, 2014 at 1:34 PM, <mailto:leecam@chromium.org> wrote: > > Just out of interest what platform are you building for? > > > > The only arm64 device we support at the moment is the Nexus 9 and being > > Android > > does not use the gpu broker. > > > > Are you building Arm64 Linux? If so what SoC/GPU are you targeting? > > > > This area of the code is somewhat gpu driver specific and may not play well > > with > > a new platform. > > > > > > > > https://codereview.chromium.org/784733002/ > > > > -- > Best Regards, > Chanho Park > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Hi Chanho, Ah cool, as Crosswalk is a webview replacement does it make use of the GPU broker? Does Crosswalk run the GPU as a separate process when running on Tizen? Maybe you don't need to build this code at all? To be honest our GPU support for Arm Linux needs improvement, we only support Arm mali on ChromeOS builds. As part of the on-going refactoring of the GPU sandbox we hope to able to infer the correct GPU driver and apply the correct sandbox. At the moment mali is only assumed if and only if we running ChromeOS on Arm. So you might need to do some work in your chromium fork to use mali on Arm64 Linux. Anyhow thanks for the CL to get it fixed as the Chromium sandbox should at least build for Arm64 Linux (even if the policies need a little work).
https://codereview.chromium.org/784733002/diff/1/content/common/sandbox_linux... File content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc (right): https://codereview.chromium.org/784733002/diff/1/content/common/sandbox_linux... content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:112: case __NR_access: On 2014/12/07 06:47:38, cometzero wrote: > On 2014/12/07 03:46:40, mdempsky wrote: > > Do we need to allow __NR_accessat or something else? > > Hmm. There is no __NR_access on the arm64. Maybe __NR_faccessat can be used > instead of it. Yep __NR_faccessat is the replacement for __NR_access. In sandbox/linux/syscall_broker/broker_host.cc you'll see a sys_open function that wraps 'open' so we ensure we directly call __NR_openat. It might be worth doing something similar for __NR_access and explicitly calling the faccessat syscall then only allowing that in the policy. That should work across all of our supported architectures.
The CQ bit was checked by parkch98@gmail.com
The CQ bit was unchecked by parkch98@gmail.com
Need to get review of patchset#2
https://codereview.chromium.org/784733002/diff/20001/content/common/sandbox_l... File content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc (right): https://codereview.chromium.org/784733002/diff/20001/content/common/sandbox_l... content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:111: #if !defined(aarch64) You mean __aarch64__, no?
Thanks for the change https://codereview.chromium.org/784733002/diff/20001/content/common/sandbox_l... File content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc (right): https://codereview.chromium.org/784733002/diff/20001/content/common/sandbox_l... content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:111: #if !defined(aarch64) Would #if defined(__NR_access) and similar be more a more self-documenting check here and elsewhere? https://codereview.chromium.org/784733002/diff/20001/content/common/sandbox_l... content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:114: #endif // !defined(aarch64) nit: 2 spaces before the // here and elsewhere https://codereview.chromium.org/784733002/diff/20001/content/common/sandbox_l... content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:131: #if defined(__arm__) || defined(__aarch64__) Are any of these #ifs in this file actually needed? The policy is called CrosArmGpuProcessPolicy, so I suspect these ifs might be left over from before we had a separate class for the ARM policy.
https://codereview.chromium.org/784733002/diff/20001/content/common/sandbox_l... File content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc (right): https://codereview.chromium.org/784733002/diff/20001/content/common/sandbox_l... content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:131: #if defined(__arm__) || defined(__aarch64__) On 2014/12/08 19:22:07, rickyz wrote: > Are any of these #ifs in this file actually needed? The policy is called > CrosArmGpuProcessPolicy, so I suspect these ifs might be left over from before > we had a separate class for the ARM policy. At some point we could consider doing that, but for consistency, let's keep the architecture version for now in this small patch. It's nice to be able to immediately gauge the security depending on the architecture, so I'm not certain we'll want to switch, but I could be convinced.
https://codereview.chromium.org/784733002/diff/20001/content/common/sandbox_l... File content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc (right): https://codereview.chromium.org/784733002/diff/20001/content/common/sandbox_l... content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:111: #if !defined(aarch64) On 2014/12/08 19:09:24, jln wrote: > You mean __aarch64__, no? Oops. You're right. I'll fix it. https://codereview.chromium.org/784733002/diff/20001/content/common/sandbox_l... content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:111: #if !defined(aarch64) On 2014/12/08 19:22:07, rickyz wrote: > Would #if defined(__NR_access) and similar be more a more self-documenting check > here and elsewhere? Hmm. Looks good. To do that, however, we should change all of the syscalls to support such self-checks. https://codereview.chromium.org/784733002/diff/20001/content/common/sandbox_l... content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:114: #endif // !defined(aarch64) On 2014/12/08 19:22:07, rickyz wrote: > nit: 2 spaces before the // here and elsewhere Thanks. I'll fix it. https://codereview.chromium.org/784733002/diff/20001/content/common/sandbox_l... content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:131: #if defined(__arm__) || defined(__aarch64__) On 2014/12/08 19:22:07, rickyz wrote: > Are any of these #ifs in this file actually needed? The policy is called > CrosArmGpuProcessPolicy, so I suspect these ifs might be left over from before > we had a separate class for the ARM policy. I think arm and arm64 are using similar GPU like mali and have similar policy for them except syscall numbers. BTW, I prepared below three patches on second patch. However, they were merged into only one patch during posting second patch. ea26ef9 content: bpf: add __NR_faccessat syscall e298c77 content: bpf: support aarch64 syscalls likewise arm b1c8dac content: bpf: exclude the syscalls if arm64
https://codereview.chromium.org/784733002/diff/20001/content/common/sandbox_l... File content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc (right): https://codereview.chromium.org/784733002/diff/20001/content/common/sandbox_l... content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:111: #if !defined(aarch64) > Hmm. Looks good. To do that, however, we should change all of the syscalls to > support such self-checks. Sorry, I don't understand - does #if defined(__NR_access) not work currently? I thought the macros wouldn't be defined on architectures where the syscall isn't supported.
https://codereview.chromium.org/784733002/diff/20001/content/common/sandbox_l... File content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc (right): https://codereview.chromium.org/784733002/diff/20001/content/common/sandbox_l... content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:111: #if !defined(aarch64) On 2014/12/09 20:34:14, rickyz wrote: > > Hmm. Looks good. To do that, however, we should change all of the syscalls to > > support such self-checks. > Sorry, I don't understand - does #if defined(__NR_access) not work currently? I > thought the macros wouldn't be defined on architectures where the syscall isn't > supported. Yes, I think that's correct, it should work (we have linux_syscalls.h to make sure that everything is defined, regardless of the system's header files). However, I do not want to mix the two models, of architecture / syscall existence #ifdefs. We did make a conscious choice to use the latter, rather than the former. I do still think that overall this works better, but I could be convinced otherwise. If we do decide we want to switch to a #ifdef(syscall) model, we should write the pros / cons in a bug and then do so consistently and it should be independent of this CL.
https://codereview.chromium.org/784733002/diff/20001/content/common/sandbox_l... File content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc (right): https://codereview.chromium.org/784733002/diff/20001/content/common/sandbox_l... content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:111: #if !defined(aarch64) On 2014/12/09 20:39:01, jln wrote: > On 2014/12/09 20:34:14, rickyz wrote: > However, I do not want to mix the two models, of architecture / syscall > existence #ifdefs. > > We did make a conscious choice to use the latter, rather than the former. I do > still think that overall this works better, but I could be convinced otherwise. > If we do decide we want to switch to a #ifdef(syscall) model, we should write > the pros / cons in a bug and then do so consistently and it should be > independent of this CL. Ah, I see, I had not fully grasped what you were saying in your other comment before. Never mind about this for now then.
On 2014/12/09 20:39:01, jln wrote: > https://codereview.chromium.org/784733002/diff/20001/content/common/sandbox_l... > File content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc (right): > > https://codereview.chromium.org/784733002/diff/20001/content/common/sandbox_l... > content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:111: #if > !defined(aarch64) > On 2014/12/09 20:34:14, rickyz wrote: > > > Hmm. Looks good. To do that, however, we should change all of the syscalls > to > > > support such self-checks. > > Sorry, I don't understand - does #if defined(__NR_access) not work currently? > I > > thought the macros wouldn't be defined on architectures where the syscall > isn't > > supported. > > Yes, I think that's correct, it should work (we have linux_syscalls.h to make > sure that everything is defined, regardless of the system's header files). > > However, I do not want to mix the two models, of architecture / syscall > existence #ifdefs. > > We did make a conscious choice to use the latter, rather than the former. I do > still think that overall this works better, but I could be convinced otherwise. > If we do decide we want to switch to a #ifdef(syscall) model, we should write > the pros / cons in a bug and then do so consistently and it should be > independent of this CL. Do I need to prepare next patch? Or Is this patch good to you?
lgtm
On 2014/12/10 18:18:55, jln wrote: > lgtm Thank you for your review :)
On 2014/12/09 20:43:33, rickyz wrote: > https://codereview.chromium.org/784733002/diff/20001/content/common/sandbox_l... > File content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc (right): > > https://codereview.chromium.org/784733002/diff/20001/content/common/sandbox_l... > content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:111: #if > !defined(aarch64) > On 2014/12/09 20:39:01, jln wrote: > > On 2014/12/09 20:34:14, rickyz wrote: > > However, I do not want to mix the two models, of architecture / syscall > > existence #ifdefs. > > > > We did make a conscious choice to use the latter, rather than the former. I do > > still think that overall this works better, but I could be convinced > otherwise. > > If we do decide we want to switch to a #ifdef(syscall) model, we should write > > the pros / cons in a bug and then do so consistently and it should be > > independent of this CL. > Ah, I see, I had not fully grasped what you were saying in your other comment > before. Never mind about this for now then. Hi ricky, I need one more +1 to be merged this patch. Can you add +1 for this patch?
The CQ bit was checked by parkch98@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/784733002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c37fb8efaecfd3ca87725466de4abdeae86ad343 Cr-Commit-Position: refs/heads/master@{#308281} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
