On Tue, Oct 23, 2012 at 11:10:58AM -0400, Corey Bryant wrote: > > > On 10/23/2012 01:55 AM, Eduardo Otubo wrote: > >This patch includes a second whitelist right before the main loop. It's > >a smaller and more restricted whitelist, excluding execve() among many > >others. > > > >v2: * ctx changed to main_loop_ctx > > * seccomp_on now inside ifdef > > * open syscall added to the main_loop whitelist > > > >Signed-off-by: Eduardo Otubo <ot...@linux.vnet.ibm.com> > >--- > > qemu-seccomp.c | 99 > > ++++++++++++++++++++++++++++++++++++++++++++++++++++------ > > qemu-seccomp.h | 7 ++++- > > vl.c | 21 +++++++++++-- > > 3 files changed, 114 insertions(+), 13 deletions(-) > > > >diff --git a/qemu-seccomp.c b/qemu-seccomp.c > >index a7b33e2..033cfad 100644 > >--- a/qemu-seccomp.c > >+++ b/qemu-seccomp.c > >@@ -13,6 +13,7 @@ > > * GNU GPL, version 2 or (at your option) any later version. > > */ > > #include <stdio.h> > >+#include <stdlib.h> > > #include <seccomp.h> > > #include "qemu-seccomp.h" > > > >@@ -21,7 +22,7 @@ struct QemuSeccompSyscall { > > uint8_t priority; > > }; > > > >-static const struct QemuSeccompSyscall seccomp_whitelist[] = { > >+static const struct QemuSeccompSyscall seccomp_whitelist_init[] = { > > { SCMP_SYS(timer_settime), 255 }, > > { SCMP_SYS(timer_gettime), 254 }, > > { SCMP_SYS(futex), 253 }, > >@@ -121,27 +122,107 @@ static const struct QemuSeccompSyscall > >seccomp_whitelist[] = { > > { SCMP_SYS(rt_sigtimedwait), 242 } > > }; > > > >-int seccomp_start(void) > >+static const struct QemuSeccompSyscall seccomp_whitelist_main_loop[] = { > >+ { SCMP_SYS(timer_settime), 255 }, > >+ { SCMP_SYS(timer_gettime), 254 }, > >+ { SCMP_SYS(futex), 253 }, > >+ { SCMP_SYS(select), 252 }, > >+ { SCMP_SYS(recvfrom), 251 }, > >+ { SCMP_SYS(sendto), 250 }, > >+ { SCMP_SYS(read), 249 }, > >+ { SCMP_SYS(brk), 248 }, > >+ { SCMP_SYS(mmap), 247 }, > >+ { SCMP_SYS(open), 247 }, > >+#if defined(__i386__) > >+ { SCMP_SYS(fcntl64), 245 }, > >+ { SCMP_SYS(fstat64), 245 }, > >+ { SCMP_SYS(stat64), 245 }, > >+ { SCMP_SYS(getgid32), 245 }, > >+ { SCMP_SYS(getegid32), 245 }, > >+ { SCMP_SYS(getuid32), 245 }, > >+ { SCMP_SYS(geteuid32), 245 }, > >+ { SCMP_SYS(sigreturn), 245 }, > >+ { SCMP_SYS(_newselect), 245 }, > >+ { SCMP_SYS(_llseek), 245 }, > >+ { SCMP_SYS(mmap2), 245}, > >+ { SCMP_SYS(sigprocmask), 245 }, > >+#endif > >+ { SCMP_SYS(exit), 245 }, > >+ { SCMP_SYS(timer_delete), 245 }, > >+ { SCMP_SYS(exit_group), 245 }, > >+ { SCMP_SYS(rt_sigreturn), 245 }, > >+ { SCMP_SYS(madvise), 245 }, > >+ { SCMP_SYS(write), 244 }, > >+ { SCMP_SYS(fcntl), 243 }, > >+ { SCMP_SYS(tgkill), 242 }, > >+ { SCMP_SYS(rt_sigaction), 242 }, > >+ { SCMP_SYS(pipe2), 242 }, > >+ { SCMP_SYS(munmap), 242 }, > >+ { SCMP_SYS(mremap), 242 }, > >+ { SCMP_SYS(getsockname), 242 }, > >+ { SCMP_SYS(getpeername), 242 }, > >+ { SCMP_SYS(close), 242 }, > >+ { SCMP_SYS(accept4), 242 }, > >+ { SCMP_SYS(eventfd2), 242 }, > >+ { SCMP_SYS(recvmsg), 242 }, > >+ { SCMP_SYS(ioctl), 242 }, > >+ { SCMP_SYS(rt_sigprocmask), 242 } > >+}; > >+ > >+static int > >+process_whitelist(const struct QemuSeccompSyscall *whitelist, > >+ unsigned int size, scmp_filter_ctx *ctx) > > { > > int rc = 0; > >+ > > unsigned int i = 0; > >- scmp_filter_ctx ctx; > >+ > >+ for (i = 0; i < size; i++) { > >+ rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, whitelist[i].num, 0); > >+ if (rc < 0) { > >+ return -1; > >+ } > >+ > >+ rc = seccomp_syscall_priority(ctx, whitelist[i].num, > >+ whitelist[i].priority); > >+ if (rc < 0) { > >+ return -1; > >+ } > >+ } > >+ return 0; > >+} > >+ > >+int > >+seccomp_start(enum whitelist_mode mode, scmp_filter_ctx *ctx) > >+{ > >+ int rc = 0; > > > > ctx = seccomp_init(SCMP_ACT_KILL); > > Is there any reason why ctx can't be a local variable in this > function? It is allocated and freed on each entry and exit in this > function. >
It does make sense. I'll make it local in the next version, thanks. > > if (ctx == NULL) { > >+ rc = -1; > > goto seccomp_return; > > } > > > >- for (i = 0; i < ARRAY_SIZE(seccomp_whitelist); i++) { > >- rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, > >seccomp_whitelist[i].num, 0); > >- if (rc < 0) { > >+ switch (mode) { > >+ case INIT: > >+ if (process_whitelist > >+ (seccomp_whitelist_init, > >+ ARRAY_SIZE(seccomp_whitelist_init), ctx) < 0) { > >+ rc = -1; > > goto seccomp_return; > > } > >- rc = seccomp_syscall_priority(ctx, seccomp_whitelist[i].num, > >- seccomp_whitelist[i].priority); > >- if (rc < 0) { > >+ break; > >+ case MAIN_LOOP: > >+ if (process_whitelist > >+ (seccomp_whitelist_main_loop, > >+ ARRAY_SIZE(seccomp_whitelist_main_loop), ctx) < 0) { > >+ rc = -1; > > goto seccomp_return; > > } > >+ break; > >+ default: > >+ rc = -1; > >+ goto seccomp_return; > > } > > > > rc = seccomp_load(ctx); > >diff --git a/qemu-seccomp.h b/qemu-seccomp.h > >index b2fc3f8..1c97978 100644 > >--- a/qemu-seccomp.h > >+++ b/qemu-seccomp.h > >@@ -18,5 +18,10 @@ > > #include <seccomp.h> > > #include "osdep.h" > > > >-int seccomp_start(void); > >+enum whitelist_mode { > >+ INIT = 0, > >+ MAIN_LOOP = 1, > >+}; > >+ > >+int seccomp_start(enum whitelist_mode mode, scmp_filter_ctx *ctx); > > #endif > >diff --git a/vl.c b/vl.c > >index bec68cd..d50018f 100644 > >--- a/vl.c > >+++ b/vl.c > >@@ -774,10 +774,11 @@ static int bt_parse(const char *opt) > > return 1; > > } > > > >-static int install_seccomp_filters(void) > >+static int > >+install_seccomp_filters(enum whitelist_mode mode, scmp_filter_ctx *ctx) > > { > > #ifdef CONFIG_SECCOMP > >- if (seccomp_start() < 0) { > >+ if (seccomp_start(mode, ctx) < 0) { > > qerror_report(ERROR_CLASS_GENERIC_ERROR, > > "failed to install seccomp syscall filter in the kernel"); > > I heard from Luiz Capitulino on one of my patches that > qerror_report() is deprecated. So you'll want to update this. Luiz Capitulino commented on the IRC that fprintf(stderr) is fine for vl.c. I'll use that. > > > return -1; > >@@ -2407,6 +2408,10 @@ int main(int argc, char **argv, char **envp) > > const char *trace_events = NULL; > > const char *trace_file = NULL; > > > >+#ifdef CONFIG_SECCOMP > >+ scmp_filter_ctx main_loop_ctx; > >+#endif > >+ > > atexit(qemu_run_exit_notifiers); > > error_set_progname(argv[0]); > > > >@@ -3330,11 +3335,13 @@ int main(int argc, char **argv, char **envp) > > } > > > > /* We should install seccomp filters even if -sandbox on is not used. > > */ > >+#ifdef CONFIG_SECCOMP > > if (seccomp_on) { > >- if (install_seccomp_filters() < 0) { > >+ if (install_seccomp_filters(INIT, &main_loop_ctx) < 0) { > > I don't think the variable name "main_loop_ctx" makes sense here. > Should the name be more generic since it's used wherever a seccomp > filter is installed? since the ctx variable now is local in the function install_seccomp_filters() I'll remove this reference in the next version. Thanks for all the comments :) -- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group