Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10830348:
Add ARM syscalls to syscall sets. (Closed)
|
Created:
8 years, 4 months ago by Jorge Lucangeli Obes Modified:
8 years, 4 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, agl, jln+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdd ARM syscalls to syscall sets.
BUG=141157
TEST=Build and boot on daisy.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152164
Patch Set 1 #
Total comments: 12
Patch Set 2 : Address jln's comments. #Patch Set 3 : Add EABI guard and comment. #
Messages
Total messages: 16 (0 generated)
PTAL. CL adding all syscalls to ARM header coming up next.
I didn't see some system calls that I remember from arm. I'm also pretty sure arm has a socketcall, doesn't it? Perhaps it's because we're targeting EABI only ? If that's the case, can you make sure you document this on line 158 and also add EABI line 28 for defining SECCOMP_BPF ? https://chromiumcodereview.appspot.com/10830348/diff/1/content/common/sandbox... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://chromiumcodereview.appspot.com/10830348/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:156: #if defined(__i386__) || defined(__x86_64__) || defined(__arm__) I think we don't need this line at all now. Hurray! We only need a #if defined (SECCOMP_BPF) around the anonymous namespace section, which we already have. https://chromiumcodereview.appspot.com/10830348/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:158: // The functions below cover all existing x86_64 and i386 system calls. You now have the privilege to add an architecture here. https://chromiumcodereview.appspot.com/10830348/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:324: #else // defined(__arm__) Please, make this a #elif. https://chromiumcodereview.appspot.com/10830348/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:614: case __NR_mmap: // TODO(jln): to restrict flags. I'm pretty certain arm has mmap(). Maybe it's called old_mmap now ? Did you explicitly exclude obsolete syscalls? https://chromiumcodereview.appspot.com/10830348/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:741: #if defined(__i386__) || defined(__x86_64__) Feel free to collapse these in one big #if defined. https://chromiumcodereview.appspot.com/10830348/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:1095: bool IsArmPciConfig(int sysno) { I really want to keep the property that the baseline explicitly covers all system calls. Please add these to IsBaselinePolicyWatched_x86_64
https://chromiumcodereview.appspot.com/10830348/diff/1/content/common/sandbox... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://chromiumcodereview.appspot.com/10830348/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:156: #if defined(__i386__) || defined(__x86_64__) || defined(__arm__) On 2012/08/16 20:45:08, Julien Tinnes wrote: > I think we don't need this line at all now. Hurray! > > We only need a #if defined (SECCOMP_BPF) around the anonymous namespace section, > which we already have. Done. https://chromiumcodereview.appspot.com/10830348/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:158: // The functions below cover all existing x86_64 and i386 system calls. On 2012/08/16 20:45:08, Julien Tinnes wrote: > You now have the privilege to add an architecture here. Done. https://chromiumcodereview.appspot.com/10830348/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:324: #else // defined(__arm__) On 2012/08/16 20:45:08, Julien Tinnes wrote: > Please, make this a #elif. Done. https://chromiumcodereview.appspot.com/10830348/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:614: case __NR_mmap: // TODO(jln): to restrict flags. On 2012/08/16 20:45:08, Julien Tinnes wrote: > I'm pretty certain arm has mmap(). Maybe it's called old_mmap now ? Did you > explicitly exclude obsolete syscalls? It's explicitly undefined in unistd.h and marked OBSOLETE in calls.S, which automatically returns ENOSYS. https://chromiumcodereview.appspot.com/10830348/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:741: #if defined(__i386__) || defined(__x86_64__) On 2012/08/16 20:45:08, Julien Tinnes wrote: > Feel free to collapse these in one big #if defined. Done. https://chromiumcodereview.appspot.com/10830348/diff/1/content/common/sandbox... content/common/sandbox_seccomp_bpf_linux.cc:1095: bool IsArmPciConfig(int sysno) { On 2012/08/16 20:45:08, Julien Tinnes wrote: > I really want to keep the property that the baseline explicitly covers all > system calls. > > Please add these to IsBaselinePolicyWatched_x86_64 I don't think these exist in x86/64. I don't understand the suggestion. Do you want to have a Baseline valid for ARM as well?
On 2012/08/16 21:32:07, Jorge Lucangeli Obes wrote: Did you forget to upload the new CL ? > I don't think these exist in x86/64. I don't understand the suggestion. Do you > want to have a Baseline valid for ARM as well? The current baseline is called x86_64 because it has been tested on this architecture alone. But compilation-wise it should work everywhere. Moreover, I would like the current baseline to explicitly do "something" with all system calls. So I would just add those new function to IsBaselinePolicyWatched_x86_64. I agree, the naming isn't great, if you want you can rename these functions to remove the x86_64 (which initially indicated a compilation requirement) and just add a comment that it's not ready yet for use on i386 or arm.
On 2012/08/16 21:51:58, Julien Tinnes wrote: > On 2012/08/16 21:32:07, Jorge Lucangeli Obes wrote: > > Did you forget to upload the new CL ? I was waiting for follow-ups to the questions, will do shortly. > > I don't think these exist in x86/64. I don't understand the suggestion. Do you > > want to have a Baseline valid for ARM as well? > > The current baseline is called x86_64 because it has been tested on this > architecture alone. But compilation-wise it should work everywhere. Moreover, I > would like the current baseline to explicitly do "something" with all system > calls. So I would just add those new function to IsBaselinePolicyWatched_x86_64. > > I agree, the naming isn't great, if you want you can rename these functions to > remove the x86_64 (which initially indicated a compilation requirement) and just > add a comment that it's not ready yet for use on i386 or arm.
Uploaded.
On 2012/08/16 22:14:30, Jorge Lucangeli Obes wrote: > Uploaded. You may have missed the top of my first review email ? I should have made those comments inline.
On 2012/08/16 20:45:08, Julien Tinnes wrote: > I didn't see some system calls that I remember from arm. I'm also pretty sure > arm has a socketcall, doesn't it? No socketcall in EABI, same as mmap. > Perhaps it's because we're targeting EABI only ? If that's the case, can you > make sure you document this on line 158 and also add EABI line 28 for defining > SECCOMP_BPF ? We're using the EABI define to guard the seccomp-bpf sandbox code (what lives in /sandbox), a failed inclusion would blow up there befor e it blows up here, but we can be extra careful and do it here as well. > https://chromiumcodereview.appspot.com/10830348/diff/1/content/common/sandbox... > File content/common/sandbox_seccomp_bpf_linux.cc (right): > > https://chromiumcodereview.appspot.com/10830348/diff/1/content/common/sandbox... > content/common/sandbox_seccomp_bpf_linux.cc:156: #if defined(__i386__) || > defined(__x86_64__) || defined(__arm__) > I think we don't need this line at all now. Hurray! > > We only need a #if defined (SECCOMP_BPF) around the anonymous namespace section, > which we already have. > > https://chromiumcodereview.appspot.com/10830348/diff/1/content/common/sandbox... > content/common/sandbox_seccomp_bpf_linux.cc:158: // The functions below cover > all existing x86_64 and i386 system calls. > You now have the privilege to add an architecture here. > > https://chromiumcodereview.appspot.com/10830348/diff/1/content/common/sandbox... > content/common/sandbox_seccomp_bpf_linux.cc:324: #else // defined(__arm__) > Please, make this a #elif. > > https://chromiumcodereview.appspot.com/10830348/diff/1/content/common/sandbox... > content/common/sandbox_seccomp_bpf_linux.cc:614: case __NR_mmap: // TODO(jln): > to restrict flags. > I'm pretty certain arm has mmap(). Maybe it's called old_mmap now ? Did you > explicitly exclude obsolete syscalls? > > https://chromiumcodereview.appspot.com/10830348/diff/1/content/common/sandbox... > content/common/sandbox_seccomp_bpf_linux.cc:741: #if defined(__i386__) || > defined(__x86_64__) > Feel free to collapse these in one big #if defined. > > https://chromiumcodereview.appspot.com/10830348/diff/1/content/common/sandbox... > content/common/sandbox_seccomp_bpf_linux.cc:1095: bool IsArmPciConfig(int sysno) > { > I really want to keep the property that the baseline explicitly covers all > system calls. > > Please add these to IsBaselinePolicyWatched_x86_64
On 2012/08/16 22:20:43, Jorge Lucangeli Obes wrote: > On 2012/08/16 20:45:08, Julien Tinnes wrote: > > I didn't see some system calls that I remember from arm. I'm also pretty sure > > arm has a socketcall, doesn't it? > > No socketcall in EABI, same as mmap. > > > Perhaps it's because we're targeting EABI only ? If that's the case, can you > > make sure you document this on line 158 and also add EABI line 28 for defining > > SECCOMP_BPF ? > > We're using the EABI define to guard the seccomp-bpf sandbox code (what lives in > /sandbox), a failed inclusion would blow up there befor e it blows up here, but > we can be extra careful and do it here as well. Uploaded. PTAL. Thanks!
On 2012/08/16 22:20:43, Jorge Lucangeli Obes wrote: > On 2012/08/16 20:45:08, Julien Tinnes wrote: > > I didn't see some system calls that I remember from arm. I'm also pretty sure > > arm has a socketcall, doesn't it? > > No socketcall in EABI, same as mmap. > > > Perhaps it's because we're targeting EABI only ? If that's the case, can you > > make sure you document this on line 158 and also add EABI line 28 for defining > > SECCOMP_BPF ? > > We're using the EABI define to guard the seccomp-bpf sandbox code (what lives in > /sandbox), a failed inclusion would blow up there befor e it blows up here, but > we can be extra careful and do it here as well. Yeah exactly, I would prefer it not to "blow up" in content/ at all. On non EABI or other unsupported architectures, I want it to work with no sandbox.
lgtm But you'll probably need to land a arm_linux_syscalls.h update before you can land this.
On 2012/08/16 22:34:15, Julien Tinnes wrote: > lgtm > > But you'll probably need to land a arm_linux_syscalls.h update before you can > land this. We only need __ARM_NR_cmpxchg to compile inside the CrOS chroot, and that has been added in this CL.
On 2012/08/16 22:36:06, Jorge Lucangeli Obes wrote: > On 2012/08/16 22:34:15, Julien Tinnes wrote: > > lgtm > > > > But you'll probably need to land a arm_linux_syscalls.h update before you can > > land this. > > We only need __ARM_NR_cmpxchg to compile inside the CrOS chroot, and that has > been added in this CL. And Android is not blowing up ? That would be nice!
On 2012/08/16 22:48:17, Julien Tinnes wrote: > On 2012/08/16 22:36:06, Jorge Lucangeli Obes wrote: > > On 2012/08/16 22:34:15, Julien Tinnes wrote: > > > lgtm > > > > > > But you'll probably need to land a arm_linux_syscalls.h update before you > can > > > land this. > > > > We only need __ARM_NR_cmpxchg to compile inside the CrOS chroot, and that has > > been added in this CL. > > And Android is not blowing up ? That would be nice! Trybots to the rescue!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jorgelo@chromium.org/10830348/6003
Change committed as 152164 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
