Re: [Qemu-devel] [RFC] [PATCH 2/2] Adding basic calls to libseccomp in vl.c

[Eduardo Otubo]

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 <address@hidden>
> > ---
> >  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 
address@hidden