From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:56421) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SzBH2-0005LW-6j for qemu-devel@nongnu.org; Wed, 08 Aug 2012 14:46:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SzBH0-000723-76 for qemu-devel@nongnu.org; Wed, 08 Aug 2012 14:46:28 -0400 Received: from e24smtp01.br.ibm.com ([32.104.18.85]:38144) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SzBGz-0006ox-NA for qemu-devel@nongnu.org; Wed, 08 Aug 2012 14:46:26 -0400 Received: from /spool/local by e24smtp01.br.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 8 Aug 2012 15:46:19 -0300 Received: from d24relay03.br.ibm.com (d24relay03.br.ibm.com [9.13.184.25]) by d24dlp01.br.ibm.com (Postfix) with ESMTP id A8A143520044 for ; Wed, 8 Aug 2012 14:46:10 -0400 (EDT) Received: from d24av04.br.ibm.com (d24av04.br.ibm.com [9.8.31.97]) by d24relay03.br.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q78IjMZw26476560 for ; Wed, 8 Aug 2012 15:45:22 -0300 Received: from d24av04.br.ibm.com (loopback [127.0.0.1]) by d24av04.br.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q78Gk2BK022620 for ; Wed, 8 Aug 2012 13:46:02 -0300 Date: Wed, 8 Aug 2012 15:46:08 -0300 From: Eduardo Otubo Message-ID: <20120808184607.GA7024@bluepex.com> References: <87obmru25r.fsf@codemonkey.ws> <20120806131940.GA12163@bluepex.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20120806131940.GA12163@bluepex.com> Subject: Re: [Qemu-devel] [PATCHv5 3/4] Adding qemu-seccomp-debug.[ch] List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori , qemu-devel@nongnu.org, pmoore@redhat.com, wad@chromium.org, blauwirbel@gmail.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 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 > > > --- > > > 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 > > > + * > > > + * 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 > > but I wonder why you need this in the first place. > > > > You're right, 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 > > > + * > > > + * 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