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

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

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

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.

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

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