Closed Bug 907002 Opened 7 years ago Closed 7 years ago

seccomp sandbox needs to allow restart_syscall, to debug failures

Categories

(Core :: Security, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: jld, Assigned: jld)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch seccomp-restart-syscall-hg0.diff (obsolete) — Splinter Review
If I attach to a seccomp'ed b2g child process with gdb while it's in a syscall (e.g., if it was idle), then as soon as I `continue`, it receives SIGSYS for attempting system call 0, which is __NR_restart_syscall.  This makes it difficult (and confusing) to try to debug seccomp failures.

However, I don't know whether globally allowing syscall 0, as I've done in the attached patch, could be used to indirect to arbitrary syscalls and completely bypass seccomp.  That would be bad.
Attachment #792573 - Flags: review?(gdestuynder)
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Comment on attachment 792573 [details] [diff] [review]
seccomp-restart-syscall-hg0.diff

Review of attachment 792573 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, it's ok to have restart_syscall. It's called internally (i.e. the kernel calls it).

::: security/sandbox/seccomp_filter.h
@@ +81,5 @@
>    ALLOW_SYSCALL(getpriority), \
>    ALLOW_SYSCALL(setpriority), \
>    ALLOW_SYSCALL(sigprocmask), \
> +  /* Seen when resuming from gdb; does this need more restriction? */ \
> +  ALLOW_SYSCALL(restart_syscall), \

I would put it here instead:

  ALLOW_SYSCALL(setpriority), \
  ALLOW_SYSCALL(sigprocmask), \
  /* Always last and always OK calls */ \
  SECCOMP_WHITELIST_ADD \
  /* restart_syscall is called internally, generally when debugging */ \
+  ALLOW_SYSCALL(restart_syscall), \
  ALLOW_SYSCALL(exit_group), \
  ALLOW_SYSCALL(exit)

This is because I believe this system call can stay - and because it shouln't be called often, it's at the bottom of the allowed list, but not in the "should remove" list.
Attachment #792573 - Flags: review?(gdestuynder) → review+
Attachment #792573 - Attachment is obsolete: true
Attachment #793758 - Flags: review?(gdestuynder)
Attachment #793758 - Flags: checkin?(gdestuynder)
Comment on attachment 793758 [details] [diff] [review]
seccomp-restart-syscall-hg1.diff

Review of attachment 793758 [details] [diff] [review]:
-----------------------------------------------------------------

Apparently I'm doing bug flags wrong, or at least suboptimally.  Carrying over r+ from attachment 792573 [details] [diff] [review] (r=kang).  Also this is pretty much comment #1 reformatted as a patch.
Attachment #793758 - Flags: review?(gdestuynder)
Attachment #793758 - Flags: review+
Attachment #793758 - Flags: checkin?(gdestuynder)
sorry - I'm away this week hence the delay. i was going to do it regardless, but thinking about it, checkin-needed is probably better indeed :) if nothing happened when im back around or if this becomes a pain let me know i'll *try* to get it landed (ive intermittent internets)
https://hg.mozilla.org/mozilla-central/rev/13ecac047855
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.