On Fri, May 04, 2012 at 11:59:00PM +0200, Andreas Färber wrote:
> Am 04.05.2012 21:08, schrieb Eduardo Otubo:
> > I added a syscall struct using priority levels as described in the
> > libseccomp
> > man page. The priority numbers are based to the frequency they appear in a
> > sample strace from a regular qemu guest run under libvirt.
> >
> > Libseccomp generates linear BPF code to filter system calls, those rules are
> > read one after another. The priority system places the most common rules
> > first
> > in order to reduce the overhead when processing them.
> >
> > Also, since this is just a first RFC, the whitelist is a little raw. We
> > might
> > need your help to improve, test and fine tune the set of system calls.
> >
> > Signed-off-by: Eduardo Otubo <ot...@linux.vnet.ibm.com>
> > ---
> > vl.c | 81
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 81 insertions(+)
> >
> > diff --git a/vl.c b/vl.c
> > index ae91a8a..e23838b 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -63,6 +63,9 @@
> >
> > #include <linux/ppdev.h>
> > #include <linux/parport.h>
> > +#ifdef CONFIG_LIBSECCOMP
> > +#include <seccomp.h>
> > +#endif
> > #endif
> > #ifdef __sun__
> > #include <sys/stat.h>
> > @@ -175,6 +178,8 @@ int main(int argc, char **argv)
> >
> > #define MAX_VIRTIO_CONSOLES 1
> >
> > +static int seccomp_start(void);
>
> You haven't tested this without libseccomp apparently?
>
> Other than that mostly some Coding Style issues...
Actually I did run the scripts/checkpatch.pl, and there were no errors. But
I'll fix those issues for my next version. Thanks for pointing them out :)
>
> > +
> > static const char *data_dir;
> > const char *bios_name = NULL;
> > enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
> > @@ -313,6 +318,75 @@ static int default_driver_check(QemuOpts *opts, void
> > *opaque)
> > return 0;
> > }
> >
> > +#ifdef CONFIG_LIBSECCOMP
> > +struct qemu_seccomp_syscall {
>
> Struct names are expected to be CamelCase.
>
> > + int32_t num;
> > + uint8_t priority;
> > +};
> > +
> > +static struct qemu_seccomp_syscall seccomp_whitelist[] = {
> > + {SCMP_SYS(timer_settime), 255},
>
> Spaces inside braces please.
>
> > + {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(clone), 247},
> > + {SCMP_SYS(mmap), 247},
> > + {SCMP_SYS(mprotect), 246},
> > + {SCMP_SYS(rt_sigprocmask), 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(fdatasync), 242},
> > + {SCMP_SYS(close), 242}
> > +};
> > +
> > +#define seccomp_whitelist_count \
> > + (sizeof(seccomp_whitelist)/sizeof(seccomp_whitelist[0]))
>
> Please just use the ARRAY_SIZE() macro inline.
>
> > +
> > +int seccomp_start(void)
> > +{
> > + int rc = 0;
> > + unsigned int i = 0;
> > +
> > + rc = seccomp_init(SCMP_ACT_KILL);
> > + if (rc < 0) {
> > + goto seccomp_return;
> > + }
> > +
> > + for (i = 0; i < seccomp_whitelist_count; i++) {
> > + rc = seccomp_rule_add(SCMP_ACT_ALLOW, seccomp_whitelist[i].num, 0);
> > + if (rc < 0) {
> > + goto seccomp_return;
> > + }
> > + rc = seccomp_syscall_priority(seccomp_whitelist[i].num,
> > + seccomp_whitelist[i].priority);
> > + if (rc < 0) {
> > + goto seccomp_return;
> > + }
> > + }
> > +
> > + rc = seccomp_load();
> > +
> > + seccomp_return:
> > + seccomp_release();
> > + if (rc < 0) {
> > + fprintf(stderr,
> > + "ERROR: failed to configure the seccomp syscall filter in
> > the kernel");
>
> \n missing.
>
> > + }
> > + return rc;
> > +}
> > +#endif
> > +
> > /***********************************************************/
> > /* QEMU state */
> >
> > @@ -2257,6 +2331,13 @@ int qemu_init_main_loop(void)
> >
> > int main(int argc, char **argv, char **envp)
> > {
> > +
> > +#ifdef CONFIG_LIBSECCOMP
> > + if (seccomp_start() < 0) {
> > + exit(1);
>
> This is inconsistent: Either exit() within seccomp_start() where you
> print the error message, or move the fprintf() here.
>
Agreed.
> > + }
> > +#endif
>
> This is adding code before the variable declaration block, please move
> to after.
Agreed.
Thanks for the comments.
--
Eduardo Otubo
Software Engineer
Linux Technology Center
IBM Systems & Technology Group
Mobile: +55 19 8135 0885
eot...@linux.vnet.ibm.com
