[Qemu-devel] [PATCHv5 0/4] Sandboxing Qemu guests with Libseccomp

[Qemu-devel] [PATCHv5 0/4] Sandboxing Qemu guests with Libseccomp

[Eduardo Otubo]

Hello all,

This patch is an effort to sandbox Qemu guests using Libseccomp[0]. The patches
that follows are pretty simple and straightforward. I added the correct options
and checks to the configure script and the basic calls to libseccomp in the
main loop at vl.c. Details of each one are in the emails of the patch set.

This support limits the system call footprint of the entire QEMU process to a
limited set of syscalls, those that we know QEMU uses. The idea is to limit the
allowable syscalls, therefore limiting the impact that an attacked guest could
have on the host system.

It's important to note that the libseccomp itself needs the seccomp mode 2
feature in the kernel, which is only available in kernel versions older (or
equal) than 3.5-rc1.

v2: Files separated in qemu-seccomp.c and qemu-seccomp.h for a cleaner
    implementation. The development was tested with the 3.5-rc1 kernel.

v3: As we discussed in previous emails in this mailing list, this feature is
    not supposed to replace existing security feature, but add another layer to
    the whole. The whitelist should contain all the syscalls QEMU needs. And as
    stated by Will Drewry's commit message[1]: "Filter programs will be inherited
    across fork/clone and execve.", the same white list should be passed along from
    the father process to the child, then execve() shouldn't be a problem. Note
    that there's a feature PR_SET_NO_NEW_PRIVS in seccomp mode 2 in the kernel,
    this prevents processes from gaining privileges on execve. For example, this
    will prevent qemu (if running unprivileged) from executing setuid programs[2].

v4: Introducing "debug" mode on libseccomp support. The "debug" mode will set
    the flag SCMP_ACT_TRAP when calling seccomp_start(). It will verbosely
    print a message to the stderr in the form "seccomp: illegal system call
    execution trapped: XXX" and resume the execution. This is really just used as
    debug mode, it helps users and developers to full fill the whitelist.

v5: Libseccomp release 1.0.0[3]: The API now is context aware and it breaks the
    compatibility with older versions. I updated all the functions that differs
    from one version to another.

As always, comments are more than welcome.

Regards,

[0] - http://sourceforge.net/projects/libseccomp/
[1] - http://git.kernel.org/?p=linux/kernel/git/next/linux-next.git;a=commit;h=e2cfabdfd075648216f99c2c03821cf3f47c1727
[2] - https://lkml.org/lkml/2012/4/12/457
[3] - http://sourceforge.net/mailarchive/forum.php?thread_name=1633205.5jr3eG7nQ5%40sifl&forum_name=libseccomp-discuss


Eduardo Otubo (4):
  Adding support for libseccomp in configure and Makefile
  Adding qemu-seccomp.[ch]
  Adding qemu-seccomp-debug.[ch]
  Adding seccomp calls to vl.c

 Makefile.objs        |   10 ++++
 configure            |   34 ++++++++++++
 qemu-seccomp-debug.c |   95 ++++++++++++++++++++++++++++++++++
 qemu-seccomp-debug.h |   38 ++++++++++++++
 qemu-seccomp.c       |  139 ++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-seccomp.h       |   22 ++++++++
 vl.c                 |   31 +++++++++++
 7 files changed, 369 insertions(+), 0 deletions(-)
 create mode 100644 qemu-seccomp-debug.c
 create mode 100644 qemu-seccomp-debug.h
 create mode 100644 qemu-seccomp.c
 create mode 100644 qemu-seccomp.h

[Qemu-devel] [PATCHv5 1/4] Adding support for libseccomp in configure and Makefile

[Eduardo Otubo]

Adding basic options to the configure script to use libseccomp or not.
The default is set to 'no'. If the flag --enable-libseccomp is used, the
script will check for its existence using pkg-config.

v2:
 * As I removed all the code related to seccomp from vl.c, I created
   qemu-seccomp.[ch].
 * Also making the configure script to add the specific line to
   Makefile.obj in order to compile with appropriate support to seccomp.

v3:
 * Removing the line from Makefile.obj and adding it to Makefile.objs.
 * Marking libseccomp default option to 'yes' in the configure script.

v4:
 * Now two new options added:

     --enable-seccomp-debug
     --disable-seccomp-debug

   Enabling debug will cause libseccomp to be configured with
   SCMP_ACT_TRAP. This will help users/developers to catch system calls
   that were not previously whitelisted.

Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
---
 Makefile.objs |   10 ++++++++++
 configure     |   34 ++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/Makefile.objs b/Makefile.objs
index 5ebbcfa..eb4efa3 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -96,6 +96,16 @@ common-obj-y += qemu-timer.o qemu-timer-common.o
 common-obj-$(CONFIG_SLIRP) += slirp/
 
 ######################################################################
+# libseccomp
+ifeq ($(CONFIG_SECCOMP),y)
+common-obj-y += qemu-seccomp.o
+endif
+
+ifeq ($(CONFIG_SECCOMP_DEBUG),y)
+common-obj-y += qemu-seccomp-debug.o
+endif
+
+######################################################################
 # libuser
 
 user-obj-y =
diff --git a/configure b/configure
index 027a718..c12629b 100755
--- a/configure
+++ b/configure
@@ -195,6 +195,8 @@ zlib="yes"
 guest_agent="yes"
 libiscsi=""
 coroutine=""
+seccomp="yes"
+seccomp_debug="no"
 
 # parse CC options first
 for opt do
@@ -824,6 +826,14 @@ for opt do
   ;;
   --disable-guest-agent) guest_agent="no"
   ;;
+  --enable-seccomp-debug) seccomp_debug="yes"
+  ;;
+  --disable-seccomp-debug) seccomp_debug="no"
+  ;;
+  --enable-seccomp) seccomp="yes"
+  ;;
+  --disable-seccomp) seccomp="no"
+  ;;
   *) echo "ERROR: unknown option $opt"; show_help="yes"
   ;;
   esac
@@ -1110,6 +1120,10 @@ echo "  --disable-usb-redir      disable usb network redirection support"
 echo "  --enable-usb-redir       enable usb network redirection support"
 echo "  --disable-guest-agent    disable building of the QEMU Guest Agent"
 echo "  --enable-guest-agent     enable building of the QEMU Guest Agent"
+echo "  --disable-seccomp-debug  disable seccomp debug support"
+echo "  --enable-seccomp-debug   enables seccomp debug support"
+echo "  --disable-seccomp        disable seccomp support"
+echo "  --enable-seccomp         enables seccomp support"
 echo "  --with-coroutine=BACKEND coroutine backend. Supported options:"
 echo "                           gthread, ucontext, sigaltstack, windows"
 echo ""
@@ -1372,6 +1386,16 @@ EOF
 fi
 
 ##########################################
+# libseccomp check
+
+if test "$seccomp" = "yes" ; then
+    if $pkg_config libseccomp --modversion >/dev/null 2>&1; then
+        LIBS=`$pkg_config --libs libseccomp`
+    else
+        feature_not_found "libseccomp"
+    fi
+fi
+##########################################
 # xen probe
 
 if test "$xen" != "no" ; then
@@ -3103,6 +3127,8 @@ echo "usb net redir     $usb_redir"
 echo "OpenGL support    $opengl"
 echo "libiscsi support  $libiscsi"
 echo "build guest agent $guest_agent"
+echo "seccomp support   $seccomp"
+echo "seccomp debug     $seccomp_debug"
 echo "coroutine backend $coroutine_backend"
 
 if test "$sdl_too_old" = "yes"; then
@@ -3401,6 +3427,14 @@ if test "$libiscsi" = "yes" ; then
   echo "CONFIG_LIBISCSI=y" >> $config_host_mak
 fi
 
+if test "$seccomp" = "yes"; then
+  echo "CONFIG_SECCOMP=y" >> $config_host_mak
+fi
+
+if test "$seccomp_debug" = "yes"; then
+  echo "CONFIG_SECCOMP_DEBUG=y" >> $config_host_mak
+fi
+
 # XXX: suppress that
 if [ "$bsd" = "yes" ] ; then
   echo "CONFIG_BSD=y" >> $config_host_mak
-- 
1.7.1

[Qemu-devel] [PATCHv5 2/4] Adding qemu-seccomp.[ch]

[Eduardo Otubo]

v1:
 * 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.

v2:
 * Fixed some style issues
 * Removed code from vl.c and created qemu-seccomp.[ch]
 * Now using ARRAY_SIZE macro
 * Added more syscalls without priority/frequency set yet

v3:
 * Adding copyright and license information
 * Replacing seccomp_whitelist_count just by ARRAY_SIZE
 * Adding header protection to qemu-seccomp.h
 * Moving QemuSeccompSyscall definition to qemu-seccomp.c
 * Negative return from seccomp_start is fatal now.
 * Adding open() and execve() to the whitelis

v4:
 * Tests revealed a bigger set of syscalls.
 * seccomp_start() now has an argument to set the mode according to the
   configure option trap or kill.

v5:
 * Tests on x86_64 required a new specific set of system calls.
 * libseccomp release 1.0.0: part of the API have changed in this last
   release, had to adapt to the new function signatures.

Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
---
 qemu-seccomp.c |  139 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-seccomp.h |   22 +++++++++
 2 files changed, 161 insertions(+), 0 deletions(-)
 create mode 100644 qemu-seccomp.c
 create mode 100644 qemu-seccomp.h

diff --git a/qemu-seccomp.c b/qemu-seccomp.c
new file mode 100644
index 0000000..0e6efa4
--- /dev/null
+++ b/qemu-seccomp.c
@@ -0,0 +1,139 @@
+/*
+ * QEMU seccomp mode 2 support with libseccomp
+ *
+ * Copyright IBM, Corp. 2012
+ *
+ * Authors:
+ *  Eduardo Otubo    <eotubo@br.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ * Contributions after 2012-01-13 are licensed under the terms of the
+ * GNU GPL, version 2 or (at your option) any later version.
+ */
+#include <stdio.h>
+#include <seccomp.h>
+#include "qemu-seccomp.h"
+
+struct QemuSeccompSyscall {
+    int32_t num;
+    uint8_t priority;
+};
+
+static const struct QemuSeccompSyscall seccomp_whitelist[] = {
+    { 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(clone), 247 },
+    { SCMP_SYS(mmap), 247 },
+    { SCMP_SYS(mprotect), 246 },
+    { SCMP_SYS(execve), 245 },
+    { SCMP_SYS(open), 245 },
+    { SCMP_SYS(ioctl), 245 },
+    { SCMP_SYS(recvmsg), 245 },
+    { SCMP_SYS(sendmsg), 245 },
+    { SCMP_SYS(accept), 245 },
+    { SCMP_SYS(connect), 245 },
+    { SCMP_SYS(gettimeofday), 245 },
+    { SCMP_SYS(readlink), 245 },
+    { SCMP_SYS(access), 245 },
+    { SCMP_SYS(prctl), 245 },
+    { SCMP_SYS(signalfd), 245 },
+#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 },
+#elif defined(__x86_64__)
+    { SCMP_SYS(sched_getparam), 245},
+    { SCMP_SYS(sched_getscheduler), 245},
+    { SCMP_SYS(fstat), 245},
+    { SCMP_SYS(clock_getres), 245},
+    { SCMP_SYS(sched_get_priority_min), 245},
+    { SCMP_SYS(sched_get_priority_max), 245},
+    { SCMP_SYS(stat), 245},
+    { SCMP_SYS(socket), 245},
+    { SCMP_SYS(setsockopt), 245},
+#endif
+    { SCMP_SYS(eventfd2), 245 },
+    { SCMP_SYS(dup), 245 },
+    { SCMP_SYS(gettid), 245 },
+    { SCMP_SYS(timer_create), 245 },
+    { SCMP_SYS(exit), 245 },
+    { SCMP_SYS(clock_gettime), 245 },
+    { SCMP_SYS(time), 245 },
+    { SCMP_SYS(restart_syscall), 245 },
+    { SCMP_SYS(pwrite64), 245 },
+    { SCMP_SYS(chown), 245 },
+    { SCMP_SYS(openat), 245 },
+    { SCMP_SYS(getdents), 245 },
+    { SCMP_SYS(timer_delete), 245 },
+    { SCMP_SYS(exit_group), 245 },
+    { SCMP_SYS(rt_sigreturn), 245 },
+    { SCMP_SYS(sync), 245 },
+    { SCMP_SYS(pread64), 245 },
+    { SCMP_SYS(madvise), 245 },
+    { SCMP_SYS(set_robust_list), 245 },
+    { SCMP_SYS(lseek), 245 },
+    { SCMP_SYS(pselect6), 245 },
+    { SCMP_SYS(fork), 245 },
+    { SCMP_SYS(bind), 245 },
+    { SCMP_SYS(listen), 245 },
+    { SCMP_SYS(eventfd), 245 },
+    { 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 }
+};
+
+int seccomp_start(uint32_t mode)
+{
+    int rc = 0;
+    unsigned int i = 0;
+    scmp_filter_ctx ctx;
+
+    ctx = seccomp_init(mode);
+    if (ctx == NULL) {
+        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) {
+            goto seccomp_return;
+        }
+        rc = seccomp_syscall_priority(ctx, seccomp_whitelist[i].num,
+                                      seccomp_whitelist[i].priority);
+        if (rc < 0) {
+            goto seccomp_return;
+        }
+    }
+
+    rc = seccomp_load(ctx);
+
+  seccomp_return:
+    seccomp_release(ctx);
+    return rc;
+}
diff --git a/qemu-seccomp.h b/qemu-seccomp.h
new file mode 100644
index 0000000..087333f
--- /dev/null
+++ b/qemu-seccomp.h
@@ -0,0 +1,22 @@
+/*
+ * QEMU seccomp mode 2 support with libseccomp
+ *
+ * Copyright IBM, Corp. 2012
+ *
+ * Authors:
+ *  Eduardo Otubo    <eotubo@br.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ * Contributions after 2012-01-13 are licensed under the terms of the
+ * GNU GPL, version 2 or (at your option) any later version.
+ */
+#ifndef QEMU_SECCOMP_H
+#define QEMU_SECCOMP_H
+
+#include <seccomp.h>
+#include "osdep.h"
+
+int seccomp_start(uint32_t mode);
+#endif
-- 
1.7.1

[Qemu-devel] [PATCHv5 3/4] Adding qemu-seccomp-debug.[ch]

[Eduardo Otubo]

The new 'trap' (debug) mode will capture the illegal system call before it is
executed. The feature and the implementation is based on Will Drewry's
patch - https://lkml.org/lkml/2012/4/12/449

v4:
 * New files in v4
 * If SCMP_ACT_TRAP flag used when calling seccomp_init(), the kernel will
   send a SIGSYS every time a not whitelisted syscall is called. This
   sighandler install_seccomp_syscall_debug() is installed in this mode so
   we can intercept the signal and print to the user the illegal syscall.
   The process resumes after that.
 * The behavior of the code inside a signal handler sometimes is
   unpredictable (as stated in man 7 signals). That's why I deliberately
   used write() and _exit() functions, and had the string-to-int helper
   functions as well.

Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
---
 qemu-seccomp-debug.c |   95 ++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-seccomp-debug.h |   38 ++++++++++++++++++++
 2 files changed, 133 insertions(+), 0 deletions(-)
 create mode 100644 qemu-seccomp-debug.c
 create mode 100644 qemu-seccomp-debug.h

diff --git a/qemu-seccomp-debug.c b/qemu-seccomp-debug.c
new file mode 100644
index 0000000..162c2f1
--- /dev/null
+++ b/qemu-seccomp-debug.c
@@ -0,0 +1,95 @@
+
+/*
+ * QEMU seccomp mode 2 support with libseccomp
+ * Debug system calls helper functions
+ *
+ * Copyright IBM, Corp. 2012
+ *
+ * Authors:
+ *  Eduardo Otubo    <eotubo@br.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ * Contributions after 2012-01-13 are licensed under the terms of the
+ * GNU GPL, version 2 or (at your option) any later version.
+ */
+
+#include "qemu-seccomp-debug.h"
+#include "asm-generic/unistd.h"
+
+#define safe_warn(data) write(STDERR_FILENO, (const void *) data, sizeof(data))
+
+static int count_digits(int number)
+{
+    int digits = 0;
+    while (number) {
+        number /= 10;
+        digits++;
+    }
+
+    return digits;
+}
+
+static char *sput_i(int integer, char *string)
+{
+    if (integer / 10 != 0) {
+        string = sput_i(integer / 10, string);
+    }
+    *string++ = (char) ('0' + integer % 10);
+    return string;
+}
+
+static void int_to_asc(int integer, char *string)
+{
+    *sput_i(integer, string) = '\n';
+}
+
+static void syscall_debug(int nr, siginfo_t *info, void *void_context)
+{
+    ucontext_t *ctx = (ucontext_t *) (void_context);
+    char errormsg[] = "seccomp: illegal syscall trapped: ";
+    char syscall_char[count_digits(__NR_syscalls) + 1];
+    int syscall_num = 0;
+
+    if (info->si_code != SYS_SECCOMP) {
+        return;
+    }
+    if (!ctx) {
+        return;
+    }
+    syscall_num = ctx->uc_mcontext.gregs[REG_SYSCALL];
+    if (syscall_num < 0 || syscall_num >= __NR_syscalls) {
+        if ((safe_warn("seccomp: error reading syscall from register\n") < 0)) {
+            return;
+        }
+        return;
+    }
+    int_to_asc(syscall_num, syscall_char);
+    if ((safe_warn(errormsg) < 0) || (safe_warn(syscall_char) < 0)) {
+        return;
+    }
+    return;
+}
+
+int install_seccomp_syscall_debug(void)
+{
+    struct sigaction act;
+    sigset_t mask;
+
+    memset(&act, 0, sizeof(act));
+    sigemptyset(&mask);
+    sigaddset(&mask, SIGSYS);
+
+    act.sa_sigaction = &syscall_debug;
+    act.sa_flags = SA_SIGINFO;
+    if (sigaction(SIGSYS, &act, NULL) < 0) {
+        perror("seccomp: sigaction returned with errors\n");
+        return -1;
+    }
+    if (pthread_sigmask(SIG_UNBLOCK, &mask, NULL)) {
+        perror("seccomp: sigprocmask returned with errors\n");
+        return -1;
+    }
+    return 0;
+}
diff --git a/qemu-seccomp-debug.h b/qemu-seccomp-debug.h
new file mode 100644
index 0000000..d3863d6
--- /dev/null
+++ b/qemu-seccomp-debug.h
@@ -0,0 +1,38 @@
+/*
+ * QEMU seccomp mode 2 support with libseccomp
+ * Trap system calls helper functions
+ *
+ * Copyright IBM, Corp. 2012
+ *
+ * Authors:
+ *  Eduardo Otubo    <eotubo@br.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ * Contributions after 2012-01-13 are licensed under the terms of the
+ * GNU GPL, version 2 or (at your option) any later version.
+ */
+#ifndef QEMU_SECCOMP_TRAP_H
+#define QEMU_SECCOMP_TRAP_H
+
+#include <signal.h>
+#include <string.h>
+#include <stdio.h>
+#include <unistd.h>
+
+#if defined(__i386__)
+#define REG_SYSCALL REG_EAX
+#elif defined(__x86_64__)
+#define REG_SYSCALL REG_RAX
+#else
+#error Unsupported platform
+#endif
+
+#ifndef SYS_SECCOMP
+#define SYS_SECCOMP 1
+#endif
+
+int install_seccomp_syscall_debug(void);
+
+#endif
-- 
1.7.1

Re: [Qemu-devel] [PATCHv5 3/4] Adding qemu-seccomp-debug.[ch]

[Anthony Liguori]

Eduardo Otubo <otubo@linux.vnet.ibm.com> writes:

> The new 'trap' (debug) mode will capture the illegal system call before it is
> executed. The feature and the implementation is based on Will Drewry's
> patch - https://lkml.org/lkml/2012/4/12/449
>
> v4:
>  * New files in v4
>  * If SCMP_ACT_TRAP flag used when calling seccomp_init(), the kernel will
>    send a SIGSYS every time a not whitelisted syscall is called. This
>    sighandler install_seccomp_syscall_debug() is installed in this mode so
>    we can intercept the signal and print to the user the illegal syscall.
>    The process resumes after that.
>  * The behavior of the code inside a signal handler sometimes is
>    unpredictable (as stated in man 7 signals). That's why I deliberately
>    used write() and _exit() functions, and had the string-to-int helper
>    functions as well.
>
> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
> ---
>  qemu-seccomp-debug.c |   95 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  qemu-seccomp-debug.h |   38 ++++++++++++++++++++
>  2 files changed, 133 insertions(+), 0 deletions(-)
>  create mode 100644 qemu-seccomp-debug.c
>  create mode 100644 qemu-seccomp-debug.h
>
> diff --git a/qemu-seccomp-debug.c b/qemu-seccomp-debug.c
> new file mode 100644
> index 0000000..162c2f1
> --- /dev/null
> +++ b/qemu-seccomp-debug.c
> @@ -0,0 +1,95 @@
> +
> +/*
> + * QEMU seccomp mode 2 support with libseccomp
> + * Debug system calls helper functions
> + *
> + * Copyright IBM, Corp. 2012
> + *
> + * Authors:
> + *  Eduardo Otubo    <eotubo@br.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + * Contributions after 2012-01-13 are licensed under the terms of the
> + * GNU GPL, version 2 or (at your option) any later version.
> + */
> +
> +#include "qemu-seccomp-debug.h"
> +#include "asm-generic/unistd.h"

This looks like an odd include to me.  I assume you're relying on Linux
headers being installed?  You should at least do <asm-generic/unistd.h>
but I wonder why you need this in the first place.

> +
> +#define safe_warn(data) write(STDERR_FILENO, (const void *) data, sizeof(data))
> +
> +static int count_digits(int number)
> +{
> +    int digits = 0;
> +    while (number) {
> +        number /= 10;
> +        digits++;
> +    }
> +
> +    return digits;
> +}
> +
> +static char *sput_i(int integer, char *string)
> +{
> +    if (integer / 10 != 0) {
> +        string = sput_i(integer / 10, string);
> +    }
> +    *string++ = (char) ('0' + integer % 10);
> +    return string;
> +}
> +
> +static void int_to_asc(int integer, char *string)
> +{
> +    *sput_i(integer, string) = '\n';
> +}
> +
> +static void syscall_debug(int nr, siginfo_t *info, void *void_context)
> +{
> +    ucontext_t *ctx = (ucontext_t *) (void_context);
> +    char errormsg[] = "seccomp: illegal syscall trapped: ";
> +    char syscall_char[count_digits(__NR_syscalls) + 1];
> +    int syscall_num = 0;
> +
> +    if (info->si_code != SYS_SECCOMP) {
> +        return;
> +    }
> +    if (!ctx) {
> +        return;
> +    }
> +    syscall_num = ctx->uc_mcontext.gregs[REG_SYSCALL];
> +    if (syscall_num < 0 || syscall_num >= __NR_syscalls) {
> +        if ((safe_warn("seccomp: error reading syscall from register\n") < 0)) {
> +            return;
> +        }
> +        return;
> +    }
> +    int_to_asc(syscall_num, syscall_char);

I assume you're doign this because of fear of signal safety?  Is there a
reason to believe that snprintf() wouldn't be signal safe?  Even if it's
not on the white list, the implementation can't reasonably rely on
global data, can it?

> +    if ((safe_warn(errormsg) < 0) || (safe_warn(syscall_char) < 0)) {
> +        return;
> +    }
> +    return;
> +}
> +
> +int install_seccomp_syscall_debug(void)
> +{
> +    struct sigaction act;
> +    sigset_t mask;
> +
> +    memset(&act, 0, sizeof(act));
> +    sigemptyset(&mask);
> +    sigaddset(&mask, SIGSYS);
> +
> +    act.sa_sigaction = &syscall_debug;
> +    act.sa_flags = SA_SIGINFO;
> +    if (sigaction(SIGSYS, &act, NULL) < 0) {
> +        perror("seccomp: sigaction returned with errors\n");
> +        return -1;
> +    }
> +    if (pthread_sigmask(SIG_UNBLOCK, &mask, NULL)) {
> +        perror("seccomp: sigprocmask returned with errors\n");
> +        return -1;
> +    }

This looks fishy to me.  We aggressively modify our signal mask in order
to launch a KVM VCPU so I'm pretty sure we'll quickly block SIGSYS.  I
think you need to touch more code than this for it to work.

Regards,

Anthony Liguori

> +    return 0;
> +}
> diff --git a/qemu-seccomp-debug.h b/qemu-seccomp-debug.h
> new file mode 100644
> index 0000000..d3863d6
> --- /dev/null
> +++ b/qemu-seccomp-debug.h
> @@ -0,0 +1,38 @@
> +/*
> + * QEMU seccomp mode 2 support with libseccomp
> + * Trap system calls helper functions
> + *
> + * Copyright IBM, Corp. 2012
> + *
> + * Authors:
> + *  Eduardo Otubo    <eotubo@br.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + * Contributions after 2012-01-13 are licensed under the terms of the
> + * GNU GPL, version 2 or (at your option) any later version.

Version 2 or later for all new files.  Don't include this disclaimer in
new code.

Regards,

Anthony Liguori

> + */
> +#ifndef QEMU_SECCOMP_TRAP_H
> +#define QEMU_SECCOMP_TRAP_H
> +
> +#include <signal.h>
> +#include <string.h>
> +#include <stdio.h>
> +#include <unistd.h>
> +
> +#if defined(__i386__)
> +#define REG_SYSCALL REG_EAX
> +#elif defined(__x86_64__)
> +#define REG_SYSCALL REG_RAX
> +#else
> +#error Unsupported platform
> +#endif
> +
> +#ifndef SYS_SECCOMP
> +#define SYS_SECCOMP 1
> +#endif
> +
> +int install_seccomp_syscall_debug(void);
> +
> +#endif
> -- 
> 1.7.1

Re: [Qemu-devel] [PATCHv5 3/4] Adding qemu-seccomp-debug.[ch]

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1688 bytes --]

On 08/03/2012 02:54 PM, Anthony Liguori wrote:
> Eduardo Otubo <otubo@linux.vnet.ibm.com> writes:
> 
>> The new 'trap' (debug) mode will capture the illegal system call before it is
>> executed. The feature and the implementation is based on Will Drewry's
>> patch - https://lkml.org/lkml/2012/4/12/449
>>

>> +    if (syscall_num < 0 || syscall_num >= __NR_syscalls) {
>> +        if ((safe_warn("seccomp: error reading syscall from register\n") < 0)) {
>> +            return;
>> +        }
>> +        return;
>> +    }
>> +    int_to_asc(syscall_num, syscall_char);
> 
> I assume you're doign this because of fear of signal safety?  Is there a
> reason to believe that snprintf() wouldn't be signal safe?  Even if it's
> not on the white list, the implementation can't reasonably rely on
> global data, can it?

Unfortunately snprintf can malloc (seriously! even in glibc), which
therefore involves not just global data, but a potential for deadlock
while handling the malloc locks.  True, the situations in which snprintf
mallocs are limited to a subset of possible % directives, and while it
differs between libc implementations which set triggers questionable
behavior, you can at least argue that this seccomp code is heavily tied
to Linux and therefore an audit of the code path in glibc for your
particular format string will not malloc.  But I'd rather not play those
sorts of games; it is easier to just follow the rule and stick to
async-signal-safe functions from within signal handlers, which rules out
the entire *printf family.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCHv5 3/4] Adding qemu-seccomp-debug.[ch]

[Eduardo Otubo]

On Fri, Aug 03, 2012 at 03:54:40PM -0500, Anthony Liguori wrote:
> Eduardo Otubo <otubo@linux.vnet.ibm.com> writes:
> 
> > The new 'trap' (debug) mode will capture the illegal system call before it is
> > executed. The feature and the implementation is based on Will Drewry's
> > patch - https://lkml.org/lkml/2012/4/12/449
> >
> > v4:
> >  * New files in v4
> >  * If SCMP_ACT_TRAP flag used when calling seccomp_init(), the kernel will
> >    send a SIGSYS every time a not whitelisted syscall is called. This
> >    sighandler install_seccomp_syscall_debug() is installed in this mode so
> >    we can intercept the signal and print to the user the illegal syscall.
> >    The process resumes after that.
> >  * The behavior of the code inside a signal handler sometimes is
> >    unpredictable (as stated in man 7 signals). That's why I deliberately
> >    used write() and _exit() functions, and had the string-to-int helper
> >    functions as well.
> >
> > Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
> > ---
> >  qemu-seccomp-debug.c |   95 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  qemu-seccomp-debug.h |   38 ++++++++++++++++++++
> >  2 files changed, 133 insertions(+), 0 deletions(-)
> >  create mode 100644 qemu-seccomp-debug.c
> >  create mode 100644 qemu-seccomp-debug.h
> >
> > diff --git a/qemu-seccomp-debug.c b/qemu-seccomp-debug.c
> > new file mode 100644
> > index 0000000..162c2f1
> > --- /dev/null
> > +++ b/qemu-seccomp-debug.c
> > @@ -0,0 +1,95 @@
> > +
> > +/*
> > + * QEMU seccomp mode 2 support with libseccomp
> > + * Debug system calls helper functions
> > + *
> > + * Copyright IBM, Corp. 2012
> > + *
> > + * Authors:
> > + *  Eduardo Otubo    <eotubo@br.ibm.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2.  See
> > + * the COPYING file in the top-level directory.
> > + *
> > + * Contributions after 2012-01-13 are licensed under the terms of the
> > + * GNU GPL, version 2 or (at your option) any later version.
> > + */
> > +
> > +#include "qemu-seccomp-debug.h"
> > +#include "asm-generic/unistd.h"
> 
> This looks like an odd include to me.  I assume you're relying on Linux
> headers being installed?  You should at least do <asm-generic/unistd.h>
> but I wonder why you need this in the first place.
> 

You're right, <asm-generic/unistd.h> is ideal. I include this header
because I need __NR_syscalls to be defined. Not sure if there's any other
place I can find its definition other than Linux header.

> > +
> > +#define safe_warn(data) write(STDERR_FILENO, (const void *) data, sizeof(data))
> > +
> > +static int count_digits(int number)
> > +{
> > +    int digits = 0;
> > +    while (number) {
> > +        number /= 10;
> > +        digits++;
> > +    }
> > +
> > +    return digits;
> > +}
> > +
> > +static char *sput_i(int integer, char *string)
> > +{
> > +    if (integer / 10 != 0) {
> > +        string = sput_i(integer / 10, string);
> > +    }
> > +    *string++ = (char) ('0' + integer % 10);
> > +    return string;
> > +}
> > +
> > +static void int_to_asc(int integer, char *string)
> > +{
> > +    *sput_i(integer, string) = '\n';
> > +}
> > +
> > +static void syscall_debug(int nr, siginfo_t *info, void *void_context)
> > +{
> > +    ucontext_t *ctx = (ucontext_t *) (void_context);
> > +    char errormsg[] = "seccomp: illegal syscall trapped: ";
> > +    char syscall_char[count_digits(__NR_syscalls) + 1];
> > +    int syscall_num = 0;
> > +
> > +    if (info->si_code != SYS_SECCOMP) {
> > +        return;
> > +    }
> > +    if (!ctx) {
> > +        return;
> > +    }
> > +    syscall_num = ctx->uc_mcontext.gregs[REG_SYSCALL];
> > +    if (syscall_num < 0 || syscall_num >= __NR_syscalls) {
> > +        if ((safe_warn("seccomp: error reading syscall from register\n") < 0)) {
> > +            return;
> > +        }
> > +        return;
> > +    }
> > +    int_to_asc(syscall_num, syscall_char);
> 
> I assume you're doign this because of fear of signal safety?  Is there a
> reason to believe that snprintf() wouldn't be signal safe?  Even if it's
> not on the white list, the implementation can't reasonably rely on
> global data, can it?
> 

Eric Blake made a good point on his answer. Better stick with
async-signal-safe function from within a signal handler.

> > +    if ((safe_warn(errormsg) < 0) || (safe_warn(syscall_char) < 0)) {
> > +        return;
> > +    }
> > +    return;
> > +}
> > +
> > +int install_seccomp_syscall_debug(void)
> > +{
> > +    struct sigaction act;
> > +    sigset_t mask;
> > +
> > +    memset(&act, 0, sizeof(act));
> > +    sigemptyset(&mask);
> > +    sigaddset(&mask, SIGSYS);
> > +
> > +    act.sa_sigaction = &syscall_debug;
> > +    act.sa_flags = SA_SIGINFO;
> > +    if (sigaction(SIGSYS, &act, NULL) < 0) {
> > +        perror("seccomp: sigaction returned with errors\n");
> > +        return -1;
> > +    }
> > +    if (pthread_sigmask(SIG_UNBLOCK, &mask, NULL)) {
> > +        perror("seccomp: sigprocmask returned with errors\n");
> > +        return -1;
> > +    }
> 
> This looks fishy to me.  We aggressively modify our signal mask in order
> to launch a KVM VCPU so I'm pretty sure we'll quickly block SIGSYS.  I
> think you need to touch more code than this for it to work.
> 

I didn't know there were other parts in Qemu that set sig masks as well.
I'll try to adjust my patch and put my handler in the correct place in the
next time. Thanks :)

> > +    return 0;
> > +}
> > diff --git a/qemu-seccomp-debug.h b/qemu-seccomp-debug.h
> > new file mode 100644
> > index 0000000..d3863d6
> > --- /dev/null
> > +++ b/qemu-seccomp-debug.h
> > @@ -0,0 +1,38 @@
> > +/*
> > + * QEMU seccomp mode 2 support with libseccomp
> > + * Trap system calls helper functions
> > + *
> > + * Copyright IBM, Corp. 2012
> > + *
> > + * Authors:
> > + *  Eduardo Otubo    <eotubo@br.ibm.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2.  See
> > + * the COPYING file in the top-level directory.
> > + *
> > + * Contributions after 2012-01-13 are licensed under the terms of the
> > + * GNU GPL, version 2 or (at your option) any later version.
> 
> Version 2 or later for all new files.  Don't include this disclaimer in
> new code.

ok

-- 
Eduardo Otubo
Software Engineer
Linux Technology Center
IBM Systems & Technology Group
Mobile: +55 19 8135 0885 
eotubo@linux.vnet.ibm.com

Re: [Qemu-devel] [PATCHv5 3/4] Adding qemu-seccomp-debug.[ch]

[Eduardo Otubo]

On Mon, Aug 06, 2012 at 10:19:40AM -0300, Eduardo Otubo wrote:
> On Fri, Aug 03, 2012 at 03:54:40PM -0500, Anthony Liguori wrote:
> > Eduardo Otubo <otubo@linux.vnet.ibm.com> writes:
> > 
> > > The new 'trap' (debug) mode will capture the illegal system call before it is
> > > executed. The feature and the implementation is based on Will Drewry's
> > > patch - https://lkml.org/lkml/2012/4/12/449
> > >
> > > v4:
> > >  * New files in v4
> > >  * If SCMP_ACT_TRAP flag used when calling seccomp_init(), the kernel will
> > >    send a SIGSYS every time a not whitelisted syscall is called. This
> > >    sighandler install_seccomp_syscall_debug() is installed in this mode so
> > >    we can intercept the signal and print to the user the illegal syscall.
> > >    The process resumes after that.
> > >  * The behavior of the code inside a signal handler sometimes is
> > >    unpredictable (as stated in man 7 signals). That's why I deliberately
> > >    used write() and _exit() functions, and had the string-to-int helper
> > >    functions as well.
> > >
> > > Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
> > > ---
> > >  qemu-seccomp-debug.c |   95 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  qemu-seccomp-debug.h |   38 ++++++++++++++++++++
> > >  2 files changed, 133 insertions(+), 0 deletions(-)
> > >  create mode 100644 qemu-seccomp-debug.c
> > >  create mode 100644 qemu-seccomp-debug.h
> > >
> > > diff --git a/qemu-seccomp-debug.c b/qemu-seccomp-debug.c
> > > new file mode 100644
> > > index 0000000..162c2f1
> > > --- /dev/null
> > > +++ b/qemu-seccomp-debug.c
> > > @@ -0,0 +1,95 @@
> > > +
> > > +/*
> > > + * QEMU seccomp mode 2 support with libseccomp
> > > + * Debug system calls helper functions
> > > + *
> > > + * Copyright IBM, Corp. 2012
> > > + *
> > > + * Authors:
> > > + *  Eduardo Otubo    <eotubo@br.ibm.com>
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 2.  See
> > > + * the COPYING file in the top-level directory.
> > > + *
> > > + * Contributions after 2012-01-13 are licensed under the terms of the
> > > + * GNU GPL, version 2 or (at your option) any later version.
> > > + */
> > > +
> > > +#include "qemu-seccomp-debug.h"
> > > +#include "asm-generic/unistd.h"
> > 
> > This looks like an odd include to me.  I assume you're relying on Linux
> > headers being installed?  You should at least do <asm-generic/unistd.h>
> > but I wonder why you need this in the first place.
> > 
> 
> You're right, <asm-generic/unistd.h> is ideal. I include this header
> because I need __NR_syscalls to be defined. Not sure if there's any other
> place I can find its definition other than Linux header.
> 
> > > +
> > > +#define safe_warn(data) write(STDERR_FILENO, (const void *) data, sizeof(data))
> > > +
> > > +static int count_digits(int number)
> > > +{
> > > +    int digits = 0;
> > > +    while (number) {
> > > +        number /= 10;
> > > +        digits++;
> > > +    }
> > > +
> > > +    return digits;
> > > +}
> > > +
> > > +static char *sput_i(int integer, char *string)
> > > +{
> > > +    if (integer / 10 != 0) {
> > > +        string = sput_i(integer / 10, string);
> > > +    }
> > > +    *string++ = (char) ('0' + integer % 10);
> > > +    return string;
> > > +}
> > > +
> > > +static void int_to_asc(int integer, char *string)
> > > +{
> > > +    *sput_i(integer, string) = '\n';
> > > +}
> > > +
> > > +static void syscall_debug(int nr, siginfo_t *info, void *void_context)
> > > +{
> > > +    ucontext_t *ctx = (ucontext_t *) (void_context);
> > > +    char errormsg[] = "seccomp: illegal syscall trapped: ";
> > > +    char syscall_char[count_digits(__NR_syscalls) + 1];
> > > +    int syscall_num = 0;
> > > +
> > > +    if (info->si_code != SYS_SECCOMP) {
> > > +        return;
> > > +    }
> > > +    if (!ctx) {
> > > +        return;
> > > +    }
> > > +    syscall_num = ctx->uc_mcontext.gregs[REG_SYSCALL];
> > > +    if (syscall_num < 0 || syscall_num >= __NR_syscalls) {
> > > +        if ((safe_warn("seccomp: error reading syscall from register\n") < 0)) {
> > > +            return;
> > > +        }
> > > +        return;
> > > +    }
> > > +    int_to_asc(syscall_num, syscall_char);
> > 
> > I assume you're doign this because of fear of signal safety?  Is there a
> > reason to believe that snprintf() wouldn't be signal safe?  Even if it's
> > not on the white list, the implementation can't reasonably rely on
> > global data, can it?
> > 
> 
> Eric Blake made a good point on his answer. Better stick with
> async-signal-safe function from within a signal handler.
> 
> > > +    if ((safe_warn(errormsg) < 0) || (safe_warn(syscall_char) < 0)) {
> > > +        return;
> > > +    }
> > > +    return;
> > > +}
> > > +
> > > +int install_seccomp_syscall_debug(void)
> > > +{
> > > +    struct sigaction act;
> > > +    sigset_t mask;
> > > +
> > > +    memset(&act, 0, sizeof(act));
> > > +    sigemptyset(&mask);
> > > +    sigaddset(&mask, SIGSYS);
> > > +
> > > +    act.sa_sigaction = &syscall_debug;
> > > +    act.sa_flags = SA_SIGINFO;
> > > +    if (sigaction(SIGSYS, &act, NULL) < 0) {
> > > +        perror("seccomp: sigaction returned with errors\n");
> > > +        return -1;
> > > +    }
> > > +    if (pthread_sigmask(SIG_UNBLOCK, &mask, NULL)) {
> > > +        perror("seccomp: sigprocmask returned with errors\n");
> > > +        return -1;
> > > +    }
> > 
> > This looks fishy to me.  We aggressively modify our signal mask in order
> > to launch a KVM VCPU so I'm pretty sure we'll quickly block SIGSYS.  I
> > think you need to touch more code than this for it to work.
> > 

Are you talking about the function qemu_kvm_init_cpu_signals()? Not sure
if I understood you correctly, you're saying you planning to add SIGSYS
into the blocked set of signals inside this function?

In this case we better skip the debug mode for now, sice we're getting
close to the feature freeze period, and I can think about a better way
to handle SIGSYS in the future.

How does that sound?

Regards,

> 
> I didn't know there were other parts in Qemu that set sig masks as well.
> I'll try to adjust my patch and put my handler in the correct place in the
> next time. Thanks :)
> 
> > > +    return 0;
> > > +}
> > > diff --git a/qemu-seccomp-debug.h b/qemu-seccomp-debug.h
> > > new file mode 100644
> > > index 0000000..d3863d6
> > > --- /dev/null
> > > +++ b/qemu-seccomp-debug.h
> > > @@ -0,0 +1,38 @@
> > > +/*
> > > + * QEMU seccomp mode 2 support with libseccomp
> > > + * Trap system calls helper functions
> > > + *
> > > + * Copyright IBM, Corp. 2012
> > > + *
> > > + * Authors:
> > > + *  Eduardo Otubo    <eotubo@br.ibm.com>
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 2.  See
> > > + * the COPYING file in the top-level directory.
> > > + *
> > > + * Contributions after 2012-01-13 are licensed under the terms of the
> > > + * GNU GPL, version 2 or (at your option) any later version.
> > 
> > Version 2 or later for all new files.  Don't include this disclaimer in
> > new code.
> 
> ok
> 
> -- 
> Eduardo Otubo
> Software Engineer
> Linux Technology Center
> IBM Systems & Technology Group
> Mobile: +55 19 8135 0885 
> eotubo@linux.vnet.ibm.com

[Qemu-devel] [PATCHv5 4/4] Adding seccomp calls to vl.c

[Eduardo Otubo]

v1:
 * Full seccomp calls and data included in vl.c

v2:
 * Full seccomp calls and data removed from vl.c and put into separate
   qemu-seccomp.[ch] file.

v4:
 * Call to install_seccomp_syscall_debug() added.
 * Now calling seccomp_start() with 'SECCOMP_MODE' argument, depending on
   settings used in configure script.

Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
---
 vl.c |   31 +++++++++++++++++++++++++++++++
 1 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/vl.c b/vl.c
index 9fea320..808f020 100644
--- a/vl.c
+++ b/vl.c
@@ -62,6 +62,14 @@
 #include <linux/ppdev.h>
 #include <linux/parport.h>
 #endif
+
+#ifdef CONFIG_SECCOMP
+#include "qemu-seccomp.h"
+#endif
+#ifdef CONFIG_SECCOMP_DEBUG
+#include "qemu-seccomp-debug.h"
+#endif
+
 #ifdef __sun__
 #include <sys/stat.h>
 #include <sys/ethernet.h>
@@ -169,6 +177,14 @@ int main(int argc, char **argv)
 
 #define MAX_VIRTIO_CONSOLES 1
 
+#ifdef CONFIG_SECCOMP
+#ifdef CONFIG_SECCOMP_DEBUG
+#define SECCOMP_MODE SCMP_ACT_TRAP
+#else
+#define SECCOMP_MODE SCMP_ACT_KILL
+#endif
+#endif
+
 static const char *data_dir;
 const char *bios_name = NULL;
 enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
@@ -2295,6 +2311,21 @@ int main(int argc, char **argv, char **envp)
     const char *trace_events = NULL;
     const char *trace_file = NULL;
 
+#ifdef CONFIG_SECCOMP_DEBUG
+    if (install_seccomp_syscall_debug()) {
+        fprintf(stderr, "seccomp: failed to install system call debug\n");
+        exit(1);
+    }
+#endif
+
+#ifdef CONFIG_SECCOMP
+    if (seccomp_start(SECCOMP_MODE) < 0) {
+        fprintf(stderr,
+                "seccomp: failed to install syscall filter in the kernel\n");
+        exit(1);
+    }
+#endif
+
     atexit(qemu_run_exit_notifiers);
     error_set_progname(argv[0]);
 
-- 
1.7.1