From: Yi Min Zhao <zyimin@linux.ibm.com>
To: Eduardo Otubo <otubo@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
qemu-devel@nongnu.org, borntraeger@de.ibm.com,
fiuczy@linux.ibm.com, jtomko@redhat.com, jferlan@redhat.com
Subject: Re: [Qemu-devel] [PATCH v3] sandbox: disable -sandbox if CONFIG_SECCOMP undefined
Date: Thu, 31 May 2018 11:20:40 +0800 [thread overview]
Message-ID: <c0e5fdde-36cf-3441-e68c-bc523f9f154b@linux.ibm.com> (raw)
In-Reply-To: <20180530105407.GB19798@vader>
在 2018/5/30 下午6:54, Eduardo Otubo 写道:
> 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 <zyimin@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
>> 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.
Sure. Thanks!
>
>>> 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)) {
>>>>
>>>
>>
next prev parent reply other threads:[~2018-05-31 3:20 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-29 7:31 [Qemu-devel] [PATCH v3] sandbox: disable -sandbox if CONFIG_SECCOMP undefined Yi Min Zhao
2018-05-29 8:40 ` Ján Tomko
2018-05-29 9:01 ` Yi Min Zhao
2018-05-29 9:37 ` Paolo Bonzini
2018-05-29 9:45 ` Yi Min Zhao
2018-05-29 10:05 ` Yi Min Zhao
2018-05-30 10:54 ` Eduardo Otubo
2018-05-31 3:20 ` Yi Min Zhao [this message]
2018-05-31 3:24 ` Yi Min Zhao
2018-05-29 9:39 ` Eduardo Otubo
2018-05-29 9:53 ` Yi Min Zhao
2018-05-29 10:14 ` Paolo Bonzini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c0e5fdde-36cf-3441-e68c-bc523f9f154b@linux.ibm.com \
--to=zyimin@linux.ibm.com \
--cc=borntraeger@de.ibm.com \
--cc=fiuczy@linux.ibm.com \
--cc=jferlan@redhat.com \
--cc=jtomko@redhat.com \
--cc=otubo@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).