Re: [Qemu-devel] [PATCHv2 1/3] seccomp: adding blacklist support

[Corey Bryant <coreyb@linux.vnet.ibm.com>]

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 <otubo@linux.vnet.ibm.com>
> ---
>   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 <seccomp.h>
>   #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