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

["Andreas Färber" <afaerber@suse.de>]

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 <otubo@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...

> +
>  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.

> +    }
> +#endif

This is adding code before the variable declaration block, please move
to after.

> +
>      int i;
>      int snapshot, linux_boot;
>      const char *icount_option = NULL;

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg