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

[Eduardo Otubo <otubo@linux.vnet.ibm.com>]

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