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