[PATCHv4,2/6] seccomp: add obsolete argument to command line
diff mbox

Message ID 20170901105818.31956-3-otubo@redhat.com
State New
Headers show

Commit Message

Eduardo Otubo Sept. 1, 2017, 10:58 a.m. UTC
This patch introduces the argument [,obsolete=allow] to the `-sandbox on'
option. It allows Qemu to run safely on old system that still relies on
old system calls.

Signed-off-by: Eduardo Otubo <otubo@redhat.com>
---
 include/sysemu/seccomp.h |  3 ++-
 qemu-options.hx          | 12 ++++++++++--
 qemu-seccomp.c           | 23 ++++++++++++++++++++++-
 vl.c                     | 22 +++++++++++++++++++++-
 4 files changed, 55 insertions(+), 5 deletions(-)

Comments

Daniel P. Berrangé Sept. 1, 2017, 11:05 a.m. UTC | #1
On Fri, Sep 01, 2017 at 12:58:14PM +0200, Eduardo Otubo wrote:
> This patch introduces the argument [,obsolete=allow] to the `-sandbox on'
> option. It allows Qemu to run safely on old system that still relies on
> old system calls.
> 
> Signed-off-by: Eduardo Otubo <otubo@redhat.com>
> ---
>  include/sysemu/seccomp.h |  3 ++-
>  qemu-options.hx          | 12 ++++++++++--
>  qemu-seccomp.c           | 23 ++++++++++++++++++++++-
>  vl.c                     | 22 +++++++++++++++++++++-
>  4 files changed, 55 insertions(+), 5 deletions(-)
> 

> @@ -72,6 +85,14 @@ int seccomp_start(void)
>  
>      for (i = 0; i < ARRAY_SIZE(blacklist); i++) {
>          switch (blacklist[i].set) {
> +        case QEMU_SECCOMP_SET_OBSOLETE:
> +            if (!(seccomp_opts & QEMU_SECCOMP_SET_OBSOLETE)) {
> +                goto add_syscall;
> +            } else {
> +                continue;
> +            }
> +
> +            break;

THis can be simplified:

            if ((seccomp_opts & QEMU_SECCOMP_SET_OBSOLETE)) {
                continue;
            }

            break;

thus avoiding need to 'goto'

Likewise for all following patches

>          default:
>              goto add_syscall;
>          }

Regards,
Daniel
Eduardo Otubo Sept. 7, 2017, 9:31 a.m. UTC | #2
On Fri, Sep 01, 2017 at 12:05:41PM +0100, Daniel P. Berrange wrote:
> On Fri, Sep 01, 2017 at 12:58:14PM +0200, Eduardo Otubo wrote:
> > This patch introduces the argument [,obsolete=allow] to the `-sandbox on'
> > option. It allows Qemu to run safely on old system that still relies on
> > old system calls.
> > 
> > Signed-off-by: Eduardo Otubo <otubo@redhat.com>
> > ---
> >  include/sysemu/seccomp.h |  3 ++-
> >  qemu-options.hx          | 12 ++++++++++--
> >  qemu-seccomp.c           | 23 ++++++++++++++++++++++-
> >  vl.c                     | 22 +++++++++++++++++++++-
> >  4 files changed, 55 insertions(+), 5 deletions(-)
> > 
> 
> > @@ -72,6 +85,14 @@ int seccomp_start(void)
> >  
> >      for (i = 0; i < ARRAY_SIZE(blacklist); i++) {
> >          switch (blacklist[i].set) {
> > +        case QEMU_SECCOMP_SET_OBSOLETE:
> > +            if (!(seccomp_opts & QEMU_SECCOMP_SET_OBSOLETE)) {
> > +                goto add_syscall;
> > +            } else {
> > +                continue;
> > +            }
> > +
> > +            break;
> 
> THis can be simplified:
> 
>             if ((seccomp_opts & QEMU_SECCOMP_SET_OBSOLETE)) {
>                 continue;
>             }
> 
>             break;
> 
> thus avoiding need to 'goto'
> 
> Likewise for all following patches

Do you think there's anything else to fix on this series? if nothing
else emerges, I'll send the v5 tomorrow (also with the style fixes).
Daniel P. Berrangé Sept. 7, 2017, 9:57 a.m. UTC | #3
On Fri, Sep 01, 2017 at 12:58:14PM +0200, Eduardo Otubo wrote:
> This patch introduces the argument [,obsolete=allow] to the `-sandbox on'
> option. It allows Qemu to run safely on old system that still relies on
> old system calls.
> 
> Signed-off-by: Eduardo Otubo <otubo@redhat.com>
> ---
>  include/sysemu/seccomp.h |  3 ++-
>  qemu-options.hx          | 12 ++++++++++--
>  qemu-seccomp.c           | 23 ++++++++++++++++++++++-
>  vl.c                     | 22 +++++++++++++++++++++-
>  4 files changed, 55 insertions(+), 5 deletions(-)
> 

> @@ -1032,7 +1036,23 @@ static int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
>  {
>      if (qemu_opt_get_bool(opts, "enable", false)) {
>  #ifdef CONFIG_SECCOMP
> -        if (seccomp_start() < 0) {
> +        uint32_t seccomp_opts = 0x00000;
> +        const char *value = NULL;
> +
> +        value = qemu_opt_get(opts, "obsolete");
> +        if (value) {
> +            if (strcmp(value, "allow") == 0) {

I would have a slight preference for g_str_equal(value, "allow")

> +                seccomp_opts |= QEMU_SECCOMP_SET_OBSOLETE;
> +            } else if (strcmp(value, "deny")) {

and  !g_str_equal(value, "deny")

> +                /* this is the default option, this if is here
> +		 * to provide a little bit of consistency for
> +		 * the command line */
> +	    } else {
> +		error_report("invalid argument for obsolete");
> +	    }

There seem to be tabs for indent here too

> +        }
> +
> +        if (seccomp_start(seccomp_opts) < 0) {
>              error_report("failed to install seccomp syscall filter "
>                           "in the kernel");
>              return -1;
> -- 
> 2.13.5
> 

Regards,
Daniel
Daniel P. Berrangé Sept. 7, 2017, 9:59 a.m. UTC | #4
On Thu, Sep 07, 2017 at 11:31:04AM +0200, Eduardo Otubo wrote:
> On Fri, Sep 01, 2017 at 12:05:41PM +0100, Daniel P. Berrange wrote:
> > On Fri, Sep 01, 2017 at 12:58:14PM +0200, Eduardo Otubo wrote:
> > > This patch introduces the argument [,obsolete=allow] to the `-sandbox on'
> > > option. It allows Qemu to run safely on old system that still relies on
> > > old system calls.
> > > 
> > > Signed-off-by: Eduardo Otubo <otubo@redhat.com>
> > > ---
> > >  include/sysemu/seccomp.h |  3 ++-
> > >  qemu-options.hx          | 12 ++++++++++--
> > >  qemu-seccomp.c           | 23 ++++++++++++++++++++++-
> > >  vl.c                     | 22 +++++++++++++++++++++-
> > >  4 files changed, 55 insertions(+), 5 deletions(-)
> > > 
> > 
> > > @@ -72,6 +85,14 @@ int seccomp_start(void)
> > >  
> > >      for (i = 0; i < ARRAY_SIZE(blacklist); i++) {
> > >          switch (blacklist[i].set) {
> > > +        case QEMU_SECCOMP_SET_OBSOLETE:
> > > +            if (!(seccomp_opts & QEMU_SECCOMP_SET_OBSOLETE)) {
> > > +                goto add_syscall;
> > > +            } else {
> > > +                continue;
> > > +            }
> > > +
> > > +            break;
> > 
> > THis can be simplified:
> > 
> >             if ((seccomp_opts & QEMU_SECCOMP_SET_OBSOLETE)) {
> >                 continue;
> >             }
> > 
> >             break;
> > 
> > thus avoiding need to 'goto'
> > 
> > Likewise for all following patches
> 
> Do you think there's anything else to fix on this series? if nothing
> else emerges, I'll send the v5 tomorrow (also with the style fixes).

I just sent one more comment, but apart from the that & the style fixes
it looks good to me.

Regards,
Daniel

Patch
diff mbox

diff --git a/include/sysemu/seccomp.h b/include/sysemu/seccomp.h
index 23b9c3c789..215138a372 100644
--- a/include/sysemu/seccomp.h
+++ b/include/sysemu/seccomp.h
@@ -16,8 +16,9 @@ 
 #define QEMU_SECCOMP_H
 
 #define QEMU_SECCOMP_SET_DEFAULT     (1 << 0)
+#define QEMU_SECCOMP_SET_OBSOLETE    (1 << 1)
 
 #include <seccomp.h>
 
-int seccomp_start(void);
+int seccomp_start(uint32_t seccomp_opts);
 #endif
diff --git a/qemu-options.hx b/qemu-options.hx
index 9f6e2adfff..72150c6b84 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4017,13 +4017,21 @@  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 on[,obsolete=allow|deny]\n" \
+    "                Enable seccomp mode 2 system call filter (default 'off').\n" \
+    "                use 'obsolete' to allow obsolete system calls that are provided\n" \
+    "                    by the kernel, but typically no longer used by modern\n" \
+    "                    C library implementations.\n",
     QEMU_ARCH_ALL)
 STEXI
-@item -sandbox @var{arg}
+@item -sandbox @var{arg}[,obsolete=@var{string}]
 @findex -sandbox
 Enable Seccomp mode 2 system call filter. 'on' will enable syscall filtering and 'off' will
 disable it.  The default is 'off'.
+@table @option
+@item obsolete=@var{string}
+Enable Obsolete system calls
+@end table
 ETEXI
 
 DEF("readconfig", HAS_ARG, QEMU_OPTION_readconfig,
diff --git a/qemu-seccomp.c b/qemu-seccomp.c
index 585de42a97..3e3f15cc08 100644
--- a/qemu-seccomp.c
+++ b/qemu-seccomp.c
@@ -56,9 +56,22 @@  static const struct QemuSeccompSyscall blacklist[] = {
     { SCMP_SYS(tuxcall),               1, QEMU_SECCOMP_SET_DEFAULT },
     { SCMP_SYS(ulimit),                1, QEMU_SECCOMP_SET_DEFAULT },
     { SCMP_SYS(vserver),               1, QEMU_SECCOMP_SET_DEFAULT },
+    /* obsolete */
+    { SCMP_SYS(readdir),               2, QEMU_SECCOMP_SET_OBSOLETE },
+    { SCMP_SYS(_sysctl),               2, QEMU_SECCOMP_SET_OBSOLETE },
+    { SCMP_SYS(bdflush),               2, QEMU_SECCOMP_SET_OBSOLETE },
+    { SCMP_SYS(create_module),         2, QEMU_SECCOMP_SET_OBSOLETE },
+    { SCMP_SYS(get_kernel_syms),       2, QEMU_SECCOMP_SET_OBSOLETE },
+    { SCMP_SYS(query_module),          2, QEMU_SECCOMP_SET_OBSOLETE },
+    { SCMP_SYS(sgetmask),              2, QEMU_SECCOMP_SET_OBSOLETE },
+    { SCMP_SYS(ssetmask),              2, QEMU_SECCOMP_SET_OBSOLETE },
+    { SCMP_SYS(sysfs),                 2, QEMU_SECCOMP_SET_OBSOLETE },
+    { SCMP_SYS(uselib),                2, QEMU_SECCOMP_SET_OBSOLETE },
+    { SCMP_SYS(ustat),                 2, QEMU_SECCOMP_SET_OBSOLETE },
 };
 
-int seccomp_start(void)
+
+int seccomp_start(uint32_t seccomp_opts)
 {
     int rc = 0;
     unsigned int i = 0;
@@ -72,6 +85,14 @@  int seccomp_start(void)
 
     for (i = 0; i < ARRAY_SIZE(blacklist); i++) {
         switch (blacklist[i].set) {
+        case QEMU_SECCOMP_SET_OBSOLETE:
+            if (!(seccomp_opts & QEMU_SECCOMP_SET_OBSOLETE)) {
+                goto add_syscall;
+            } else {
+                continue;
+            }
+
+            break;
         default:
             goto add_syscall;
         }
diff --git a/vl.c b/vl.c
index 305531aba8..ca267f9918 100644
--- a/vl.c
+++ b/vl.c
@@ -271,6 +271,10 @@  static QemuOptsList qemu_sandbox_opts = {
             .name = "enable",
             .type = QEMU_OPT_BOOL,
         },
+        {
+            .name = "obsolete",
+            .type = QEMU_OPT_STRING,
+        },
         { /* end of list */ }
     },
 };
@@ -1032,7 +1036,23 @@  static int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
 {
     if (qemu_opt_get_bool(opts, "enable", false)) {
 #ifdef CONFIG_SECCOMP
-        if (seccomp_start() < 0) {
+        uint32_t seccomp_opts = 0x00000;
+        const char *value = NULL;
+
+        value = qemu_opt_get(opts, "obsolete");
+        if (value) {
+            if (strcmp(value, "allow") == 0) {
+                seccomp_opts |= QEMU_SECCOMP_SET_OBSOLETE;
+            } else if (strcmp(value, "deny")) {
+                /* this is the default option, this if is here
+		 * to provide a little bit of consistency for
+		 * the command line */
+	    } else {
+		error_report("invalid argument for obsolete");
+	    }
+        }
+
+        if (seccomp_start(seccomp_opts) < 0) {
             error_report("failed to install seccomp syscall filter "
                          "in the kernel");
             return -1;