* Re: [Qemu-devel] [PATCH v3] sandbox: disable -sandbox if CONFIG_SECCOMP undefined
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:39 ` Eduardo Otubo
2 siblings, 1 reply; 12+ messages in thread
From: Ján Tomko @ 2018-05-29 8:40 UTC (permalink / raw)
To: Yi Min Zhao; +Cc: qemu-devel, otubo, fiuczy, borntraeger, jferlan, pbonzini
[-- Attachment #1: Type: text/plain, Size: 1706 bytes --]
On Tue, May 29, 2018 at 03:31:40PM +0800, 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>
>---
>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(-)
>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Tested-by: Ján Tomko <jtomko@redhat.com>
Jano
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v3] sandbox: disable -sandbox if CONFIG_SECCOMP undefined
2018-05-29 8:40 ` Ján Tomko
@ 2018-05-29 9:01 ` Yi Min Zhao
0 siblings, 0 replies; 12+ messages in thread
From: Yi Min Zhao @ 2018-05-29 9:01 UTC (permalink / raw)
To: Ján Tomko; +Cc: otubo, fiuczy, qemu-devel, borntraeger, jferlan, pbonzini
在 2018/5/29 下午4:40, Ján Tomko 写道:
> On Tue, May 29, 2018 at 03:31:40PM +0800, 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>
>> ---
>> 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(-)
>>
>
> Reviewed-by: Ján Tomko <jtomko@redhat.com>
> Tested-by: Ján Tomko <jtomko@redhat.com>
>
> Jano
Thanks very much!
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v3] sandbox: disable -sandbox if CONFIG_SECCOMP undefined
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:37 ` Paolo Bonzini
2018-05-29 9:45 ` Yi Min Zhao
2018-05-29 10:05 ` Yi Min Zhao
2018-05-29 9:39 ` Eduardo Otubo
2 siblings, 2 replies; 12+ messages in thread
From: Paolo Bonzini @ 2018-05-29 9:37 UTC (permalink / raw)
To: Yi Min Zhao, qemu-devel
Cc: jtomko, jferlan, berrange, otubo, borntraeger, fiuczy
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
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)) {
>
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH v3] sandbox: disable -sandbox if CONFIG_SECCOMP undefined
2018-05-29 9:37 ` Paolo Bonzini
@ 2018-05-29 9:45 ` Yi Min Zhao
2018-05-29 10:05 ` Yi Min Zhao
1 sibling, 0 replies; 12+ messages in thread
From: Yi Min Zhao @ 2018-05-29 9:45 UTC (permalink / raw)
To: qemu-devel
在 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
> 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
I have updated this. Thanks for your reminder! Could I add your s-o-b,
r-b or ack?
>
>> ---
>> 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)) {
>>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH v3] sandbox: disable -sandbox if CONFIG_SECCOMP undefined
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
1 sibling, 1 reply; 12+ messages in thread
From: Yi Min Zhao @ 2018-05-29 10:05 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: otubo, fiuczy, jtomko, borntraeger, jferlan
在 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 ?
> 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)) {
>>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH v3] sandbox: disable -sandbox if CONFIG_SECCOMP undefined
2018-05-29 10:05 ` Yi Min Zhao
@ 2018-05-30 10:54 ` Eduardo Otubo
2018-05-31 3:20 ` Yi Min Zhao
2018-05-31 3:24 ` Yi Min Zhao
0 siblings, 2 replies; 12+ messages in thread
From: Eduardo Otubo @ 2018-05-30 10:54 UTC (permalink / raw)
To: Yi Min Zhao
Cc: Paolo Bonzini, qemu-devel, borntraeger, fiuczy, jtomko, jferlan
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.
> > 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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH v3] sandbox: disable -sandbox if CONFIG_SECCOMP undefined
2018-05-30 10:54 ` Eduardo Otubo
@ 2018-05-31 3:20 ` Yi Min Zhao
2018-05-31 3:24 ` Yi Min Zhao
1 sibling, 0 replies; 12+ messages in thread
From: Yi Min Zhao @ 2018-05-31 3:20 UTC (permalink / raw)
To: Eduardo Otubo
Cc: Paolo Bonzini, qemu-devel, borntraeger, fiuczy, jtomko, jferlan
在 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)) {
>>>>
>>>
>>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH v3] sandbox: disable -sandbox if CONFIG_SECCOMP undefined
2018-05-30 10:54 ` Eduardo Otubo
2018-05-31 3:20 ` Yi Min Zhao
@ 2018-05-31 3:24 ` Yi Min Zhao
1 sibling, 0 replies; 12+ messages in thread
From: Yi Min Zhao @ 2018-05-31 3:24 UTC (permalink / raw)
To: Eduardo Otubo
Cc: Paolo Bonzini, qemu-devel, borntraeger, fiuczy, jtomko, jferlan
在 2018/5/30 下午6:54, Eduardo Otubo 写道:
> Can you send a v4 with a cleaned up version? Also fixing a typo on the text
> (elevateDprivileges).
Why elevateDprivileges? The option name in the code is 'elevateprivileges'.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v3] sandbox: disable -sandbox if CONFIG_SECCOMP undefined
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:37 ` Paolo Bonzini
@ 2018-05-29 9:39 ` Eduardo Otubo
2018-05-29 9:53 ` Yi Min Zhao
2 siblings, 1 reply; 12+ messages in thread
From: Eduardo Otubo @ 2018-05-29 9:39 UTC (permalink / raw)
To: Yi Min Zhao
Cc: qemu-devel, pbonzini, jtomko, jferlan, berrange, borntraeger,
fiuczy
On 29/05/2018 - 15:31:40, 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>
> ---
> 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)) {
> --
> Yi Min
>
Acked-by: Eduardo Otubo <otubo@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH v3] sandbox: disable -sandbox if CONFIG_SECCOMP undefined
2018-05-29 9:39 ` Eduardo Otubo
@ 2018-05-29 9:53 ` Yi Min Zhao
2018-05-29 10:14 ` Paolo Bonzini
0 siblings, 1 reply; 12+ messages in thread
From: Yi Min Zhao @ 2018-05-29 9:53 UTC (permalink / raw)
To: Eduardo Otubo; +Cc: fiuczy, borntraeger, qemu-devel, jtomko, jferlan, pbonzini
在 2018/5/29 下午5:39, Eduardo Otubo 写道:
> On 29/05/2018 - 15:31:40, 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>
>> ---
>> 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);
>> }
>> I
>> +#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)) {
>> --
>> Yi Min
>>
> Acked-by: Eduardo Otubo <otubo@redhat.com>
>
>
Thanks! I have included Paolo's squashing code. I'm not sure what I
should do next, sending a new version?
^ permalink raw reply [flat|nested] 12+ messages in thread