From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47684) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VTvXt-0005tz-UP for qemu-devel@nongnu.org; Wed, 09 Oct 2013 11:19:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VTvXk-0006uW-Ur for qemu-devel@nongnu.org; Wed, 09 Oct 2013 11:19:29 -0400 Received: from e36.co.us.ibm.com ([32.97.110.154]:54947) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VTvXk-0006uE-N8 for qemu-devel@nongnu.org; Wed, 09 Oct 2013 11:19:20 -0400 Received: from /spool/local by e36.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 9 Oct 2013 09:19:19 -0600 Received: from d03relay03.boulder.ibm.com (d03relay03.boulder.ibm.com [9.17.195.228]) by d03dlp01.boulder.ibm.com (Postfix) with ESMTP id 2A26F1FF0049 for ; Wed, 9 Oct 2013 09:19:08 -0600 (MDT) Received: from d03av05.boulder.ibm.com (d03av05.boulder.ibm.com [9.17.195.85]) by d03relay03.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r99FJEnA207398 for ; Wed, 9 Oct 2013 09:19:14 -0600 Received: from d03av05.boulder.ibm.com (localhost [127.0.0.1]) by d03av05.boulder.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id r99FJEfB008078 for ; Wed, 9 Oct 2013 09:19:14 -0600 Message-ID: <525573F1.4050301@linux.vnet.ibm.com> Date: Wed, 09 Oct 2013 11:19:13 -0400 From: Corey Bryant MIME-Version: 1.0 References: <1381279346-23676-1-git-send-email-otubo@linux.vnet.ibm.com> <1381279346-23676-2-git-send-email-otubo@linux.vnet.ibm.com> In-Reply-To: <1381279346-23676-2-git-send-email-otubo@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCHv3 1/3] seccomp: adding blacklist support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Otubo Cc: pmoore@redhat.com, qemu-devel@nongnu.org, anthony@codemonkey.ws On 10/08/2013 08:42 PM, Eduardo Otubo wrote: > v3: The "-netdev tap" option is checked in the vl.c file during the > process of the command line argument list. It sets tap_enabled to true > or false according to the configuration found. Later at the seccomp > filter installation, this value is checked wheter to install or not this > feature. > > Adding a system call blacklist right before the vcpus starts. This > filter is composed by the system calls that can't be executed after the > guests are up. This list should be refined as whitelist is, with as much > testing as we can do using virt-test. > > Signed-off-by: Eduardo Otubo > --- > include/sysemu/seccomp.h | 6 ++++- > qemu-seccomp.c | 64 +++++++++++++++++++++++++++++++++++++++--------- > vl.c | 21 +++++++++++++++- > 3 files changed, 77 insertions(+), 14 deletions(-) > > diff --git a/include/sysemu/seccomp.h b/include/sysemu/seccomp.h > index 1189fa2..9dc7e52 100644 > --- a/include/sysemu/seccomp.h > +++ b/include/sysemu/seccomp.h > @@ -15,8 +15,12 @@ > #ifndef QEMU_SECCOMP_H > #define QEMU_SECCOMP_H > > +#define WHITELIST 0 > +#define BLACKLIST 1 > + > #include > #include "qemu/osdep.h" > > -int seccomp_start(void); > +int seccomp_start(int list_type); > + > #endif > diff --git a/qemu-seccomp.c b/qemu-seccomp.c > index 37d38f8..84a42bc 100644 > --- a/qemu-seccomp.c > +++ b/qemu-seccomp.c > @@ -21,7 +21,7 @@ struct QemuSeccompSyscall { > uint8_t priority; > }; > > -static const struct QemuSeccompSyscall seccomp_whitelist[] = { > +static const struct QemuSeccompSyscall whitelist[] = { > { SCMP_SYS(timer_settime), 255 }, > { SCMP_SYS(timer_gettime), 254 }, > { SCMP_SYS(futex), 253 }, > @@ -221,32 +221,72 @@ static const struct QemuSeccompSyscall seccomp_whitelist[] = { > { SCMP_SYS(arch_prctl), 240 } > }; > > -int seccomp_start(void) > +/* > + * The second list, called blacklist, basically reduces previously installed > + * whitelist. All the syscalls configured by the previous whitelist are still > + * allowed, except for the ones in the blacklist. > + * */ > + > +static const struct QemuSeccompSyscall blacklist[] = { > + { SCMP_SYS(execve), 255 } > +}; > + > +static int process_list(scmp_filter_ctx *ctx, > + const struct QemuSeccompSyscall *list, > + unsigned int list_size, uint32_t action) > { > int rc = 0; > unsigned int i = 0; > - scmp_filter_ctx ctx; > > - ctx = seccomp_init(SCMP_ACT_KILL); > - if (ctx == NULL) { > - goto seccomp_return; > - } > + for (i = 0; i < list_size; i++) { > + rc = seccomp_rule_add(ctx, action, list[i].num, 0); > + if (rc < 0) { > + goto seccomp_return; > + } > > - for (i = 0; i < ARRAY_SIZE(seccomp_whitelist); i++) { > - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, seccomp_whitelist[i].num, 0); > + rc = seccomp_syscall_priority(ctx, list[i].num, > + list[i].priority); > if (rc < 0) { > goto seccomp_return; > } > - rc = seccomp_syscall_priority(ctx, seccomp_whitelist[i].num, > - seccomp_whitelist[i].priority); > + } > + > +seccomp_return: > + return rc; > +} > + > +int seccomp_start(int list_type) > +{ > + int rc = 0; > + scmp_filter_ctx ctx; > + > + switch (list_type) { > + case WHITELIST: > + ctx = seccomp_init(SCMP_ACT_KILL); > + if (ctx == NULL) { > + goto seccomp_return; > + } > + rc = process_list(ctx, whitelist, ARRAY_SIZE(whitelist), SCMP_ACT_ALLOW); > if (rc < 0) { > goto seccomp_return; > } > + break; > + case BLACKLIST: > + ctx = seccomp_init(SCMP_ACT_ALLOW); > + if (ctx == NULL) { > + goto seccomp_return; > + } > + rc = process_list(ctx, blacklist, ARRAY_SIZE(blacklist), SCMP_ACT_KILL); > + break; > + default: > + rc = -1; > + goto seccomp_return; > } > > rc = seccomp_load(ctx); > > seccomp_return: > - seccomp_release(ctx); > + if (ctx) > + seccomp_release(ctx); > return rc; > } > diff --git a/vl.c b/vl.c > index b4b119a..ee95674 100644 > --- a/vl.c > +++ b/vl.c > @@ -179,6 +179,8 @@ int main(int argc, char **argv) > #define MAX_VIRTIO_CONSOLES 1 > #define MAX_SCLP_CONSOLES 1 > > +static bool enable_blacklist = false; > +static bool tap_enabled = false; > static const char *data_dir[16]; > static int data_dir_idx; > const char *bios_name = NULL; > @@ -1033,7 +1035,7 @@ static int parse_sandbox(QemuOpts *opts, void *opaque) > /* FIXME: change this to true for 1.3 */ > if (qemu_opt_get_bool(opts, "enable", false)) { > #ifdef CONFIG_SECCOMP > - if (seccomp_start() < 0) { > + if (seccomp_start(WHITELIST) < 0) { > qerror_report(ERROR_CLASS_GENERIC_ERROR, > "failed to install seccomp syscall filter in the kernel"); > return -1; > @@ -1765,12 +1767,24 @@ void vm_state_notify(int running, RunState state) > } > } > > +static void install_seccomp_blacklist(void) > +{ > + if (enable_blacklist && !tap_enabled) { > + if (seccomp_start(BLACKLIST) < 0) { I don't think this is flexible enough for future growth. If you're going to use a dynamic approach to building the blacklist, then wouldn't you want to blacklist syscalls individually based on the option that causes them to be used? The approach you have here would be all or nothing. > + qerror_report(ERROR_CLASS_GENERIC_ERROR, > + "failed to install seccomp syscall second level filter in the kernel"); > + exit(1); > + } > + } > +} > + > void vm_start(void) > { > if (!runstate_is_running()) { > cpu_enable_ticks(); > runstate_set(RUN_STATE_RUNNING); > vm_state_notify(1, RUN_STATE_RUNNING); > + install_seccomp_blacklist(); > resume_all_vcpus(); > monitor_protocol_event(QEVENT_RESUME, NULL); > } > @@ -3208,6 +3222,11 @@ int main(int argc, char **argv, char **envp) > if (net_client_parse(qemu_find_opts("netdev"), optarg) == -1) { > exit(1); > } > + > + if(strcmp(optarg, "tap")){ > + tap_enabled = true; > + } You're not covering all command line options that lead to exec calls. I see the following with 'git grep execv': net/tap.c: execv(setup_script, args); net/tap.c: execv("/bin/sh", args); net/tap.c: execv(helper, args); slirp/misc.c: execvp(argv[0], (char **)argv); So I know you're at least missing -net bridge. And maybe slirp, but I'm not sure about that. What about hotplugging a network tap or bridge device? You'll need to at least document that they're not going to work when -sandbox is in effect, and you'll need to fail nicely if they're attempted. > + > break; > case QEMU_OPTION_net: > if (net_client_parse(qemu_find_opts("net"), optarg) == -1) { > -- Regards, Corey Bryant