[Qemu-devel] [PATCH 1/4] Adding new syscalls (bugzilla 855162)

[Qemu-devel] [PATCH 1/4] Adding new syscalls (bugzilla 855162)

[Eduardo Otubo]

According to the bug 855162[0] - there's the need of adding new syscalls
to the whitelist whenn using Qemu with Libvirt.

[1] - https://bugzilla.redhat.com/show_bug.cgi?id=855162

Reported-by: Paul Moore <pmoore@redhat.com>
Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
---
 qemu-seccomp.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/qemu-seccomp.c b/qemu-seccomp.c
index 64329a3..a25f2fa 100644
--- a/qemu-seccomp.c
+++ b/qemu-seccomp.c
@@ -45,6 +45,13 @@ static const struct QemuSeccompSyscall seccomp_whitelist[] = {
     { SCMP_SYS(access), 245 },
     { SCMP_SYS(prctl), 245 },
     { SCMP_SYS(signalfd), 245 },
+    { SCMP_SYS(getrlimit), 245 },
+    { SCMP_SYS(set_tid_address), 245 },
+    { SCMP_SYS(socketpair), 245 },
+    { SCMP_SYS(statfs), 245 },
+    { SCMP_SYS(unlink), 245 },
+    { SCMP_SYS(wait4), 245 },
+    { SCMP_SYS(getuid), 245 },
 #if defined(__i386__)
     { SCMP_SYS(fcntl64), 245 },
     { SCMP_SYS(fstat64), 245 },
@@ -107,7 +114,8 @@ static const struct QemuSeccompSyscall seccomp_whitelist[] = {
     { SCMP_SYS(getsockname), 242 },
     { SCMP_SYS(getpeername), 242 },
     { SCMP_SYS(fdatasync), 242 },
-    { SCMP_SYS(close), 242 }
+    { SCMP_SYS(close), 242 },
+    { SCMP_SYS(accept4), 242 }
 };
 
 int seccomp_start(void)
-- 
1.7.12

[Qemu-devel] [PATCH 2/4] Setting "-sandbox on" as deafult

[Eduardo Otubo]

Now the seccomp filter will be set to "on" even if no argument
"-sandbox" is given.

Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
---
 configure |  2 +-
 vl.c      | 38 +++++++++++++++++++++++++++-----------
 2 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/configure b/configure
index 353d788..c613a51 100755
--- a/configure
+++ b/configure
@@ -220,7 +220,7 @@ guest_agent="yes"
 want_tools="yes"
 libiscsi=""
 coroutine=""
-seccomp=""
+seccomp="yes"
 glusterfs=""
 
 # parse CC options first
diff --git a/vl.c b/vl.c
index 5b357a3..bec68cd 100644
--- a/vl.c
+++ b/vl.c
@@ -276,6 +276,10 @@ static int default_cdrom = 1;
 static int default_sdcard = 1;
 static int default_vga = 1;
 
+#ifdef CONFIG_SECCOMP
+bool seccomp_on = true;
+#endif
+
 static struct {
     const char *driver;
     int *flag;
@@ -770,23 +774,28 @@ static int bt_parse(const char *opt)
     return 1;
 }
 
-static int parse_sandbox(QemuOpts *opts, void *opaque)
+static int install_seccomp_filters(void)
 {
-    /* FIXME: change this to true for 1.3 */
-    if (qemu_opt_get_bool(opts, "enable", false)) {
 #ifdef CONFIG_SECCOMP
-        if (seccomp_start() < 0) {
-            qerror_report(ERROR_CLASS_GENERIC_ERROR,
-                          "failed to install seccomp syscall filter in the kernel");
-            return -1;
-        }
-#else
+    if (seccomp_start() < 0) {
         qerror_report(ERROR_CLASS_GENERIC_ERROR,
-                      "sandboxing request but seccomp is not compiled into this build");
+                "failed to install seccomp syscall filter in the kernel");
         return -1;
-#endif
     }
+#else
+    qerror_report(ERROR_CLASS_GENERIC_ERROR,
+            "sandboxing requested but seccomp is not compiled into this build");
+    return -1;
+#endif
+    return 0;
+}
+
 
+static int parse_sandbox(QemuOpts *opts, void *opaque)
+{
+    if (!qemu_opt_get_bool(opts, "enable", true)) {
+        seccomp_on = false;
+    }
     return 0;
 }
 
@@ -3320,6 +3329,13 @@ int main(int argc, char **argv, char **envp)
         exit(1);
     }
 
+    /* We should install seccomp filters even if -sandbox on is not used. */
+    if (seccomp_on) {
+        if (install_seccomp_filters() < 0) {
+            exit(1);
+        }
+    }
+
     if (machine == NULL) {
         fprintf(stderr, "No machine found.\n");
         exit(1);
-- 
1.7.12

Re: [Qemu-devel] [PATCH 2/4] Setting "-sandbox on" as deafult

[Corey Bryant]

I think it's worth nothing that Eduardo is planning to submit a separate 
patch providing (commented out?) code that will allow developers to 
easily determine the syscalls that need to be added to the whitelist. 
That is, if QEMU is being killed by seccomp due to disallowed syscall usage.

-- 
Regards,
Corey Bryant

[Qemu-devel] [PATCH 3/4] Support for "double whitelist" filters

[Eduardo Otubo]

This patch includes a second whitelist right before the main loop. It's
a smaller and more restricted whitelist, excluding execve() among many
others.

Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
---
 qemu-seccomp.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
 qemu-seccomp.h |  7 ++++-
 vl.c           | 13 +++++++-
 3 files changed, 103 insertions(+), 11 deletions(-)

diff --git a/qemu-seccomp.c b/qemu-seccomp.c
index a25f2fa..9c68af5 100644
--- a/qemu-seccomp.c
+++ b/qemu-seccomp.c
@@ -13,6 +13,7 @@
  * GNU GPL, version 2 or (at your option) any later version.
  */
 #include <stdio.h>
+#include <stdlib.h>
 #include <seccomp.h>
 #include "qemu-seccomp.h"
 
@@ -21,7 +22,7 @@ struct QemuSeccompSyscall {
     uint8_t priority;
 };
 
-static const struct QemuSeccompSyscall seccomp_whitelist[] = {
+static const struct QemuSeccompSyscall seccomp_whitelist_init[] = {
     { SCMP_SYS(timer_settime), 255 },
     { SCMP_SYS(timer_gettime), 254 },
     { SCMP_SYS(futex), 253 },
@@ -118,27 +119,102 @@ static const struct QemuSeccompSyscall seccomp_whitelist[] = {
     { SCMP_SYS(accept4), 242 }
 };
 
-int seccomp_start(void)
+static const struct QemuSeccompSyscall seccomp_whitelist_main_loop[] = {
+    { SCMP_SYS(timer_settime), 255 },
+    { SCMP_SYS(timer_gettime), 254 },
+    { SCMP_SYS(futex), 253 },
+    { SCMP_SYS(select), 252 },
+    { SCMP_SYS(recvfrom), 251 },
+    { SCMP_SYS(sendto), 250 },
+    { SCMP_SYS(read), 249 },
+    { SCMP_SYS(brk), 248 },
+    { SCMP_SYS(mmap), 247 },
+#if defined(__i386__)
+    { SCMP_SYS(fcntl64), 245 },
+    { SCMP_SYS(fstat64), 245 },
+    { SCMP_SYS(stat64), 245 },
+    { SCMP_SYS(getgid32), 245 },
+    { SCMP_SYS(getegid32), 245 },
+    { SCMP_SYS(getuid32), 245 },
+    { SCMP_SYS(geteuid32), 245 },
+    { SCMP_SYS(sigreturn), 245 },
+    { SCMP_SYS(_newselect), 245 },
+    { SCMP_SYS(_llseek), 245 },
+    { SCMP_SYS(mmap2), 245},
+    { SCMP_SYS(sigprocmask), 245 },
+#endif
+    { SCMP_SYS(exit), 245 },
+    { SCMP_SYS(timer_delete), 245 },
+    { SCMP_SYS(exit_group), 245 },
+    { SCMP_SYS(rt_sigreturn), 245 },
+    { SCMP_SYS(madvise), 245 },
+    { SCMP_SYS(write), 244 },
+    { SCMP_SYS(fcntl), 243 },
+    { SCMP_SYS(tgkill), 242 },
+    { SCMP_SYS(rt_sigaction), 242 },
+    { SCMP_SYS(pipe2), 242 },
+    { SCMP_SYS(munmap), 242 },
+    { SCMP_SYS(mremap), 242 },
+    { SCMP_SYS(getsockname), 242 },
+    { SCMP_SYS(getpeername), 242 },
+    { SCMP_SYS(close), 242 },
+    { SCMP_SYS(accept4), 242 }
+};
+
+static int
+process_whitelist(const struct QemuSeccompSyscall *whitelist,
+                  unsigned int size, scmp_filter_ctx *ctx)
 {
     int rc = 0;
+
     unsigned int i = 0;
-    scmp_filter_ctx ctx;
+
+    for (i = 0; i < size; i++) {
+        rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, whitelist[i].num, 0);
+        if (rc < 0) {
+            return -1;
+        }
+
+        rc = seccomp_syscall_priority(ctx, whitelist[i].num,
+                                      whitelist[i].priority);
+        if (rc < 0) {
+            return -1;
+        }
+    }
+    return 0;
+}
+
+int
+seccomp_start(enum whitelist_mode mode, scmp_filter_ctx *ctx)
+{
+    int rc = 0;
 
     ctx = seccomp_init(SCMP_ACT_KILL);
     if (ctx == NULL) {
+        rc = -1;
         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);
-        if (rc < 0) {
+    switch (mode) {
+    case INIT:
+        if (process_whitelist
+            (seccomp_whitelist_init,
+             ARRAY_SIZE(seccomp_whitelist_init), ctx) < 0) {
+            rc = -1;
             goto seccomp_return;
         }
-        rc = seccomp_syscall_priority(ctx, seccomp_whitelist[i].num,
-                                      seccomp_whitelist[i].priority);
-        if (rc < 0) {
+        break;
+    case MAIN_LOOP:
+        if (process_whitelist
+            (seccomp_whitelist_main_loop,
+             ARRAY_SIZE(seccomp_whitelist_main_loop), ctx) < 0) {
+            rc = -1;
             goto seccomp_return;
         }
+        break;
+    default:
+        rc = -1;
+        goto seccomp_return;
     }
 
     rc = seccomp_load(ctx);
diff --git a/qemu-seccomp.h b/qemu-seccomp.h
index b2fc3f8..1c97978 100644
--- a/qemu-seccomp.h
+++ b/qemu-seccomp.h
@@ -18,5 +18,10 @@
 #include <seccomp.h>
 #include "osdep.h"
 
-int seccomp_start(void);
+enum whitelist_mode {
+    INIT = 0,
+    MAIN_LOOP = 1,
+};
+
+int seccomp_start(enum whitelist_mode mode, scmp_filter_ctx *ctx);
 #endif
diff --git a/vl.c b/vl.c
index bec68cd..773d488 100644
--- a/vl.c
+++ b/vl.c
@@ -278,6 +278,7 @@ static int default_vga = 1;
 
 #ifdef CONFIG_SECCOMP
 bool seccomp_on = true;
+scmp_filter_ctx ctx;
 #endif
 
 static struct {
@@ -777,7 +778,7 @@ static int bt_parse(const char *opt)
 static int install_seccomp_filters(void)
 {
 #ifdef CONFIG_SECCOMP
-    if (seccomp_start() < 0) {
+    if (seccomp_start(INIT, &ctx) < 0) {
         qerror_report(ERROR_CLASS_GENERIC_ERROR,
                 "failed to install seccomp syscall filter in the kernel");
         return -1;
@@ -3794,6 +3795,16 @@ int main(int argc, char **argv, char **envp)
 
     os_setup_post();
 
+    if (seccomp_on) {
+#ifdef CONFIG_SECCOMP
+        if (seccomp_start(MAIN_LOOP, &ctx) < 0) {
+            qerror_report(ERROR_CLASS_GENERIC_ERROR,
+                          "failed to install seccomp syscall filter in the kernel");
+            return -1;
+        }
+#endif
+    }
+
     resume_all_vcpus();
     main_loop();
     bdrv_close_all();
-- 
1.7.12

Re: [Qemu-devel] [PATCH 3/4] Support for "double whitelist" filters

[Blue Swirl]

On Wed, Oct 17, 2012 at 1:15 PM, Eduardo Otubo <otubo@linux.vnet.ibm.com> wrote:
> This patch includes a second whitelist right before the main loop. It's
> a smaller and more restricted whitelist, excluding execve() among many
> others.
>
> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
> ---
>  qemu-seccomp.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
>  qemu-seccomp.h |  7 ++++-
>  vl.c           | 13 +++++++-
>  3 files changed, 103 insertions(+), 11 deletions(-)
>
> diff --git a/qemu-seccomp.c b/qemu-seccomp.c
> index a25f2fa..9c68af5 100644
> --- a/qemu-seccomp.c
> +++ b/qemu-seccomp.c
> @@ -13,6 +13,7 @@
>   * GNU GPL, version 2 or (at your option) any later version.
>   */
>  #include <stdio.h>
> +#include <stdlib.h>
>  #include <seccomp.h>
>  #include "qemu-seccomp.h"
>
> @@ -21,7 +22,7 @@ struct QemuSeccompSyscall {
>      uint8_t priority;
>  };
>
> -static const struct QemuSeccompSyscall seccomp_whitelist[] = {
> +static const struct QemuSeccompSyscall seccomp_whitelist_init[] = {
>      { SCMP_SYS(timer_settime), 255 },
>      { SCMP_SYS(timer_gettime), 254 },
>      { SCMP_SYS(futex), 253 },
> @@ -118,27 +119,102 @@ static const struct QemuSeccompSyscall seccomp_whitelist[] = {
>      { SCMP_SYS(accept4), 242 }
>  };
>
> -int seccomp_start(void)
> +static const struct QemuSeccompSyscall seccomp_whitelist_main_loop[] = {
> +    { SCMP_SYS(timer_settime), 255 },
> +    { SCMP_SYS(timer_gettime), 254 },
> +    { SCMP_SYS(futex), 253 },
> +    { SCMP_SYS(select), 252 },
> +    { SCMP_SYS(recvfrom), 251 },
> +    { SCMP_SYS(sendto), 250 },
> +    { SCMP_SYS(read), 249 },
> +    { SCMP_SYS(brk), 248 },
> +    { SCMP_SYS(mmap), 247 },
> +#if defined(__i386__)
> +    { SCMP_SYS(fcntl64), 245 },
> +    { SCMP_SYS(fstat64), 245 },
> +    { SCMP_SYS(stat64), 245 },
> +    { SCMP_SYS(getgid32), 245 },
> +    { SCMP_SYS(getegid32), 245 },
> +    { SCMP_SYS(getuid32), 245 },
> +    { SCMP_SYS(geteuid32), 245 },
> +    { SCMP_SYS(sigreturn), 245 },
> +    { SCMP_SYS(_newselect), 245 },
> +    { SCMP_SYS(_llseek), 245 },
> +    { SCMP_SYS(mmap2), 245},
> +    { SCMP_SYS(sigprocmask), 245 },
> +#endif
> +    { SCMP_SYS(exit), 245 },
> +    { SCMP_SYS(timer_delete), 245 },
> +    { SCMP_SYS(exit_group), 245 },
> +    { SCMP_SYS(rt_sigreturn), 245 },
> +    { SCMP_SYS(madvise), 245 },
> +    { SCMP_SYS(write), 244 },
> +    { SCMP_SYS(fcntl), 243 },
> +    { SCMP_SYS(tgkill), 242 },
> +    { SCMP_SYS(rt_sigaction), 242 },
> +    { SCMP_SYS(pipe2), 242 },
> +    { SCMP_SYS(munmap), 242 },
> +    { SCMP_SYS(mremap), 242 },
> +    { SCMP_SYS(getsockname), 242 },
> +    { SCMP_SYS(getpeername), 242 },
> +    { SCMP_SYS(close), 242 },
> +    { SCMP_SYS(accept4), 242 }

It's nice to see that for example open, creat, unlink, socket, bind,
mprotect, setrlimit and kill are not present.

> +};
> +
> +static int
> +process_whitelist(const struct QemuSeccompSyscall *whitelist,
> +                  unsigned int size, scmp_filter_ctx *ctx)
>  {
>      int rc = 0;
> +
>      unsigned int i = 0;
> -    scmp_filter_ctx ctx;
> +
> +    for (i = 0; i < size; i++) {
> +        rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, whitelist[i].num, 0);
> +        if (rc < 0) {
> +            return -1;
> +        }
> +
> +        rc = seccomp_syscall_priority(ctx, whitelist[i].num,
> +                                      whitelist[i].priority);
> +        if (rc < 0) {
> +            return -1;
> +        }
> +    }
> +    return 0;
> +}
> +
> +int
> +seccomp_start(enum whitelist_mode mode, scmp_filter_ctx *ctx)
> +{
> +    int rc = 0;
>
>      ctx = seccomp_init(SCMP_ACT_KILL);
>      if (ctx == NULL) {
> +        rc = -1;
>          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);
> -        if (rc < 0) {
> +    switch (mode) {
> +    case INIT:
> +        if (process_whitelist
> +            (seccomp_whitelist_init,
> +             ARRAY_SIZE(seccomp_whitelist_init), ctx) < 0) {
> +            rc = -1;
>              goto seccomp_return;
>          }
> -        rc = seccomp_syscall_priority(ctx, seccomp_whitelist[i].num,
> -                                      seccomp_whitelist[i].priority);
> -        if (rc < 0) {
> +        break;
> +    case MAIN_LOOP:
> +        if (process_whitelist
> +            (seccomp_whitelist_main_loop,
> +             ARRAY_SIZE(seccomp_whitelist_main_loop), ctx) < 0) {
> +            rc = -1;
>              goto seccomp_return;
>          }
> +        break;
> +    default:
> +        rc = -1;
> +        goto seccomp_return;
>      }
>
>      rc = seccomp_load(ctx);
> diff --git a/qemu-seccomp.h b/qemu-seccomp.h
> index b2fc3f8..1c97978 100644
> --- a/qemu-seccomp.h
> +++ b/qemu-seccomp.h
> @@ -18,5 +18,10 @@
>  #include <seccomp.h>
>  #include "osdep.h"
>
> -int seccomp_start(void);
> +enum whitelist_mode {
> +    INIT = 0,
> +    MAIN_LOOP = 1,
> +};
> +
> +int seccomp_start(enum whitelist_mode mode, scmp_filter_ctx *ctx);
>  #endif
> diff --git a/vl.c b/vl.c
> index bec68cd..773d488 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -278,6 +278,7 @@ static int default_vga = 1;
>
>  #ifdef CONFIG_SECCOMP
>  bool seccomp_on = true;
> +scmp_filter_ctx ctx;

This should be a local variable to main(), maybe also named
'main_loop_ctx' so we can add further contexts.

>  #endif
>
>  static struct {
> @@ -777,7 +778,7 @@ static int bt_parse(const char *opt)
>  static int install_seccomp_filters(void)
>  {
>  #ifdef CONFIG_SECCOMP
> -    if (seccomp_start() < 0) {
> +    if (seccomp_start(INIT, &ctx) < 0) {
>          qerror_report(ERROR_CLASS_GENERIC_ERROR,
>                  "failed to install seccomp syscall filter in the kernel");
>          return -1;
> @@ -3794,6 +3795,16 @@ int main(int argc, char **argv, char **envp)
>
>      os_setup_post();
>
> +    if (seccomp_on) {

'seccomp_on' is only available with CONFIG_SECCOMP, so this would break build.

> +#ifdef CONFIG_SECCOMP
> +        if (seccomp_start(MAIN_LOOP, &ctx) < 0) {
> +            qerror_report(ERROR_CLASS_GENERIC_ERROR,
> +                          "failed to install seccomp syscall filter in the kernel");

This error message could be different from the first one.

> +            return -1;
> +        }
> +#endif
> +    }
> +
>      resume_all_vcpus();
>      main_loop();
>      bdrv_close_all();
> --
> 1.7.12
>
>

Re: [Qemu-devel] [PATCH 3/4] Support for "double whitelist" filters

[Corey Bryant]

On 10/19/2012 01:04 PM, Blue Swirl wrote:
> On Wed, Oct 17, 2012 at 1:15 PM, Eduardo Otubo <otubo@linux.vnet.ibm.com> wrote:
>> This patch includes a second whitelist right before the main loop. It's
>> a smaller and more restricted whitelist, excluding execve() among many
>> others.
>>
>> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
>> ---
>>   qemu-seccomp.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
>>   qemu-seccomp.h |  7 ++++-
>>   vl.c           | 13 +++++++-
>>   3 files changed, 103 insertions(+), 11 deletions(-)
>>
>> diff --git a/qemu-seccomp.c b/qemu-seccomp.c
>> index a25f2fa..9c68af5 100644
>> --- a/qemu-seccomp.c
>> +++ b/qemu-seccomp.c
>> @@ -13,6 +13,7 @@
>>    * GNU GPL, version 2 or (at your option) any later version.
>>    */
>>   #include <stdio.h>
>> +#include <stdlib.h>
>>   #include <seccomp.h>
>>   #include "qemu-seccomp.h"
>>
>> @@ -21,7 +22,7 @@ struct QemuSeccompSyscall {
>>       uint8_t priority;
>>   };
>>
>> -static const struct QemuSeccompSyscall seccomp_whitelist[] = {
>> +static const struct QemuSeccompSyscall seccomp_whitelist_init[] = {
>>       { SCMP_SYS(timer_settime), 255 },
>>       { SCMP_SYS(timer_gettime), 254 },
>>       { SCMP_SYS(futex), 253 },
>> @@ -118,27 +119,102 @@ static const struct QemuSeccompSyscall seccomp_whitelist[] = {
>>       { SCMP_SYS(accept4), 242 }
>>   };
>>
>> -int seccomp_start(void)
>> +static const struct QemuSeccompSyscall seccomp_whitelist_main_loop[] = {
>> +    { SCMP_SYS(timer_settime), 255 },
>> +    { SCMP_SYS(timer_gettime), 254 },
>> +    { SCMP_SYS(futex), 253 },
>> +    { SCMP_SYS(select), 252 },
>> +    { SCMP_SYS(recvfrom), 251 },
>> +    { SCMP_SYS(sendto), 250 },
>> +    { SCMP_SYS(read), 249 },
>> +    { SCMP_SYS(brk), 248 },
>> +    { SCMP_SYS(mmap), 247 },
>> +#if defined(__i386__)
>> +    { SCMP_SYS(fcntl64), 245 },
>> +    { SCMP_SYS(fstat64), 245 },
>> +    { SCMP_SYS(stat64), 245 },
>> +    { SCMP_SYS(getgid32), 245 },
>> +    { SCMP_SYS(getegid32), 245 },
>> +    { SCMP_SYS(getuid32), 245 },
>> +    { SCMP_SYS(geteuid32), 245 },
>> +    { SCMP_SYS(sigreturn), 245 },
>> +    { SCMP_SYS(_newselect), 245 },
>> +    { SCMP_SYS(_llseek), 245 },
>> +    { SCMP_SYS(mmap2), 245},
>> +    { SCMP_SYS(sigprocmask), 245 },
>> +#endif
>> +    { SCMP_SYS(exit), 245 },
>> +    { SCMP_SYS(timer_delete), 245 },
>> +    { SCMP_SYS(exit_group), 245 },
>> +    { SCMP_SYS(rt_sigreturn), 245 },
>> +    { SCMP_SYS(madvise), 245 },
>> +    { SCMP_SYS(write), 244 },
>> +    { SCMP_SYS(fcntl), 243 },
>> +    { SCMP_SYS(tgkill), 242 },
>> +    { SCMP_SYS(rt_sigaction), 242 },
>> +    { SCMP_SYS(pipe2), 242 },
>> +    { SCMP_SYS(munmap), 242 },
>> +    { SCMP_SYS(mremap), 242 },
>> +    { SCMP_SYS(getsockname), 242 },
>> +    { SCMP_SYS(getpeername), 242 },
>> +    { SCMP_SYS(close), 242 },
>> +    { SCMP_SYS(accept4), 242 }
>
> It's nice to see that for example open, creat, unlink, socket, bind,
> mprotect, setrlimit and kill are not present.
>

Hmm, well open minimally needs to be added to this list so that drives 
can be hotplugged.

>> +};
>> +
>> +static int
>> +process_whitelist(const struct QemuSeccompSyscall *whitelist,
>> +                  unsigned int size, scmp_filter_ctx *ctx)
>>   {
>>       int rc = 0;
>> +
>>       unsigned int i = 0;
>> -    scmp_filter_ctx ctx;
>> +
>> +    for (i = 0; i < size; i++) {
>> +        rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, whitelist[i].num, 0);
>> +        if (rc < 0) {
>> +            return -1;
>> +        }
>> +
>> +        rc = seccomp_syscall_priority(ctx, whitelist[i].num,
>> +                                      whitelist[i].priority);
>> +        if (rc < 0) {
>> +            return -1;
>> +        }
>> +    }
>> +    return 0;
>> +}
>> +
>> +int
>> +seccomp_start(enum whitelist_mode mode, scmp_filter_ctx *ctx)
>> +{
>> +    int rc = 0;
>>
>>       ctx = seccomp_init(SCMP_ACT_KILL);
>>       if (ctx == NULL) {
>> +        rc = -1;
>>           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);
>> -        if (rc < 0) {
>> +    switch (mode) {
>> +    case INIT:
>> +        if (process_whitelist
>> +            (seccomp_whitelist_init,
>> +             ARRAY_SIZE(seccomp_whitelist_init), ctx) < 0) {
>> +            rc = -1;
>>               goto seccomp_return;
>>           }
>> -        rc = seccomp_syscall_priority(ctx, seccomp_whitelist[i].num,
>> -                                      seccomp_whitelist[i].priority);
>> -        if (rc < 0) {
>> +        break;
>> +    case MAIN_LOOP:
>> +        if (process_whitelist
>> +            (seccomp_whitelist_main_loop,
>> +             ARRAY_SIZE(seccomp_whitelist_main_loop), ctx) < 0) {
>> +            rc = -1;
>>               goto seccomp_return;
>>           }
>> +        break;
>> +    default:
>> +        rc = -1;
>> +        goto seccomp_return;
>>       }
>>
>>       rc = seccomp_load(ctx);
>> diff --git a/qemu-seccomp.h b/qemu-seccomp.h
>> index b2fc3f8..1c97978 100644
>> --- a/qemu-seccomp.h
>> +++ b/qemu-seccomp.h
>> @@ -18,5 +18,10 @@
>>   #include <seccomp.h>
>>   #include "osdep.h"
>>
>> -int seccomp_start(void);
>> +enum whitelist_mode {
>> +    INIT = 0,
>> +    MAIN_LOOP = 1,
>> +};
>> +
>> +int seccomp_start(enum whitelist_mode mode, scmp_filter_ctx *ctx);
>>   #endif
>> diff --git a/vl.c b/vl.c
>> index bec68cd..773d488 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -278,6 +278,7 @@ static int default_vga = 1;
>>
>>   #ifdef CONFIG_SECCOMP
>>   bool seccomp_on = true;
>> +scmp_filter_ctx ctx;
>
> This should be a local variable to main(), maybe also named
> 'main_loop_ctx' so we can add further contexts.
>
>>   #endif
>>
>>   static struct {
>> @@ -777,7 +778,7 @@ static int bt_parse(const char *opt)
>>   static int install_seccomp_filters(void)
>>   {
>>   #ifdef CONFIG_SECCOMP
>> -    if (seccomp_start() < 0) {
>> +    if (seccomp_start(INIT, &ctx) < 0) {
>>           qerror_report(ERROR_CLASS_GENERIC_ERROR,
>>                   "failed to install seccomp syscall filter in the kernel");
>>           return -1;
>> @@ -3794,6 +3795,16 @@ int main(int argc, char **argv, char **envp)
>>
>>       os_setup_post();
>>
>> +    if (seccomp_on) {
>
> 'seccomp_on' is only available with CONFIG_SECCOMP, so this would break build.
>
>> +#ifdef CONFIG_SECCOMP
>> +        if (seccomp_start(MAIN_LOOP, &ctx) < 0) {
>> +            qerror_report(ERROR_CLASS_GENERIC_ERROR,
>> +                          "failed to install seccomp syscall filter in the kernel");
>
> This error message could be different from the first one.
>
>> +            return -1;
>> +        }
>> +#endif
>> +    }
>> +
>>       resume_all_vcpus();
>>       main_loop();
>>       bdrv_close_all();
>> --
>> 1.7.12
>>
>>
>
>

-- 
Regards,
Corey Bryant

Re: [Qemu-devel] [PATCH 3/4] Support for "double whitelist" filters

[Eric Blake]

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

On 10/19/2012 02:08 PM, Corey Bryant wrote:
> 
> 
> On 10/19/2012 01:04 PM, Blue Swirl wrote:
>> On Wed, Oct 17, 2012 at 1:15 PM, Eduardo Otubo
>> <otubo@linux.vnet.ibm.com> wrote:
>>> This patch includes a second whitelist right before the main loop. It's
>>> a smaller and more restricted whitelist, excluding execve() among many
>>> others.
>>>

>> It's nice to see that for example open, creat, unlink, socket, bind,
>> mprotect, setrlimit and kill are not present.
>>
> 
> Hmm, well open minimally needs to be added to this list so that drives
> can be hotplugged.

Unless we enforce the use of add-fd for hot-plugging drives, but that in
turn requires that we have -blockdev semantics for telling qemu how to
open backing chains.

-- 
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: 617 bytes --]

Re: [Qemu-devel] [PATCH 3/4] Support for "double whitelist" filters

[Corey Bryant]

On 10/19/2012 04:36 PM, Eric Blake wrote:
> On 10/19/2012 02:08 PM, Corey Bryant wrote:
>>
>>
>> On 10/19/2012 01:04 PM, Blue Swirl wrote:
>>> On Wed, Oct 17, 2012 at 1:15 PM, Eduardo Otubo
>>> <otubo@linux.vnet.ibm.com> wrote:
>>>> This patch includes a second whitelist right before the main loop. It's
>>>> a smaller and more restricted whitelist, excluding execve() among many
>>>> others.
>>>>
>
>>> It's nice to see that for example open, creat, unlink, socket, bind,
>>> mprotect, setrlimit and kill are not present.
>>>
>>
>> Hmm, well open minimally needs to be added to this list so that drives
>> can be hotplugged.
>
> Unless we enforce the use of add-fd for hot-plugging drives, but that in
> turn requires that we have -blockdev semantics for telling qemu how to
> open backing chains.
>

True, that would be nice.  But for now we don't have a complete fd 
passing solution so maybe we can add that restriction in the future.

-- 
Regards,
Corey Bryant

Re: [Qemu-devel] [PATCH 3/4] Support for "double whitelist" filters

[Corey Bryant]

On 10/17/2012 09:15 AM, Eduardo Otubo wrote:
> This patch includes a second whitelist right before the main loop. It's
> a smaller and more restricted whitelist, excluding execve() among many
> others.
>
> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
> ---
>   qemu-seccomp.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
>   qemu-seccomp.h |  7 ++++-
>   vl.c           | 13 +++++++-
>   3 files changed, 103 insertions(+), 11 deletions(-)
>
> diff --git a/qemu-seccomp.c b/qemu-seccomp.c
> index a25f2fa..9c68af5 100644
> --- a/qemu-seccomp.c
> +++ b/qemu-seccomp.c
> @@ -13,6 +13,7 @@
>    * GNU GPL, version 2 or (at your option) any later version.
>    */
>   #include <stdio.h>
> +#include <stdlib.h>
>   #include <seccomp.h>
>   #include "qemu-seccomp.h"
>
> @@ -21,7 +22,7 @@ struct QemuSeccompSyscall {
>       uint8_t priority;
>   };
>
> -static const struct QemuSeccompSyscall seccomp_whitelist[] = {
> +static const struct QemuSeccompSyscall seccomp_whitelist_init[] = {
>       { SCMP_SYS(timer_settime), 255 },
>       { SCMP_SYS(timer_gettime), 254 },
>       { SCMP_SYS(futex), 253 },
> @@ -118,27 +119,102 @@ static const struct QemuSeccompSyscall seccomp_whitelist[] = {
>       { SCMP_SYS(accept4), 242 }
>   };
>
> -int seccomp_start(void)
> +static const struct QemuSeccompSyscall seccomp_whitelist_main_loop[] = {
> +    { SCMP_SYS(timer_settime), 255 },
> +    { SCMP_SYS(timer_gettime), 254 },
> +    { SCMP_SYS(futex), 253 },
> +    { SCMP_SYS(select), 252 },
> +    { SCMP_SYS(recvfrom), 251 },
> +    { SCMP_SYS(sendto), 250 },
> +    { SCMP_SYS(read), 249 },
> +    { SCMP_SYS(brk), 248 },
> +    { SCMP_SYS(mmap), 247 },
> +#if defined(__i386__)
> +    { SCMP_SYS(fcntl64), 245 },
> +    { SCMP_SYS(fstat64), 245 },
> +    { SCMP_SYS(stat64), 245 },
> +    { SCMP_SYS(getgid32), 245 },
> +    { SCMP_SYS(getegid32), 245 },
> +    { SCMP_SYS(getuid32), 245 },
> +    { SCMP_SYS(geteuid32), 245 },
> +    { SCMP_SYS(sigreturn), 245 },
> +    { SCMP_SYS(_newselect), 245 },
> +    { SCMP_SYS(_llseek), 245 },
> +    { SCMP_SYS(mmap2), 245},
> +    { SCMP_SYS(sigprocmask), 245 },
> +#endif
> +    { SCMP_SYS(exit), 245 },
> +    { SCMP_SYS(timer_delete), 245 },
> +    { SCMP_SYS(exit_group), 245 },
> +    { SCMP_SYS(rt_sigreturn), 245 },
> +    { SCMP_SYS(madvise), 245 },
> +    { SCMP_SYS(write), 244 },
> +    { SCMP_SYS(fcntl), 243 },
> +    { SCMP_SYS(tgkill), 242 },
> +    { SCMP_SYS(rt_sigaction), 242 },
> +    { SCMP_SYS(pipe2), 242 },
> +    { SCMP_SYS(munmap), 242 },
> +    { SCMP_SYS(mremap), 242 },
> +    { SCMP_SYS(getsockname), 242 },
> +    { SCMP_SYS(getpeername), 242 },
> +    { SCMP_SYS(close), 242 },
> +    { SCMP_SYS(accept4), 242 }
> +};

This list also needs: eventfd2, recvmsg, ioctl, rt_sigprocmask.

> +
> +static int
> +process_whitelist(const struct QemuSeccompSyscall *whitelist,
> +                  unsigned int size, scmp_filter_ctx *ctx)
>   {
>       int rc = 0;
> +
>       unsigned int i = 0;
> -    scmp_filter_ctx ctx;
> +
> +    for (i = 0; i < size; i++) {
> +        rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, whitelist[i].num, 0);
> +        if (rc < 0) {
> +            return -1;
> +        }
> +
> +        rc = seccomp_syscall_priority(ctx, whitelist[i].num,
> +                                      whitelist[i].priority);
> +        if (rc < 0) {
> +            return -1;
> +        }
> +    }
> +    return 0;
> +}
> +
> +int
> +seccomp_start(enum whitelist_mode mode, scmp_filter_ctx *ctx)
> +{
> +    int rc = 0;
>
>       ctx = seccomp_init(SCMP_ACT_KILL);
>       if (ctx == NULL) {
> +        rc = -1;
>           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);
> -        if (rc < 0) {
> +    switch (mode) {
> +    case INIT:
> +        if (process_whitelist
> +            (seccomp_whitelist_init,
> +             ARRAY_SIZE(seccomp_whitelist_init), ctx) < 0) {
> +            rc = -1;
>               goto seccomp_return;
>           }
> -        rc = seccomp_syscall_priority(ctx, seccomp_whitelist[i].num,
> -                                      seccomp_whitelist[i].priority);
> -        if (rc < 0) {
> +        break;
> +    case MAIN_LOOP:
> +        if (process_whitelist
> +            (seccomp_whitelist_main_loop,
> +             ARRAY_SIZE(seccomp_whitelist_main_loop), ctx) < 0) {
> +            rc = -1;
>               goto seccomp_return;
>           }
> +        break;
> +    default:
> +        rc = -1;
> +        goto seccomp_return;
>       }
>
>       rc = seccomp_load(ctx);
> diff --git a/qemu-seccomp.h b/qemu-seccomp.h
> index b2fc3f8..1c97978 100644
> --- a/qemu-seccomp.h
> +++ b/qemu-seccomp.h
> @@ -18,5 +18,10 @@
>   #include <seccomp.h>
>   #include "osdep.h"
>
> -int seccomp_start(void);
> +enum whitelist_mode {
> +    INIT = 0,
> +    MAIN_LOOP = 1,
> +};
> +
> +int seccomp_start(enum whitelist_mode mode, scmp_filter_ctx *ctx);
>   #endif
> diff --git a/vl.c b/vl.c
> index bec68cd..773d488 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -278,6 +278,7 @@ static int default_vga = 1;
>
>   #ifdef CONFIG_SECCOMP
>   bool seccomp_on = true;
> +scmp_filter_ctx ctx;
>   #endif
>
>   static struct {
> @@ -777,7 +778,7 @@ static int bt_parse(const char *opt)
>   static int install_seccomp_filters(void)
>   {
>   #ifdef CONFIG_SECCOMP
> -    if (seccomp_start() < 0) {
> +    if (seccomp_start(INIT, &ctx) < 0) {
>           qerror_report(ERROR_CLASS_GENERIC_ERROR,
>                   "failed to install seccomp syscall filter in the kernel");
>           return -1;
> @@ -3794,6 +3795,16 @@ int main(int argc, char **argv, char **envp)
>
>       os_setup_post();
>
> +    if (seccomp_on) {
> +#ifdef CONFIG_SECCOMP
> +        if (seccomp_start(MAIN_LOOP, &ctx) < 0) {

The first list is installed with install_seccomp_filters() and this one 
is installed with seccomp_start().  One thing you could do make it more 
consistent is to add a parameter for whitelist_mode mode to 
install_seccomp_filters() and call install_seccomp_filters(INIT) and 
install_seccomp_filters(MAIN_LOOP).

> +            qerror_report(ERROR_CLASS_GENERIC_ERROR,
> +                          "failed to install seccomp syscall filter in the kernel");
> +            return -1;
> +        }
> +#endif
> +    }
> +
>       resume_all_vcpus();
>       main_loop();
>       bdrv_close_all();
>

-- 
Regards,
Corey Bryant

[Qemu-devel] [PATCH 4/4] Warning messages on net devices hotplug

[Eduardo Otubo]

With the inclusion of the new "double whitelist" seccomp filter, Qemu
won't be able to execve() in runtime, thus, no hotplug net devices
allowed.

Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
---
 hmp.c |  6 ++++++
 net.c | 13 +++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/hmp.c b/hmp.c
index 70bdec2..f258338 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1091,6 +1091,12 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
     QemuOpts *opts;
 
+#ifdef CONFIG_SECCOMP
+    error_set(&err, ERROR_CLASS_GENERIC_ERROR,
+            "Cannot hotplug TAP device when -sandbox is in effect");
+    goto out;
+#endif
+
     opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &err);
     if (error_is_set(&err)) {
         goto out;
diff --git a/net.c b/net.c
index ae4bc0d..a652ee9 100644
--- a/net.c
+++ b/net.c
@@ -752,6 +752,12 @@ void net_host_device_add(Monitor *mon, const QDict *qdict)
     Error *local_err = NULL;
     QemuOpts *opts;
 
+#ifdef CONFIG_SECCOMP
+    error_set(&local_err, ERROR_CLASS_GENERIC_ERROR,
+            "Cannot hotplug TAP device when -sandbox is in effect");
+    goto out;
+#endif
+
     if (!net_host_check_device(device)) {
         monitor_printf(mon, "invalid host network device %s\n", device);
         return;
@@ -765,6 +771,7 @@ void net_host_device_add(Monitor *mon, const QDict *qdict)
     qemu_opt_set(opts, "type", device);
 
     net_client_init(opts, 0, &local_err);
+out:
     if (error_is_set(&local_err)) {
         qerror_report_err(local_err);
         error_free(local_err);
@@ -800,6 +807,12 @@ int qmp_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret)
     QemuOptsList *opts_list;
     QemuOpts *opts;
 
+#ifdef CONFIG_SECCOMP
+    error_set(&local_err, ERROR_CLASS_GENERIC_ERROR,
+            "Cannot hotplug TAP device when -sandbox is in effect");
+    goto exit_err;
+#endif
+
     opts_list = qemu_find_opts_err("netdev", &local_err);
     if (error_is_set(&local_err)) {
         goto exit_err;
-- 
1.7.12

Re: [Qemu-devel] [PATCH 4/4] Warning messages on net devices hotplug

[Corey Bryant]

On 10/17/2012 09:15 AM, Eduardo Otubo wrote:
> With the inclusion of the new "double whitelist" seccomp filter, Qemu
> won't be able to execve() in runtime, thus, no hotplug net devices
> allowed.
>
> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
> ---
>   hmp.c |  6 ++++++
>   net.c | 13 +++++++++++++
>   2 files changed, 19 insertions(+)
>
> diff --git a/hmp.c b/hmp.c
> index 70bdec2..f258338 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1091,6 +1091,12 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict)
>       Error *err = NULL;
>       QemuOpts *opts;
>
> +#ifdef CONFIG_SECCOMP
> +    error_set(&err, ERROR_CLASS_GENERIC_ERROR,
> +            "Cannot hotplug TAP device when -sandbox is in effect");
> +    goto out;
> +#endif
> +
>       opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &err);
>       if (error_is_set(&err)) {
>           goto out;
> diff --git a/net.c b/net.c
> index ae4bc0d..a652ee9 100644
> --- a/net.c
> +++ b/net.c
> @@ -752,6 +752,12 @@ void net_host_device_add(Monitor *mon, const QDict *qdict)
>       Error *local_err = NULL;
>       QemuOpts *opts;
>
> +#ifdef CONFIG_SECCOMP
> +    error_set(&local_err, ERROR_CLASS_GENERIC_ERROR,
> +            "Cannot hotplug TAP device when -sandbox is in effect");
> +    goto out;
> +#endif
> +
>       if (!net_host_check_device(device)) {
>           monitor_printf(mon, "invalid host network device %s\n", device);
>           return;
> @@ -765,6 +771,7 @@ void net_host_device_add(Monitor *mon, const QDict *qdict)
>       qemu_opt_set(opts, "type", device);
>
>       net_client_init(opts, 0, &local_err);
> +out:
>       if (error_is_set(&local_err)) {
>           qerror_report_err(local_err);
>           error_free(local_err);
> @@ -800,6 +807,12 @@ int qmp_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret)
>       QemuOptsList *opts_list;
>       QemuOpts *opts;
>
> +#ifdef CONFIG_SECCOMP
> +    error_set(&local_err, ERROR_CLASS_GENERIC_ERROR,
> +            "Cannot hotplug TAP device when -sandbox is in effect");
> +    goto exit_err;
> +#endif
> +
>       opts_list = qemu_find_opts_err("netdev", &local_err);
>       if (error_is_set(&local_err)) {
>           goto exit_err;
>

I think you need to either remove "TAP" from these messages, or limit 
this new code to tap and bridge since those are the backends that call 
execve().

Also, this should be documented somewhere so that users can find out 
about this behavior before attempting to hotplug a network device. 
Perhaps this could be documented on the man page for -sandbox and notes 
could be added to the HMP/QMP commands.

-- 
Regards,
Corey Bryant

Re: [Qemu-devel] [PATCH 4/4] Warning messages on net devices hotplug

[Paolo Bonzini]

Il 17/10/2012 15:15, Eduardo Otubo ha scritto:
> With the inclusion of the new "double whitelist" seccomp filter, Qemu
> won't be able to execve() in runtime, thus, no hotplug net devices
> allowed.
> 
> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>

Please check this in net_init_tap instead.  When using libvirt, hotplug
is done with a completely different mechanism that involves
file-descriptor passing and does not require executing a helper.

Paolo

> ---
>  hmp.c |  6 ++++++
>  net.c | 13 +++++++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/hmp.c b/hmp.c
> index 70bdec2..f258338 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1091,6 +1091,12 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict)
>      Error *err = NULL;
>      QemuOpts *opts;
>  
> +#ifdef CONFIG_SECCOMP
> +    error_set(&err, ERROR_CLASS_GENERIC_ERROR,
> +            "Cannot hotplug TAP device when -sandbox is in effect");
> +    goto out;
> +#endif
> +
>      opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &err);
>      if (error_is_set(&err)) {
>          goto out;
> diff --git a/net.c b/net.c
> index ae4bc0d..a652ee9 100644
> --- a/net.c
> +++ b/net.c
> @@ -752,6 +752,12 @@ void net_host_device_add(Monitor *mon, const QDict *qdict)
>      Error *local_err = NULL;
>      QemuOpts *opts;
>  
> +#ifdef CONFIG_SECCOMP
> +    error_set(&local_err, ERROR_CLASS_GENERIC_ERROR,
> +            "Cannot hotplug TAP device when -sandbox is in effect");
> +    goto out;
> +#endif
> +
>      if (!net_host_check_device(device)) {
>          monitor_printf(mon, "invalid host network device %s\n", device);
>          return;
> @@ -765,6 +771,7 @@ void net_host_device_add(Monitor *mon, const QDict *qdict)
>      qemu_opt_set(opts, "type", device);
>  
>      net_client_init(opts, 0, &local_err);
> +out:
>      if (error_is_set(&local_err)) {
>          qerror_report_err(local_err);
>          error_free(local_err);
> @@ -800,6 +807,12 @@ int qmp_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret)
>      QemuOptsList *opts_list;
>      QemuOpts *opts;
>  
> +#ifdef CONFIG_SECCOMP
> +    error_set(&local_err, ERROR_CLASS_GENERIC_ERROR,
> +            "Cannot hotplug TAP device when -sandbox is in effect");
> +    goto exit_err;
> +#endif
> +
>      opts_list = qemu_find_opts_err("netdev", &local_err);
>      if (error_is_set(&local_err)) {
>          goto exit_err;
> 

Re: [Qemu-devel] [PATCH 4/4] Warning messages on net devices hotplug

[Corey Bryant]

On 10/18/2012 11:15 AM, Paolo Bonzini wrote:
> Il 17/10/2012 15:15, Eduardo Otubo ha scritto:
>> With the inclusion of the new "double whitelist" seccomp filter, Qemu
>> won't be able to execve() in runtime, thus, no hotplug net devices
>> allowed.
>>
>> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
>
> Please check this in net_init_tap instead.  When using libvirt, hotplug
> is done with a completely different mechanism that involves
> file-descriptor passing and does not require executing a helper.
>
> Paolo
>

Are you sure net_init_tap() is the right place for this check?  We only 
want to prevent execve() after main_loop() is entered.  In other words 
we want to allow execve() caused by a command line option (e.g. -net 
tap) but we want to prevent execve() when it is the result of a monitor 
command (e.g. netdev_add tap).

>> ---
>>   hmp.c |  6 ++++++
>>   net.c | 13 +++++++++++++
>>   2 files changed, 19 insertions(+)
>>
>> diff --git a/hmp.c b/hmp.c
>> index 70bdec2..f258338 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -1091,6 +1091,12 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict)
>>       Error *err = NULL;
>>       QemuOpts *opts;
>>
>> +#ifdef CONFIG_SECCOMP
>> +    error_set(&err, ERROR_CLASS_GENERIC_ERROR,
>> +            "Cannot hotplug TAP device when -sandbox is in effect");
>> +    goto out;
>> +#endif
>> +
>>       opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &err);
>>       if (error_is_set(&err)) {
>>           goto out;
>> diff --git a/net.c b/net.c
>> index ae4bc0d..a652ee9 100644
>> --- a/net.c
>> +++ b/net.c
>> @@ -752,6 +752,12 @@ void net_host_device_add(Monitor *mon, const QDict *qdict)
>>       Error *local_err = NULL;
>>       QemuOpts *opts;
>>
>> +#ifdef CONFIG_SECCOMP
>> +    error_set(&local_err, ERROR_CLASS_GENERIC_ERROR,
>> +            "Cannot hotplug TAP device when -sandbox is in effect");
>> +    goto out;
>> +#endif
>> +
>>       if (!net_host_check_device(device)) {
>>           monitor_printf(mon, "invalid host network device %s\n", device);
>>           return;
>> @@ -765,6 +771,7 @@ void net_host_device_add(Monitor *mon, const QDict *qdict)
>>       qemu_opt_set(opts, "type", device);
>>
>>       net_client_init(opts, 0, &local_err);
>> +out:
>>       if (error_is_set(&local_err)) {
>>           qerror_report_err(local_err);
>>           error_free(local_err);
>> @@ -800,6 +807,12 @@ int qmp_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret)
>>       QemuOptsList *opts_list;
>>       QemuOpts *opts;
>>
>> +#ifdef CONFIG_SECCOMP
>> +    error_set(&local_err, ERROR_CLASS_GENERIC_ERROR,
>> +            "Cannot hotplug TAP device when -sandbox is in effect");
>> +    goto exit_err;
>> +#endif
>> +
>>       opts_list = qemu_find_opts_err("netdev", &local_err);
>>       if (error_is_set(&local_err)) {
>>           goto exit_err;
>>
>
>
>
>

-- 
Regards,
Corey Bryant

Re: [Qemu-devel] [PATCH 4/4] Warning messages on net devices hotplug

[Corey Bryant]

On 10/24/2012 10:18 AM, Corey Bryant wrote:
>
>
> On 10/18/2012 11:15 AM, Paolo Bonzini wrote:
>> Il 17/10/2012 15:15, Eduardo Otubo ha scritto:
>>> With the inclusion of the new "double whitelist" seccomp filter, Qemu
>>> won't be able to execve() in runtime, thus, no hotplug net devices
>>> allowed.
>>>
>>> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
>>
>> Please check this in net_init_tap instead.  When using libvirt, hotplug
>> is done with a completely different mechanism that involves
>> file-descriptor passing and does not require executing a helper.
>>
>> Paolo
>>
>
> Are you sure net_init_tap() is the right place for this check?  We only
> want to prevent execve() after main_loop() is entered.  In other words
> we want to allow execve() caused by a command line option (e.g. -net
> tap) but we want to prevent execve() when it is the result of a monitor
> command (e.g. netdev_add tap).
>

Or perhaps we could put the check in net_init_tap() and only prevent the 
command when runstate != RUN_STATE_PRELAUNCH?

Note that we plan to only prevent the hotplug of net devices in the 
cases when execve() would be called.  So libvirt will still be able to 
pass an fd.

>>> ---
>>>   hmp.c |  6 ++++++
>>>   net.c | 13 +++++++++++++
>>>   2 files changed, 19 insertions(+)
>>>
>>> diff --git a/hmp.c b/hmp.c
>>> index 70bdec2..f258338 100644
>>> --- a/hmp.c
>>> +++ b/hmp.c
>>> @@ -1091,6 +1091,12 @@ void hmp_netdev_add(Monitor *mon, const QDict
>>> *qdict)
>>>       Error *err = NULL;
>>>       QemuOpts *opts;
>>>
>>> +#ifdef CONFIG_SECCOMP
>>> +    error_set(&err, ERROR_CLASS_GENERIC_ERROR,
>>> +            "Cannot hotplug TAP device when -sandbox is in effect");
>>> +    goto out;
>>> +#endif
>>> +
>>>       opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict,
>>> &err);
>>>       if (error_is_set(&err)) {
>>>           goto out;
>>> diff --git a/net.c b/net.c
>>> index ae4bc0d..a652ee9 100644
>>> --- a/net.c
>>> +++ b/net.c
>>> @@ -752,6 +752,12 @@ void net_host_device_add(Monitor *mon, const
>>> QDict *qdict)
>>>       Error *local_err = NULL;
>>>       QemuOpts *opts;
>>>
>>> +#ifdef CONFIG_SECCOMP
>>> +    error_set(&local_err, ERROR_CLASS_GENERIC_ERROR,
>>> +            "Cannot hotplug TAP device when -sandbox is in effect");
>>> +    goto out;
>>> +#endif
>>> +
>>>       if (!net_host_check_device(device)) {
>>>           monitor_printf(mon, "invalid host network device %s\n",
>>> device);
>>>           return;
>>> @@ -765,6 +771,7 @@ void net_host_device_add(Monitor *mon, const
>>> QDict *qdict)
>>>       qemu_opt_set(opts, "type", device);
>>>
>>>       net_client_init(opts, 0, &local_err);
>>> +out:
>>>       if (error_is_set(&local_err)) {
>>>           qerror_report_err(local_err);
>>>           error_free(local_err);
>>> @@ -800,6 +807,12 @@ int qmp_netdev_add(Monitor *mon, const QDict
>>> *qdict, QObject **ret)
>>>       QemuOptsList *opts_list;
>>>       QemuOpts *opts;
>>>
>>> +#ifdef CONFIG_SECCOMP
>>> +    error_set(&local_err, ERROR_CLASS_GENERIC_ERROR,
>>> +            "Cannot hotplug TAP device when -sandbox is in effect");
>>> +    goto exit_err;
>>> +#endif
>>> +
>>>       opts_list = qemu_find_opts_err("netdev", &local_err);
>>>       if (error_is_set(&local_err)) {
>>>           goto exit_err;
>>>
>>
>>
>>
>>
>

-- 
Regards,
Corey Bryant

Re: [Qemu-devel] [PATCH 4/4] Warning messages on net devices hotplug

[Paolo Bonzini]

Il 24/10/2012 16:18, Corey Bryant ha scritto:
> 
> 
> On 10/18/2012 11:15 AM, Paolo Bonzini wrote:
>> Il 17/10/2012 15:15, Eduardo Otubo ha scritto:
>>> With the inclusion of the new "double whitelist" seccomp filter, Qemu
>>> won't be able to execve() in runtime, thus, no hotplug net devices
>>> allowed.
>>>
>>> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
>>
>> Please check this in net_init_tap instead.  When using libvirt, hotplug
>> is done with a completely different mechanism that involves
>> file-descriptor passing and does not require executing a helper.
>>
>> Paolo
>>
> 
> Are you sure net_init_tap() is the right place for this check?

Yes, assuming there is a global that says whether the seccomp sandbox is
in effect.  Even something like "if (sandbox_active && !tap->has_fd)
error(...)" can be enough.

Paolo

Re: [Qemu-devel] [PATCH 4/4] Warning messages on net devices hotplug

[Corey Bryant]

On 10/24/2012 11:21 AM, Paolo Bonzini wrote:
> Il 24/10/2012 16:18, Corey Bryant ha scritto:
>>
>>
>> On 10/18/2012 11:15 AM, Paolo Bonzini wrote:
>>> Il 17/10/2012 15:15, Eduardo Otubo ha scritto:
>>>> With the inclusion of the new "double whitelist" seccomp filter, Qemu
>>>> won't be able to execve() in runtime, thus, no hotplug net devices
>>>> allowed.
>>>>
>>>> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
>>>
>>> Please check this in net_init_tap instead.  When using libvirt, hotplug
>>> is done with a completely different mechanism that involves
>>> file-descriptor passing and does not require executing a helper.
>>>
>>> Paolo
>>>
>>
>> Are you sure net_init_tap() is the right place for this check?
> 
> Yes, assuming there is a global that says whether the seccomp sandbox is
> in effect.  Even something like "if (sandbox_active && !tap->has_fd)
> error(...)" can be enough.
> 
> Paolo
> 

What do you think about this?  It moves the checks into the functions that actually cause execve() to be called, and it only prevents the commands after QEMU is done with initialization in main().

---

diff --git a/net/tap.c b/net/tap.c
index df89caa..7a8a234 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -352,6 +352,14 @@ static int launch_script(const char *setup_script, const char *ifname, int fd)
     char *args[3];
     char **parg;
 
+#ifdef CONFIG_SECCOMP
+        if (!runstate_is_prelaunch()) {
+            error_report("Cannot execute network script from QEMU monitor "
+                         "when -sandbox is in effect");
+            return -1;
+        }
+#endif
+
     /* try to launch network script */
     pid = fork();
     if (pid == 0) {
@@ -426,6 +434,14 @@ static int net_bridge_run_helper(const char *helper, const char *bridge)
     char **parg;
     int sv[2];
 
+#ifdef CONFIG_SECCOMP
+        if (!runstate_is_prelaunch()) {
+            error_report("Cannot execute network helper from QEMU monitor "
+                         "when -sandbox is in effect");
+            return -1;
+        }
+#endif
+
     sigemptyset(&mask);
     sigaddset(&mask, SIGCHLD);
     sigprocmask(SIG_BLOCK, &mask, &oldmask);
diff --git a/sysemu.h b/sysemu.h
index 0c39a3a..37d8c7d 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -23,6 +23,7 @@ void runstate_init(void);
 bool runstate_check(RunState state);
 void runstate_set(RunState new_state);
 int runstate_is_running(void);
+int runstate_is_prelaunch(void);
 typedef struct vm_change_state_entry VMChangeStateEntry;
 typedef void VMChangeStateHandler(void *opaque, int running, RunState state);
 
diff --git a/vl.c b/vl.c
index c7e88ff..b19b9fa 100644
--- a/vl.c
+++ b/vl.c
@@ -432,6 +432,11 @@ int runstate_is_running(void)
     return runstate_check(RUN_STATE_RUNNING);
 }
 
+int runstate_is_prelaunch(void)
+{
+    return runstate_check(RUN_STATE_PRELAUNCH);
+}
+
 StatusInfo *qmp_query_status(Error **errp)
 {
     StatusInfo *info = g_malloc0(sizeof(*info));
-- 
1.7.11.7


-- 
Regards,
Corey Bryant

Re: [Qemu-devel] [PATCH 4/4] Warning messages on net devices hotplug

[Paolo Bonzini]

Il 24/10/2012 17:39, Corey Bryant ha scritto:
> 
> 
> On 10/24/2012 11:21 AM, Paolo Bonzini wrote:
>> Il 24/10/2012 16:18, Corey Bryant ha scritto:
>>>
>>>
>>> On 10/18/2012 11:15 AM, Paolo Bonzini wrote:
>>>> Il 17/10/2012 15:15, Eduardo Otubo ha scritto:
>>>>> With the inclusion of the new "double whitelist" seccomp filter, Qemu
>>>>> won't be able to execve() in runtime, thus, no hotplug net devices
>>>>> allowed.
>>>>>
>>>>> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
>>>>
>>>> Please check this in net_init_tap instead.  When using libvirt, hotplug
>>>> is done with a completely different mechanism that involves
>>>> file-descriptor passing and does not require executing a helper.
>>>>
>>>> Paolo
>>>>
>>>
>>> Are you sure net_init_tap() is the right place for this check?
>>
>> Yes, assuming there is a global that says whether the seccomp sandbox is
>> in effect.  Even something like "if (sandbox_active && !tap->has_fd)
>> error(...)" can be enough.
>>
>> Paolo
>>
> 
> What do you think about this? It moves the checks into the functions
> that actually cause execve() to be called, and it only prevents the
> commands after QEMU is done with initialization in main().

It doesn't do error reporting correctly because these functions do not
get an Error **.  If you change that and use error_setg instead of
error_report, it should be okay.

However, I really think what your testing is not
runstate_is_prelaunch(), it is seccomp_effective().  If you structure
the test like that, it also lets you eliminate the #ifdef (which in
general we prefer to avoid).

Paolo

Re: [Qemu-devel] [PATCH 4/4] Warning messages on net devices hotplug

[Corey Bryant]

On 10/24/2012 11:45 AM, Paolo Bonzini wrote:
> Il 24/10/2012 17:39, Corey Bryant ha scritto:
>>
>>
>> On 10/24/2012 11:21 AM, Paolo Bonzini wrote:
>>> Il 24/10/2012 16:18, Corey Bryant ha scritto:
>>>>
>>>>
>>>> On 10/18/2012 11:15 AM, Paolo Bonzini wrote:
>>>>> Il 17/10/2012 15:15, Eduardo Otubo ha scritto:
>>>>>> With the inclusion of the new "double whitelist" seccomp filter, Qemu
>>>>>> won't be able to execve() in runtime, thus, no hotplug net devices
>>>>>> allowed.
>>>>>>
>>>>>> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
>>>>>
>>>>> Please check this in net_init_tap instead.  When using libvirt, hotplug
>>>>> is done with a completely different mechanism that involves
>>>>> file-descriptor passing and does not require executing a helper.
>>>>>
>>>>> Paolo
>>>>>
>>>>
>>>> Are you sure net_init_tap() is the right place for this check?
>>>
>>> Yes, assuming there is a global that says whether the seccomp sandbox is
>>> in effect.  Even something like "if (sandbox_active && !tap->has_fd)
>>> error(...)" can be enough.
>>>
>>> Paolo
>>>
>>
>> What do you think about this? It moves the checks into the functions
>> that actually cause execve() to be called, and it only prevents the
>> commands after QEMU is done with initialization in main().
>
> It doesn't do error reporting correctly because these functions do not
> get an Error **.  If you change that and use error_setg instead of
> error_report, it should be okay.
>
> However, I really think what your testing is not
> runstate_is_prelaunch(), it is seccomp_effective().  If you structure
> the test like that, it also lets you eliminate the #ifdef (which in
> general we prefer to avoid).
>
> Paolo
>

Ok, thanks for the quick feedback!

-- 
Regards,
Corey Bryant

Re: [Qemu-devel] [PATCH 4/4] Warning messages on net devices hotplug

[Corey Bryant]

On 10/24/2012 11:45 AM, Paolo Bonzini wrote:
> Il 24/10/2012 17:39, Corey Bryant ha scritto:
>>
>>
>> On 10/24/2012 11:21 AM, Paolo Bonzini wrote:
>>> Il 24/10/2012 16:18, Corey Bryant ha scritto:
>>>>
>>>>
>>>> On 10/18/2012 11:15 AM, Paolo Bonzini wrote:
>>>>> Il 17/10/2012 15:15, Eduardo Otubo ha scritto:
>>>>>> With the inclusion of the new "double whitelist" seccomp filter, Qemu
>>>>>> won't be able to execve() in runtime, thus, no hotplug net devices
>>>>>> allowed.
>>>>>>
>>>>>> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
>>>>>
>>>>> Please check this in net_init_tap instead.  When using libvirt, hotplug
>>>>> is done with a completely different mechanism that involves
>>>>> file-descriptor passing and does not require executing a helper.
>>>>>
>>>>> Paolo
>>>>>
>>>>
>>>> Are you sure net_init_tap() is the right place for this check?
>>>
>>> Yes, assuming there is a global that says whether the seccomp sandbox is
>>> in effect.  Even something like "if (sandbox_active && !tap->has_fd)
>>> error(...)" can be enough.
>>>
>>> Paolo
>>>
>>
>> What do you think about this? It moves the checks into the functions
>> that actually cause execve() to be called, and it only prevents the
>> commands after QEMU is done with initialization in main().
>
> It doesn't do error reporting correctly because these functions do not
> get an Error **.  If you change that and use error_setg instead of
> error_report, it should be okay.

I just wanted to follow up on a few things..

All of the following functions currently use qerror_report().  I'm 
thinking conversion of these and sub-functions to pass an Error ** 
parameter should be a separate undertaking.

net_init_nic
net_init_slirp
net_init_tap
net_init_socket
net_init_vde
net_init_dump
net_init_bridge
net_init_hubport

>
> However, I really think what your testing is not
> runstate_is_prelaunch(), it is seccomp_effective().  If you structure
> the test like that, it also lets you eliminate the #ifdef (which in
> general we prefer to avoid).

The reason for testing runstate_is_prelaunch() is because seccomp will 
be effective during and after prelaunch.  The only difference is that a 
more restrictive syscall whitelist will be in effect after prelaunch. So 
perhaps the tests can be similar to the following so that we can get rid 
of the preprocessor #ifdef:

if (seccomp_is_effective() && !runstate_is_prelaunch()) {
     error_report("Cannot execute network helper from QEMU monitor "
                  "when -sandbox is in effect");
     return -1;
}

-- 
Regards,
Corey Bryant

Re: [Qemu-devel] [PATCH 4/4] Warning messages on net devices hotplug

[Paolo Bonzini]

Il 24/10/2012 19:30, Corey Bryant ha scritto:
> 
> 
> On 10/24/2012 11:45 AM, Paolo Bonzini wrote:
>> Il 24/10/2012 17:39, Corey Bryant ha scritto:
>>>
>>>
>>> On 10/24/2012 11:21 AM, Paolo Bonzini wrote:
>>>> Il 24/10/2012 16:18, Corey Bryant ha scritto:
>>>>>
>>>>>
>>>>> On 10/18/2012 11:15 AM, Paolo Bonzini wrote:
>>>>>> Il 17/10/2012 15:15, Eduardo Otubo ha scritto:
>>>>>>> With the inclusion of the new "double whitelist" seccomp filter,
>>>>>>> Qemu
>>>>>>> won't be able to execve() in runtime, thus, no hotplug net devices
>>>>>>> allowed.
>>>>>>>
>>>>>>> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
>>>>>>
>>>>>> Please check this in net_init_tap instead.  When using libvirt,
>>>>>> hotplug
>>>>>> is done with a completely different mechanism that involves
>>>>>> file-descriptor passing and does not require executing a helper.
>>>>>>
>>>>>> Paolo
>>>>>>
>>>>>
>>>>> Are you sure net_init_tap() is the right place for this check?
>>>>
>>>> Yes, assuming there is a global that says whether the seccomp
>>>> sandbox is
>>>> in effect.  Even something like "if (sandbox_active && !tap->has_fd)
>>>> error(...)" can be enough.
>>>>
>>>> Paolo
>>>>
>>>
>>> What do you think about this? It moves the checks into the functions
>>> that actually cause execve() to be called, and it only prevents the
>>> commands after QEMU is done with initialization in main().
>>
>> It doesn't do error reporting correctly because these functions do not
>> get an Error **.  If you change that and use error_setg instead of
>> error_report, it should be okay.
> 
> I just wanted to follow up on a few things..
> 
> All of the following functions currently use qerror_report().  I'm
> thinking conversion of these and sub-functions to pass an Error **
> parameter should be a separate undertaking.
> 
> net_init_nic
> net_init_slirp
> net_init_tap
> net_init_socket
> net_init_vde
> net_init_dump
> net_init_bridge
> net_init_hubport

Ok, but it should not be hard considering that the immediate caller of
all these functions (net_client_init1) takes an Error **.  Please
consider this for 1.4 at least.

>> However, I really think what your testing is not
>> runstate_is_prelaunch(), it is seccomp_effective().  If you structure
>> the test like that, it also lets you eliminate the #ifdef (which in
>> general we prefer to avoid).
> 
> The reason for testing runstate_is_prelaunch() is because seccomp will
> be effective during and after prelaunch.  The only difference is that a
> more restrictive syscall whitelist will be in effect after prelaunch. So
> perhaps the tests can be similar to the following so that we can get rid
> of the preprocessor #ifdef:
> 
> if (seccomp_is_effective() && !runstate_is_prelaunch()) {
>     error_report("Cannot execute network helper from QEMU monitor "
>                  "when -sandbox is in effect");
>     return -1;
> }

Then you can make the seccomp query return many levels or flags, like
SECCOMP_SANDBOX_ENABLED | SECCOMP_CAN_EXECVE.

Paolo

Re: [Qemu-devel] [PATCH 4/4] Warning messages on net devices hotplug

[Corey Bryant]

On 10/25/2012 03:40 AM, Paolo Bonzini wrote:
> Il 24/10/2012 19:30, Corey Bryant ha scritto:
>>
>>
>> On 10/24/2012 11:45 AM, Paolo Bonzini wrote:
>>> Il 24/10/2012 17:39, Corey Bryant ha scritto:
>>>>
>>>>
>>>> On 10/24/2012 11:21 AM, Paolo Bonzini wrote:
>>>>> Il 24/10/2012 16:18, Corey Bryant ha scritto:
>>>>>>
>>>>>>
>>>>>> On 10/18/2012 11:15 AM, Paolo Bonzini wrote:
>>>>>>> Il 17/10/2012 15:15, Eduardo Otubo ha scritto:
>>>>>>>> With the inclusion of the new "double whitelist" seccomp filter,
>>>>>>>> Qemu
>>>>>>>> won't be able to execve() in runtime, thus, no hotplug net devices
>>>>>>>> allowed.
>>>>>>>>
>>>>>>>> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
>>>>>>>
>>>>>>> Please check this in net_init_tap instead.  When using libvirt,
>>>>>>> hotplug
>>>>>>> is done with a completely different mechanism that involves
>>>>>>> file-descriptor passing and does not require executing a helper.
>>>>>>>
>>>>>>> Paolo
>>>>>>>
>>>>>>
>>>>>> Are you sure net_init_tap() is the right place for this check?
>>>>>
>>>>> Yes, assuming there is a global that says whether the seccomp
>>>>> sandbox is
>>>>> in effect.  Even something like "if (sandbox_active && !tap->has_fd)
>>>>> error(...)" can be enough.
>>>>>
>>>>> Paolo
>>>>>
>>>>
>>>> What do you think about this? It moves the checks into the functions
>>>> that actually cause execve() to be called, and it only prevents the
>>>> commands after QEMU is done with initialization in main().
>>>
>>> It doesn't do error reporting correctly because these functions do not
>>> get an Error **.  If you change that and use error_setg instead of
>>> error_report, it should be okay.
>>
>> I just wanted to follow up on a few things..
>>
>> All of the following functions currently use qerror_report().  I'm
>> thinking conversion of these and sub-functions to pass an Error **
>> parameter should be a separate undertaking.
>>
>> net_init_nic
>> net_init_slirp
>> net_init_tap
>> net_init_socket
>> net_init_vde
>> net_init_dump
>> net_init_bridge
>> net_init_hubport
>
> Ok, but it should not be hard considering that the immediate caller of
> all these functions (net_client_init1) takes an Error **.  Please
> consider this for 1.4 at least.
>
>>> However, I really think what your testing is not
>>> runstate_is_prelaunch(), it is seccomp_effective().  If you structure
>>> the test like that, it also lets you eliminate the #ifdef (which in
>>> general we prefer to avoid).
>>
>> The reason for testing runstate_is_prelaunch() is because seccomp will
>> be effective during and after prelaunch.  The only difference is that a
>> more restrictive syscall whitelist will be in effect after prelaunch. So
>> perhaps the tests can be similar to the following so that we can get rid
>> of the preprocessor #ifdef:
>>
>> if (seccomp_is_effective() && !runstate_is_prelaunch()) {
>>      error_report("Cannot execute network helper from QEMU monitor "
>>                   "when -sandbox is in effect");
>>      return -1;
>> }
>
> Then you can make the seccomp query return many levels or flags, like
> SECCOMP_SANDBOX_ENABLED | SECCOMP_CAN_EXECVE.
>
> Paolo
>

True, we can do that.  I'll take that approach.  Thanks for the suggestion.

-- 
Regards,
Corey Bryant

Re: [Qemu-devel] [PATCH 1/4] Adding new syscalls (bugzilla 855162)

[Corey Bryant]

On 10/17/2012 09:15 AM, Eduardo Otubo wrote:
> According to the bug 855162[0] - there's the need of adding new syscalls
> to the whitelist whenn using Qemu with Libvirt.
>
> [1] - https://bugzilla.redhat.com/show_bug.cgi?id=855162
>
> Reported-by: Paul Moore <pmoore@redhat.com>
> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
> ---
>   qemu-seccomp.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/qemu-seccomp.c b/qemu-seccomp.c
> index 64329a3..a25f2fa 100644
> --- a/qemu-seccomp.c
> +++ b/qemu-seccomp.c
> @@ -45,6 +45,13 @@ static const struct QemuSeccompSyscall seccomp_whitelist[] = {
>       { SCMP_SYS(access), 245 },
>       { SCMP_SYS(prctl), 245 },
>       { SCMP_SYS(signalfd), 245 },
> +    { SCMP_SYS(getrlimit), 245 },
> +    { SCMP_SYS(set_tid_address), 245 },
> +    { SCMP_SYS(socketpair), 245 },
> +    { SCMP_SYS(statfs), 245 },
> +    { SCMP_SYS(unlink), 245 },
> +    { SCMP_SYS(wait4), 245 },
> +    { SCMP_SYS(getuid), 245 },
>   #if defined(__i386__)
>       { SCMP_SYS(fcntl64), 245 },
>       { SCMP_SYS(fstat64), 245 },
> @@ -107,7 +114,8 @@ static const struct QemuSeccompSyscall seccomp_whitelist[] = {
>       { SCMP_SYS(getsockname), 242 },
>       { SCMP_SYS(getpeername), 242 },
>       { SCMP_SYS(fdatasync), 242 },
> -    { SCMP_SYS(close), 242 }
> +    { SCMP_SYS(close), 242 },
> +    { SCMP_SYS(accept4), 242 }
>   };

This list also needs: readlink, rt_sigpending, and rt_sigtimedwait.

-- 
Regards,
Corey Bryant