From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37000) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VJnIu-0002ta-DK for qemu-devel@nongnu.org; Wed, 11 Sep 2013 12:30:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VJnIl-00085q-6L for qemu-devel@nongnu.org; Wed, 11 Sep 2013 12:30:08 -0400 Received: from e7.ny.us.ibm.com ([32.97.182.137]:51897) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VJnIl-00085e-1F for qemu-devel@nongnu.org; Wed, 11 Sep 2013 12:29:59 -0400 Received: from /spool/local by e7.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 11 Sep 2013 12:29:57 -0400 Received: from b01cxnp22033.gho.pok.ibm.com (b01cxnp22033.gho.pok.ibm.com [9.57.198.23]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id 5BDA838C80E8 for ; Wed, 11 Sep 2013 12:29:41 -0400 (EDT) Received: from d01av03.pok.ibm.com (d01av03.pok.ibm.com [9.56.224.217]) by b01cxnp22033.gho.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r8BGTahO39911624 for ; Wed, 11 Sep 2013 16:29:36 GMT Received: from d01av03.pok.ibm.com (loopback [127.0.0.1]) by d01av03.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r8BGTZ3R031242 for ; Wed, 11 Sep 2013 13:29:36 -0300 Message-ID: <52309A6F.3070902@linux.vnet.ibm.com> Date: Wed, 11 Sep 2013 12:29:35 -0400 From: Corey Bryant MIME-Version: 1.0 References: <1378495308-24560-1-git-send-email-otubo@linux.vnet.ibm.com> <1378495308-24560-2-git-send-email-otubo@linux.vnet.ibm.com> In-Reply-To: <1378495308-24560-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] [PATCHv2 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 On 09/06/2013 03:21 PM, Eduardo Otubo wrote: > 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 the whitelist is, with as much testing as we can > do using virt-test. It would be useful to point out in the commit message and maybe also in the code comments that the blacklist essentially reduces the previously installed whitelist. > > Signed-off-by: Eduardo Otubo > --- > include/sysemu/seccomp.h | 5 ++++- > qemu-seccomp.c | 57 ++++++++++++++++++++++++++++++++++++++---------- > vl.c | 16 +++++++++++++- > 3 files changed, 64 insertions(+), 14 deletions(-) > > diff --git a/include/sysemu/seccomp.h b/include/sysemu/seccomp.h > index 1189fa2..551ad12 100644 > --- a/include/sysemu/seccomp.h > +++ b/include/sysemu/seccomp.h > @@ -15,8 +15,11 @@ > #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 state); This parameter is more of a list type than a state. > #endif > diff --git a/qemu-seccomp.c b/qemu-seccomp.c > index 37d38f8..5e85eb5 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,65 @@ static const struct QemuSeccompSyscall seccomp_whitelist[] = { > { SCMP_SYS(arch_prctl), 240 } > }; > > -int seccomp_start(void) > +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_return: > seccomp_release(ctx); > return rc; > } > diff --git a/vl.c b/vl.c > index b4b119a..02f7486 100644 > --- a/vl.c > +++ b/vl.c > @@ -179,6 +179,7 @@ int main(int argc, char **argv) > #define MAX_VIRTIO_CONSOLES 1 > #define MAX_SCLP_CONSOLES 1 > > +static bool enable_blacklist = false; > static const char *data_dir[16]; > static int data_dir_idx; > const char *bios_name = NULL; > @@ -1033,11 +1034,13 @@ 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; > } > + > + enable_blacklist = true; The blacklist shouldn't be enabled until the command line option asks for it to be enabled in the next patch. > #else > qerror_report(ERROR_CLASS_GENERIC_ERROR, > "sandboxing request but seccomp is not compiled into this build"); > @@ -1765,12 +1768,23 @@ void vm_state_notify(int running, RunState state) > } > } > > +static void install_seccomp_blacklist(void) > +{ > + if (enable_blacklist) { > + if (seccomp_start(BLACKLIST) < 0) { > + qerror_report(ERROR_CLASS_GENERIC_ERROR, > + "failed to install seccomp syscall second level filter in the kernel"); As Lei Li mentioned, should you return -1 here like the whitelist failure path does? > + } > + } > +} > + > 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(); The blacklist isn't installed when the incoming migration path is taken. In other words, should install_seccomp_blacklist() be called before the "if (incoming)" check in main()? > resume_all_vcpus(); > monitor_protocol_event(QEVENT_RESUME, NULL); > } > -- Regards, Corey Bryant