[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3] sandbox: disable -sandbox if CONFIG_SECCOMP
|
From: |
Eduardo Otubo |
|
Subject: |
Re: [Qemu-devel] [PATCH v3] sandbox: disable -sandbox if CONFIG_SECCOMP undefined |
|
Date: |
Wed, 30 May 2018 12:54:07 +0200 |
|
User-agent: |
Mutt/1.8.3+47 (5f034395e53d) (2017-05-23) |
On 29/05/2018 - 18:05:25, Yi Min Zhao wrote:
>
>
> 在 2018/5/29 下午5:37, Paolo Bonzini 写道:
> > On 29/05/2018 09:31, Yi Min Zhao wrote:
> > > If CONFIG_SECCOMP is undefined, the option 'elevateprivileges' remains
> > > compiled. This would make libvirt set the corresponding capability and
> > > then trigger failure during guest startup. This patch moves the code
> > > regarding seccomp command line options to qemu-seccomp.c file and
> > > wraps qemu_opts_foreach finding sandbox option with CONFIG_SECCOMP.
> > > Because parse_sandbox() is moved into qemu-seccomp.c file, change
> > > seccomp_start() to static function.
> > >
> > > Signed-off-by: Yi Min Zhao <address@hidden>
> > I had to squash this in:
> >
> > diff --git a/vl.c b/vl.c
> > index 1140feb227..66c17ff8f8 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -3842,11 +3842,16 @@ int main(int argc, char **argv, char **envp)
> > qtest_log = optarg;
> > break;
> > case QEMU_OPTION_sandbox:
> > +#ifndef CONFIG_SECCOMP
> One question, I guess you want to use #ifdef ?
Yep, I guess he meant #ifdef.
Can you send a v4 with a cleaned up version? Also fixing a typo on the text
(elevateDprivileges).
Thanks for the contribution.
> > opts = qemu_opts_parse_noisily(qemu_find_opts("sandbox"),
> > optarg, true);
> > if (!opts) {
> > exit(1);
> > }
> > +#else
> > + error_report("-sandbox support is not enabled in this QEMU
> > binary");
> > + exit(1);
> > +#endif
> > break;
> > case QEMU_OPTION_add_fd:
> > #ifndef _WIN32
> >
> >
> > Otherwise "-sandbox" will crash with a NULL pointer dereference in a binary
> > without
> > seccomp support. Otherwise looks great, thanks!
> >
> > Paolo
> >
> > > ---
> > > 1. Problem Description
> > > ======================
> > > If QEMU is built without seccomp support, 'elevateprivileges' remains
> > > compiled.
> > > This option of sandbox is treated as an indication for seccomp blacklist
> > > support
> > > in libvirt. This behavior is introduced by the libvirt commits 31ca6a5 and
> > > 3527f9d. It would make libvirt build wrong QEMU cmdline, and then the
> > > guest
> > > startup would fail.
> > >
> > > 2. Libvirt Log
> > > ==============
> > > qemu-system-s390x: -sandbox
> > > on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
> > > resourcecontrol=deny: seccomp support is disabled
> > >
> > > 3. Fixup
> > > ========
> > > Move the code related ot sandbox to qemu-seccomp.c file and wrap them with
> > > CONFIG_SECCOMP. So compile the code related to sandbox only when
> > > CONFIG_SECCOMP is defined.
> > > ---
> > > include/sysemu/seccomp.h | 3 +-
> > > qemu-seccomp.c | 121
> > > ++++++++++++++++++++++++++++++++++++++++++++++-
> > > vl.c | 118
> > > +--------------------------------------------
> > > 3 files changed, 124 insertions(+), 118 deletions(-)
> > >
> > > diff --git a/include/sysemu/seccomp.h b/include/sysemu/seccomp.h
> > > index 9b092aa23f..fe859894f6 100644
> > > --- a/include/sysemu/seccomp.h
> > > +++ b/include/sysemu/seccomp.h
> > > @@ -21,5 +21,6 @@
> > > #define QEMU_SECCOMP_SET_SPAWN (1 << 3)
> > > #define QEMU_SECCOMP_SET_RESOURCECTL (1 << 4)
> > > -int seccomp_start(uint32_t seccomp_opts);
> > > +int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp);
> > > +
> > > #endif
> > > diff --git a/qemu-seccomp.c b/qemu-seccomp.c
> > > index b770a77d33..148e4c6f24 100644
> > > --- a/qemu-seccomp.c
> > > +++ b/qemu-seccomp.c
> > > @@ -13,6 +13,11 @@
> > > * GNU GPL, version 2 or (at your option) any later version.
> > > */
> > > #include "qemu/osdep.h"
> > > +#include "qemu/config-file.h"
> > > +#include "qemu/option.h"
> > > +#include "qemu/module.h"
> > > +#include "qemu/error-report.h"
> > > +#include <sys/prctl.h>
> > > #include <seccomp.h>
> > > #include "sysemu/seccomp.h"
> > > @@ -96,7 +101,7 @@ static const struct QemuSeccompSyscall blacklist[] = {
> > > };
> > > -int seccomp_start(uint32_t seccomp_opts)
> > > +static int seccomp_start(uint32_t seccomp_opts)
> > > {
> > > int rc = 0;
> > > unsigned int i = 0;
> > > @@ -125,3 +130,117 @@ int seccomp_start(uint32_t seccomp_opts)
> > > seccomp_release(ctx);
> > > return rc;
> > > }
> > > +
> > > +#ifdef CONFIG_SECCOMP
> > > +int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
> > > +{
> > > + if (qemu_opt_get_bool(opts, "enable", false)) {
> > > + uint32_t seccomp_opts = QEMU_SECCOMP_SET_DEFAULT
> > > + | QEMU_SECCOMP_SET_OBSOLETE;
> > > + const char *value = NULL;
> > > +
> > > + value = qemu_opt_get(opts, "obsolete");
> > > + if (value) {
> > > + if (g_str_equal(value, "allow")) {
> > > + seccomp_opts &= ~QEMU_SECCOMP_SET_OBSOLETE;
> > > + } else if (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");
> > > + return -1;
> > > + }
> > > + }
> > > +
> > > + value = qemu_opt_get(opts, "elevateprivileges");
> > > + if (value) {
> > > + if (g_str_equal(value, "deny")) {
> > > + seccomp_opts |= QEMU_SECCOMP_SET_PRIVILEGED;
> > > + } else if (g_str_equal(value, "children")) {
> > > + seccomp_opts |= QEMU_SECCOMP_SET_PRIVILEGED;
> > > +
> > > + /* calling prctl directly because we're
> > > + * not sure if host has CAP_SYS_ADMIN set*/
> > > + if (prctl(PR_SET_NO_NEW_PRIVS, 1)) {
> > > + error_report("failed to set no_new_privs "
> > > + "aborting");
> > > + return -1;
> > > + }
> > > + } else if (g_str_equal(value, "allow")) {
> > > + /* default value */
> > > + } else {
> > > + error_report("invalid argument for elevateprivileges");
> > > + return -1;
> > > + }
> > > + }
> > > +
> > > + value = qemu_opt_get(opts, "spawn");
> > > + if (value) {
> > > + if (g_str_equal(value, "deny")) {
> > > + seccomp_opts |= QEMU_SECCOMP_SET_SPAWN;
> > > + } else if (g_str_equal(value, "allow")) {
> > > + /* default value */
> > > + } else {
> > > + error_report("invalid argument for spawn");
> > > + return -1;
> > > + }
> > > + }
> > > +
> > > + value = qemu_opt_get(opts, "resourcecontrol");
> > > + if (value) {
> > > + if (g_str_equal(value, "deny")) {
> > > + seccomp_opts |= QEMU_SECCOMP_SET_RESOURCECTL;
> > > + } else if (g_str_equal(value, "allow")) {
> > > + /* default value */
> > > + } else {
> > > + error_report("invalid argument for resourcecontrol");
> > > + return -1;
> > > + }
> > > + }
> > > +
> > > + if (seccomp_start(seccomp_opts) < 0) {
> > > + error_report("failed to install seccomp syscall filter "
> > > + "in the kernel");
> > > + return -1;
> > > + }
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static QemuOptsList qemu_sandbox_opts = {
> > > + .name = "sandbox",
> > > + .implied_opt_name = "enable",
> > > + .head = QTAILQ_HEAD_INITIALIZER(qemu_sandbox_opts.head),
> > > + .desc = {
> > > + {
> > > + .name = "enable",
> > > + .type = QEMU_OPT_BOOL,
> > > + },
> > > + {
> > > + .name = "obsolete",
> > > + .type = QEMU_OPT_STRING,
> > > + },
> > > + {
> > > + .name = "elevateprivileges",
> > > + .type = QEMU_OPT_STRING,
> > > + },
> > > + {
> > > + .name = "spawn",
> > > + .type = QEMU_OPT_STRING,
> > > + },
> > > + {
> > > + .name = "resourcecontrol",
> > > + .type = QEMU_OPT_STRING,
> > > + },
> > > + { /* end of list */ }
> > > + },
> > > +};
> > > +
> > > +static void seccomp_register(void)
> > > +{
> > > + qemu_add_opts(&qemu_sandbox_opts);
> > > +}
> > > +opts_init(seccomp_register);
> > > +#endif
> > > diff --git a/vl.c b/vl.c
> > > index 806eec2ef6..ea04aa2e2b 100644
> > > --- a/vl.c
> > > +++ b/vl.c
> > > @@ -28,11 +28,7 @@
> > > #include "qemu/cutils.h"
> > > #include "qemu/help_option.h"
> > > #include "qemu/uuid.h"
> > > -
> > > -#ifdef CONFIG_SECCOMP
> > > -#include <sys/prctl.h>
> > > #include "sysemu/seccomp.h"
> > > -#endif
> > > #ifdef CONFIG_SDL
> > > #if defined(__APPLE__) || defined(main)
> > > @@ -257,35 +253,6 @@ static QemuOptsList qemu_rtc_opts = {
> > > },
> > > };
> > > -static QemuOptsList qemu_sandbox_opts = {
> > > - .name = "sandbox",
> > > - .implied_opt_name = "enable",
> > > - .head = QTAILQ_HEAD_INITIALIZER(qemu_sandbox_opts.head),
> > > - .desc = {
> > > - {
> > > - .name = "enable",
> > > - .type = QEMU_OPT_BOOL,
> > > - },
> > > - {
> > > - .name = "obsolete",
> > > - .type = QEMU_OPT_STRING,
> > > - },
> > > - {
> > > - .name = "elevateprivileges",
> > > - .type = QEMU_OPT_STRING,
> > > - },
> > > - {
> > > - .name = "spawn",
> > > - .type = QEMU_OPT_STRING,
> > > - },
> > > - {
> > > - .name = "resourcecontrol",
> > > - .type = QEMU_OPT_STRING,
> > > - },
> > > - { /* end of list */ }
> > > - },
> > > -};
> > > -
> > > static QemuOptsList qemu_option_rom_opts = {
> > > .name = "option-rom",
> > > .implied_opt_name = "romfile",
> > > @@ -1041,88 +1008,6 @@ static int bt_parse(const char *opt)
> > > return 1;
> > > }
> > > -static int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
> > > -{
> > > - if (qemu_opt_get_bool(opts, "enable", false)) {
> > > -#ifdef CONFIG_SECCOMP
> > > - uint32_t seccomp_opts = QEMU_SECCOMP_SET_DEFAULT
> > > - | QEMU_SECCOMP_SET_OBSOLETE;
> > > - const char *value = NULL;
> > > -
> > > - value = qemu_opt_get(opts, "obsolete");
> > > - if (value) {
> > > - if (g_str_equal(value, "allow")) {
> > > - seccomp_opts &= ~QEMU_SECCOMP_SET_OBSOLETE;
> > > - } else if (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");
> > > - return -1;
> > > - }
> > > - }
> > > -
> > > - value = qemu_opt_get(opts, "elevateprivileges");
> > > - if (value) {
> > > - if (g_str_equal(value, "deny")) {
> > > - seccomp_opts |= QEMU_SECCOMP_SET_PRIVILEGED;
> > > - } else if (g_str_equal(value, "children")) {
> > > - seccomp_opts |= QEMU_SECCOMP_SET_PRIVILEGED;
> > > -
> > > - /* calling prctl directly because we're
> > > - * not sure if host has CAP_SYS_ADMIN set*/
> > > - if (prctl(PR_SET_NO_NEW_PRIVS, 1)) {
> > > - error_report("failed to set no_new_privs "
> > > - "aborting");
> > > - return -1;
> > > - }
> > > - } else if (g_str_equal(value, "allow")) {
> > > - /* default value */
> > > - } else {
> > > - error_report("invalid argument for elevateprivileges");
> > > - return -1;
> > > - }
> > > - }
> > > -
> > > - value = qemu_opt_get(opts, "spawn");
> > > - if (value) {
> > > - if (g_str_equal(value, "deny")) {
> > > - seccomp_opts |= QEMU_SECCOMP_SET_SPAWN;
> > > - } else if (g_str_equal(value, "allow")) {
> > > - /* default value */
> > > - } else {
> > > - error_report("invalid argument for spawn");
> > > - return -1;
> > > - }
> > > - }
> > > -
> > > - value = qemu_opt_get(opts, "resourcecontrol");
> > > - if (value) {
> > > - if (g_str_equal(value, "deny")) {
> > > - seccomp_opts |= QEMU_SECCOMP_SET_RESOURCECTL;
> > > - } else if (g_str_equal(value, "allow")) {
> > > - /* default value */
> > > - } else {
> > > - error_report("invalid argument for resourcecontrol");
> > > - return -1;
> > > - }
> > > - }
> > > -
> > > - if (seccomp_start(seccomp_opts) < 0) {
> > > - error_report("failed to install seccomp syscall filter "
> > > - "in the kernel");
> > > - return -1;
> > > - }
> > > -#else
> > > - error_report("seccomp support is disabled");
> > > - return -1;
> > > -#endif
> > > - }
> > > -
> > > - return 0;
> > > -}
> > > -
> > > static int parse_name(void *opaque, QemuOpts *opts, Error **errp)
> > > {
> > > const char *proc_name;
> > > @@ -3074,7 +2959,6 @@ int main(int argc, char **argv, char **envp)
> > > qemu_add_opts(&qemu_mem_opts);
> > > qemu_add_opts(&qemu_smp_opts);
> > > qemu_add_opts(&qemu_boot_opts);
> > > - qemu_add_opts(&qemu_sandbox_opts);
> > > qemu_add_opts(&qemu_add_fd_opts);
> > > qemu_add_opts(&qemu_object_opts);
> > > qemu_add_opts(&qemu_tpmdev_opts);
> > > @@ -4071,10 +3955,12 @@ int main(int argc, char **argv, char **envp)
> > > exit(1);
> > > }
> > > +#ifdef CONFIG_SECCOMP
> > > if (qemu_opts_foreach(qemu_find_opts("sandbox"),
> > > parse_sandbox, NULL, NULL)) {
> > > exit(1);
> > > }
> > > +#endif
> > > if (qemu_opts_foreach(qemu_find_opts("name"),
> > > parse_name, NULL, NULL)) {
> > >
> >
> >
>
>
--
Eduardo Otubo