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

[Anthony Liguori]

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.

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

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