[Qemu-devel] [PATCHv3 0/3] seccomp: adding blacklist support with command line

[Qemu-devel] [PATCHv3 0/3] seccomp: adding blacklist support with command line

[Eduardo Otubo]

v3: The "-netdev tap" option is checked in order to decide if the blacklist is
eligible to be installed or not, since it's one the most used features that is
known to use the exec() system call. It's an automatic mechanism to avoid Qemu
to break when using the blacklist feature.

v2: The blacklist works exactly the opposite as the whitelist. I set the
default behaiour to SCMP_ACT_ALLOW and the exceptions to SCMP_ACT_KILL;
remembering it inherits the behavior from the previous installed whitelist.

v1: The second whitelist is installed right before the vcpu starts, it contains all
the system calls the first one has except for exec() and select(), which are
big major syscalls that I could extensively test with virt-test and do not
cause any damage to the general execution.

This patch series also contain the command line support for this feature and
some minor fixes, all of them described in their own commit messages.

The environment in which the second whitelist is installed seems to need less
system calls than the first, so the procedure here will be the same: Keep
testing with virt-test and get to the smallest list as possible.

Eduardo Otubo (3):
  seccomp: adding blacklist support
  seccomp: adding command line support for blacklist
  seccomp: general fixes

 include/sysemu/seccomp.h |  6 ++++-
 qemu-options.hx          |  8 +++---
 qemu-seccomp.c           | 66 ++++++++++++++++++++++++++++++++++++++----------
 vl.c                     | 42 +++++++++++++++++++++++++++---
 4 files changed, 101 insertions(+), 21 deletions(-)

-- 
1.8.3.1

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

[Eduardo Otubo]

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 <otubo@linux.vnet.ibm.com>
---
 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 <seccomp.h>
 #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) {
+            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;
+                }
+
                 break;
             case QEMU_OPTION_net:
                 if (net_client_parse(qemu_find_opts("net"), optarg) == -1) {
-- 
1.8.3.1

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1107 bytes --]

On 10/08/2013 06: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

s/wheter/whether/

> 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 <otubo@linux.vnet.ibm.com>
> ---
>  include/sysemu/seccomp.h |  6 ++++-
>  qemu-seccomp.c           | 64 +++++++++++++++++++++++++++++++++++++++---------
>  vl.c                     | 21 +++++++++++++++-
>  3 files changed, 77 insertions(+), 14 deletions(-)

No review on the actual patch, just spotting a typo.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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

[Eduardo Otubo]

On 10/08/2013 11:05 PM, Eric Blake wrote:
> On 10/08/2013 06: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
>
> s/wheter/whether/

Thank you.

>
>> 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 <otubo@linux.vnet.ibm.com>
>> ---
>>   include/sysemu/seccomp.h |  6 ++++-
>>   qemu-seccomp.c           | 64 +++++++++++++++++++++++++++++++++++++++---------
>>   vl.c                     | 21 +++++++++++++++-
>>   3 files changed, 77 insertions(+), 14 deletions(-)
>
> No review on the actual patch, just spotting a typo.
>
>

-- 
Eduardo Otubo
IBM Linux Technology Center

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

[Corey Bryant]

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

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

[Paul Moore]

On Tuesday, October 08, 2013 09:42:24 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.

I like the idea of slowly making the QEMU syscall filter dependent on the 
runtime configuration.  With that in mind, I wonder if we should have a more 
general purpose API in include/sysemu/seccomp.h that allows QEMU to indicate 
to the the QEMU/seccomp code that a particular feature is enabled.

Maybe something like this:

  #define SCMP_FEAT_TAP ...

  int seccomp_feature_enable(int feature);

One more comment below.

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

Should these #defines be namespaced in some way, e.g. SCMP_LIST_BLACKLIST?

>  #include <seccomp.h>
>  #include "qemu/osdep.h"
> 
> -int seccomp_start(void);
> +int seccomp_start(int list_type);
> +
>  #endif


-- 
paul moore
security and virtualization @ redhat

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

[Corey Bryant]

On 10/09/2013 05:36 PM, Paul Moore wrote:
> On Tuesday, October 08, 2013 09:42:24 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.
>
> I like the idea of slowly making the QEMU syscall filter dependent on the
> runtime configuration.  With that in mind, I wonder if we should have a more
> general purpose API in include/sysemu/seccomp.h that allows QEMU to indicate
> to the the QEMU/seccomp code that a particular feature is enabled.
>
> Maybe something like this:
>
>    #define SCMP_FEAT_TAP ...
>
>    int seccomp_feature_enable(int feature);

This is a good approach, and then the blacklist can vary based on what 
features are enabled.

-- 
Regards,
Corey Bryant

>
> One more comment below.
>
>> 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 <otubo@linux.vnet.ibm.com>
>> ---
>>   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
>
> Should these #defines be namespaced in some way, e.g. SCMP_LIST_BLACKLIST?
>
>>   #include <seccomp.h>
>>   #include "qemu/osdep.h"
>>
>> -int seccomp_start(void);
>> +int seccomp_start(int list_type);
>> +
>>   #endif
>
>

[Qemu-devel] [PATCHv3 2/3] seccomp: adding command line support for blacklist

[Eduardo Otubo]

v3: The options for blacklist in the command line also checkes the
existence of "-netdev tap", leaving a warning message in a positive
case.

New command line options for the seccomp blacklist feature:

 $ qemu -sandbox on[,strict=<on|off>]

The strict parameter will turn on or off the new system call blacklist

Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
---
 qemu-options.hx |  8 +++++---
 vl.c            | 17 ++++++++++++++++-
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index d15338e..05485e1 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2978,13 +2978,15 @@ Old param mode (ARM only).
 ETEXI
 
 DEF("sandbox", HAS_ARG, QEMU_OPTION_sandbox, \
-    "-sandbox <arg>  Enable seccomp mode 2 system call filter (default 'off').\n",
+    "-sandbox <arg>  Enable seccomp mode 2 system call filter (default 'off').\n"
+    "-sandbox on[,strict=<arg>]\n"
+    "                Enable seccomp mode 2 system call second level filter (default 'off').\n",
     QEMU_ARCH_ALL)
 STEXI
-@item -sandbox @var{arg}
+@item -sandbox @var{arg}[,strict=@var{value}]
 @findex -sandbox
 Enable Seccomp mode 2 system call filter. 'on' will enable syscall filtering and 'off' will
-disable it.  The default is 'off'.
+disable it.  The default is 'off'. 'strict=on' will enable second level filter (default is 'off').
 ETEXI
 
 DEF("readconfig", HAS_ARG, QEMU_OPTION_readconfig,
diff --git a/vl.c b/vl.c
index ee95674..ffdf460 100644
--- a/vl.c
+++ b/vl.c
@@ -330,6 +330,9 @@ static QemuOptsList qemu_sandbox_opts = {
         {
             .name = "enable",
             .type = QEMU_OPT_BOOL,
+        },{
+            .name = "strict",
+            .type = QEMU_OPT_STRING,
         },
         { /* end of list */ }
     },
@@ -1032,6 +1035,7 @@ static int bt_parse(const char *opt)
 
 static int parse_sandbox(QemuOpts *opts, void *opaque)
 {
+    const char *strict_value = NULL;
     /* FIXME: change this to true for 1.3 */
     if (qemu_opt_get_bool(opts, "enable", false)) {
 #ifdef CONFIG_SECCOMP
@@ -1040,6 +1044,17 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
                           "failed to install seccomp syscall filter in the kernel");
             return -1;
         }
+
+        strict_value = qemu_opt_get(opts, "strict");
+
+        if (!tap_enabled)
+           if (strict_value && !strcmp(strict_value, "on")) {
+               enable_blacklist = true;
+           }
+        } else {
+            fprintf(stderr, "Warning: seccomp syscall second level filter \"-sandbox on,strict=on\" "
+                    "cannot work together with \"-netdev tap\". Disabling it.\n");
+        }
 #else
         qerror_report(ERROR_CLASS_GENERIC_ERROR,
                       "sandboxing request but seccomp is not compiled into this build");
@@ -1769,7 +1784,7 @@ void vm_state_notify(int running, RunState state)
 
 static void install_seccomp_blacklist(void)
 {
-    if (enable_blacklist && !tap_enabled) {
+    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");
-- 
1.8.3.1

Re: [Qemu-devel] [PATCHv3 2/3] seccomp: adding command line support for blacklist

[Eduardo Otubo]

On 10/08/2013 09:42 PM, Eduardo Otubo wrote:
> v3: The options for blacklist in the command line also checkes the
> existence of "-netdev tap", leaving a warning message in a positive
> case.
>
> New command line options for the seccomp blacklist feature:
>
>   $ qemu -sandbox on[,strict=<on|off>]
>
> The strict parameter will turn on or off the new system call blacklist
>
> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
> ---
>   qemu-options.hx |  8 +++++---
>   vl.c            | 17 ++++++++++++++++-
>   2 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index d15338e..05485e1 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2978,13 +2978,15 @@ Old param mode (ARM only).
>   ETEXI
>
>   DEF("sandbox", HAS_ARG, QEMU_OPTION_sandbox, \
> -    "-sandbox <arg>  Enable seccomp mode 2 system call filter (default 'off').\n",
> +    "-sandbox <arg>  Enable seccomp mode 2 system call filter (default 'off').\n"
> +    "-sandbox on[,strict=<arg>]\n"
> +    "                Enable seccomp mode 2 system call second level filter (default 'off').\n",
>       QEMU_ARCH_ALL)
>   STEXI
> -@item -sandbox @var{arg}
> +@item -sandbox @var{arg}[,strict=@var{value}]
>   @findex -sandbox
>   Enable Seccomp mode 2 system call filter. 'on' will enable syscall filtering and 'off' will
> -disable it.  The default is 'off'.
> +disable it.  The default is 'off'. 'strict=on' will enable second level filter (default is 'off').
>   ETEXI
>
>   DEF("readconfig", HAS_ARG, QEMU_OPTION_readconfig,
> diff --git a/vl.c b/vl.c
> index ee95674..ffdf460 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -330,6 +330,9 @@ static QemuOptsList qemu_sandbox_opts = {
>           {
>               .name = "enable",
>               .type = QEMU_OPT_BOOL,
> +        },{
> +            .name = "strict",
> +            .type = QEMU_OPT_STRING,
>           },
>           { /* end of list */ }
>       },
> @@ -1032,6 +1035,7 @@ static int bt_parse(const char *opt)
>
>   static int parse_sandbox(QemuOpts *opts, void *opaque)
>   {
> +    const char *strict_value = NULL;
>       /* FIXME: change this to true for 1.3 */
>       if (qemu_opt_get_bool(opts, "enable", false)) {
>   #ifdef CONFIG_SECCOMP
> @@ -1040,6 +1044,17 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
>                             "failed to install seccomp syscall filter in the kernel");
>               return -1;
>           }
> +
> +        strict_value = qemu_opt_get(opts, "strict");
> +
> +        if (!tap_enabled)
                       .------^
Just spotted tha I erased this open brace in one of my rebases.

> +           if (strict_value && !strcmp(strict_value, "on")) {
> +               enable_blacklist = true;
> +           }
> +        } else {
> +            fprintf(stderr, "Warning: seccomp syscall second level filter \"-sandbox on,strict=on\" "
> +                    "cannot work together with \"-netdev tap\". Disabling it.\n");
> +        }
>   #else
>           qerror_report(ERROR_CLASS_GENERIC_ERROR,
>                         "sandboxing request but seccomp is not compiled into this build");
> @@ -1769,7 +1784,7 @@ void vm_state_notify(int running, RunState state)
>
>   static void install_seccomp_blacklist(void)
>   {
> -    if (enable_blacklist && !tap_enabled) {
> +    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");
>

-- 
Eduardo Otubo
IBM Linux Technology Center

[Qemu-devel] [PATCHv3 3/3] seccomp: general fixes

[Eduardo Otubo]

 1) On qemu-seccomp.c:255, the variable ctx was being used
uninitialized; now it's initialized with NULL and it's being checked at
the end of the function.

 2) Changed the name of the command line option from "enable" to
"sandbox" for a better understanding from user side.

Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
---
 qemu-seccomp.c | 4 ++--
 vl.c           | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/qemu-seccomp.c b/qemu-seccomp.c
index 84a42bc..fdd0de3 100644
--- a/qemu-seccomp.c
+++ b/qemu-seccomp.c
@@ -258,7 +258,7 @@ seccomp_return:
 int seccomp_start(int list_type)
 {
     int rc = 0;
-    scmp_filter_ctx ctx;
+    scmp_filter_ctx ctx = NULL;
 
     switch (list_type) {
     case WHITELIST:
@@ -285,7 +285,7 @@ int seccomp_start(int list_type)
 
     rc = seccomp_load(ctx);
 
-  seccomp_return:
+seccomp_return:
     if (ctx)
         seccomp_release(ctx);
     return rc;
diff --git a/vl.c b/vl.c
index ffdf460..f5106e6 100644
--- a/vl.c
+++ b/vl.c
@@ -324,11 +324,11 @@ static QemuOptsList qemu_rtc_opts = {
 
 static QemuOptsList qemu_sandbox_opts = {
     .name = "sandbox",
-    .implied_opt_name = "enable",
+    .implied_opt_name = "sandbox",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_sandbox_opts.head),
     .desc = {
         {
-            .name = "enable",
+            .name = "sandbox",
             .type = QEMU_OPT_BOOL,
         },{
             .name = "strict",
@@ -1037,7 +1037,7 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
 {
     const char *strict_value = NULL;
     /* FIXME: change this to true for 1.3 */
-    if (qemu_opt_get_bool(opts, "enable", false)) {
+    if (qemu_opt_get_bool(opts, "sandbox", false)) {
 #ifdef CONFIG_SECCOMP
         if (seccomp_start(WHITELIST) < 0) {
             qerror_report(ERROR_CLASS_GENERIC_ERROR,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCHv3 3/3] seccomp: general fixes

[Paul Moore]

On Tuesday, October 08, 2013 09:42:26 PM Eduardo Otubo wrote:
>  1) On qemu-seccomp.c:255, the variable ctx was being used
> uninitialized; now it's initialized with NULL and it's being checked at
> the end of the function.
> 
>  2) Changed the name of the command line option from "enable" to
> "sandbox" for a better understanding from user side.
> 
> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
> ---
>  qemu-seccomp.c | 4 ++--
>  vl.c           | 6 +++---
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/qemu-seccomp.c b/qemu-seccomp.c
> index 84a42bc..fdd0de3 100644
> --- a/qemu-seccomp.c
> +++ b/qemu-seccomp.c
> @@ -258,7 +258,7 @@ seccomp_return:
>  int seccomp_start(int list_type)
>  {
>      int rc = 0;
> -    scmp_filter_ctx ctx;
> +    scmp_filter_ctx ctx = NULL;
> 
>      switch (list_type) {
>      case WHITELIST:
> @@ -285,7 +285,7 @@ int seccomp_start(int list_type)
> 
>      rc = seccomp_load(ctx);
> 
> -  seccomp_return:
> +seccomp_return:
>      if (ctx)
>          seccomp_release(ctx);
>      return rc;

Any particular reason these changes weren't folded into patch 1/3?

-- 
paul moore
security and virtualization @ redhat

[Qemu-devel] [PATCHv2 0/3] seccomp: adding blacklist support with command line

[Eduardo Otubo]

v2: The blacklist works exactly the opposite as the whitelist. I set the
default behaiour to SCMP_ACT_ALLOW and the exceptions to SCMP_ACT_KILL;
remembering it inherits the behavior from the previous installed whitelist.

This patch series also contain the command line support for this feature and
some minor fixes, all of them described in their own commit messages.

v1: The second whitelist is installed right before the vcpu starts, it contains all
the system calls the first one has except for exec() and select(), which are
big major syscalls that I could extensively test with virt-test and do not
cause any damage to the general execution.

The environment in which the second whitelist is installed seems to need less
system calls than the first, so the procedure here will be the same: Keep
testing with virt-test and get to the smallest list as possible.


Eduardo Otubo (3):
  seccomp: adding blacklist support
  seccomp: adding command line support for blacklist
  seccomp: general fixes

 include/sysemu/seccomp.h |  5 +++-
 qemu-options.hx          |  8 ++++---
 qemu-seccomp.c           | 60 +++++++++++++++++++++++++++++++++++++-----------
 vl.c                     | 31 +++++++++++++++++++++----
 4 files changed, 83 insertions(+), 21 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCHv3 3/3] seccomp: general fixes

[Eduardo Otubo]

 1) On qemu-seccomp.c:255, the variable ctx was being used
uninitialized; now it's initialized with NULL and it's being checked at
the end of the function.

 2) Changed the name of the command line option from "enable" to
"sandbox" for a better understanding from user side.

Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
---
 qemu-seccomp.c | 5 +++--
 vl.c           | 6 +++---
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/qemu-seccomp.c b/qemu-seccomp.c
index 5e85eb5..f39d636 100644
--- a/qemu-seccomp.c
+++ b/qemu-seccomp.c
@@ -252,7 +252,7 @@ seccomp_return:
 int seccomp_start(int list_type)
 {
     int rc = 0;
-    scmp_filter_ctx ctx;
+    scmp_filter_ctx ctx = NULL;
 
     switch (list_type) {
     case WHITELIST:
@@ -280,6 +280,7 @@ int seccomp_start(int list_type)
     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 909f685..129919d 100644
--- a/vl.c
+++ b/vl.c
@@ -323,11 +323,11 @@ static QemuOptsList qemu_rtc_opts = {
 
 static QemuOptsList qemu_sandbox_opts = {
     .name = "sandbox",
-    .implied_opt_name = "enable",
+    .implied_opt_name = "sandbox",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_sandbox_opts.head),
     .desc = {
         {
-            .name = "enable",
+            .name = "sandbox",
             .type = QEMU_OPT_BOOL,
         },{
             .name = "strict",
@@ -1036,7 +1036,7 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
 {
     const char * strict_value = NULL;
     /* FIXME: change this to true for 1.3 */
-    if (qemu_opt_get_bool(opts, "enable", false)) {
+    if (qemu_opt_get_bool(opts, "sandbox", false)) {
 #ifdef CONFIG_SECCOMP
         if (seccomp_start(WHITELIST) < 0) {
             qerror_report(ERROR_CLASS_GENERIC_ERROR,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCHv3 3/3] seccomp: general fixes

[Corey Bryant]

On 09/06/2013 03:21 PM, Eduardo Otubo wrote:
>   1) On qemu-seccomp.c:255, the variable ctx was being used
> uninitialized; now it's initialized with NULL and it's being checked at
> the end of the function.
>
>   2) Changed the name of the command line option from "enable" to
> "sandbox" for a better understanding from user side.
>
> Signed-off-by: Eduardo Otubo<otubo@linux.vnet.ibm.com>
> ---
>   qemu-seccomp.c | 5 +++--
>   vl.c           | 6 +++---
>   2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/qemu-seccomp.c b/qemu-seccomp.c
> index 5e85eb5..f39d636 100644
> --- a/qemu-seccomp.c
> +++ b/qemu-seccomp.c
> @@ -252,7 +252,7 @@ seccomp_return:
>   int seccomp_start(int list_type)
>   {
>       int rc = 0;
> -    scmp_filter_ctx ctx;
> +    scmp_filter_ctx ctx = NULL;
>
>       switch (list_type) {
>       case WHITELIST:
> @@ -280,6 +280,7 @@ int seccomp_start(int list_type)
>       rc = seccomp_load(ctx);
>
>   seccomp_return:
> -    seccomp_release(ctx);
> +    if (!ctx)

You need to remove the ! from this check.

> +        seccomp_release(ctx);
>       return rc;
>   }
> diff --git a/vl.c b/vl.c
> index 909f685..129919d 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -323,11 +323,11 @@ static QemuOptsList qemu_rtc_opts = {
>
>   static QemuOptsList qemu_sandbox_opts = {
>       .name = "sandbox",
> -    .implied_opt_name = "enable",
> +    .implied_opt_name = "sandbox",

So does this technically make it -sandbox,sandbox=on?  If I understand 
correctly, I don't think the implied option is ever seen or used by the 
user anyway so it probably doesn't matter.  But I don't know if it's 
worth changing.

>       .head = QTAILQ_HEAD_INITIALIZER(qemu_sandbox_opts.head),
>       .desc = {
>           {
> -            .name = "enable",
> +            .name = "sandbox",
>               .type = QEMU_OPT_BOOL,
>           },{
>               .name = "strict",
> @@ -1036,7 +1036,7 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
>   {
>       const char * strict_value = NULL;
>       /* FIXME: change this to true for 1.3 */
> -    if (qemu_opt_get_bool(opts, "enable", false)) {
> +    if (qemu_opt_get_bool(opts, "sandbox", false)) {
>   #ifdef CONFIG_SECCOMP
>           if (seccomp_start(WHITELIST) < 0) {
>               qerror_report(ERROR_CLASS_GENERIC_ERROR,
> -- 1.8.3.1
>

-- 
Regards,
Corey Bryant

Re: [Qemu-devel] [PATCHv3 3/3] seccomp: general fixes

[Eduardo Otubo]

On 09/11/2013 01:56 PM, Corey Bryant wrote:
>
>
> On 09/06/2013 03:21 PM, Eduardo Otubo wrote:
>>   1) On qemu-seccomp.c:255, the variable ctx was being used
>> uninitialized; now it's initialized with NULL and it's being checked at
>> the end of the function.
>>
>>   2) Changed the name of the command line option from "enable" to
>> "sandbox" for a better understanding from user side.
>>
>> Signed-off-by: Eduardo Otubo<otubo@linux.vnet.ibm.com>
>> ---
>>   qemu-seccomp.c | 5 +++--
>>   vl.c           | 6 +++---
>>   2 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/qemu-seccomp.c b/qemu-seccomp.c
>> index 5e85eb5..f39d636 100644
>> --- a/qemu-seccomp.c
>> +++ b/qemu-seccomp.c
>> @@ -252,7 +252,7 @@ seccomp_return:
>>   int seccomp_start(int list_type)
>>   {
>>       int rc = 0;
>> -    scmp_filter_ctx ctx;
>> +    scmp_filter_ctx ctx = NULL;
>>
>>       switch (list_type) {
>>       case WHITELIST:
>> @@ -280,6 +280,7 @@ int seccomp_start(int list_type)
>>       rc = seccomp_load(ctx);
>>
>>   seccomp_return:
>> -    seccomp_release(ctx);
>> +    if (!ctx)
>
> You need to remove the ! from this check.
>
>> +        seccomp_release(ctx);
>>       return rc;
>>   }
>> diff --git a/vl.c b/vl.c
>> index 909f685..129919d 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -323,11 +323,11 @@ static QemuOptsList qemu_rtc_opts = {
>>
>>   static QemuOptsList qemu_sandbox_opts = {
>>       .name = "sandbox",
>> -    .implied_opt_name = "enable",
>> +    .implied_opt_name = "sandbox",
>
> So does this technically make it -sandbox,sandbox=on?If I understand

No. Qemu command line options is a little tricky and I had to spent some 
time to understand it. It actually make "-sandbox on,strict=on"

> correctly, I don't think the implied option is ever seen or used by the
> user anyway so it probably doesn't matter.  But I don't know if it's
> worth changing.

I changed the name so I can remember how it works in the future, since 
it's not that trivial.

>
>>       .head = QTAILQ_HEAD_INITIALIZER(qemu_sandbox_opts.head),
>>       .desc = {
>>           {
>> -            .name = "enable",
>> +            .name = "sandbox",
>>               .type = QEMU_OPT_BOOL,
>>           },{
>>               .name = "strict",
>> @@ -1036,7 +1036,7 @@ static int parse_sandbox(QemuOpts *opts, void
>> *opaque)
>>   {
>>       const char * strict_value = NULL;
>>       /* FIXME: change this to true for 1.3 */
>> -    if (qemu_opt_get_bool(opts, "enable", false)) {
>> +    if (qemu_opt_get_bool(opts, "sandbox", false)) {
>>   #ifdef CONFIG_SECCOMP
>>           if (seccomp_start(WHITELIST) < 0) {
>>               qerror_report(ERROR_CLASS_GENERIC_ERROR,
>> -- 1.8.3.1
>>
>

-- 
Eduardo Otubo
IBM Linux Technology Center