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 <zyi...@linux.ibm.com>
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
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)) {
>
