On Mon, Sep 03, 2012 at 02:03:39PM +0200, Ján Tomko wrote: > QEMU (since 1.2-rc0) supports setting up a syscall whitelist through > libseccomp on linux kernel from 3.5-rc1. This is enabled by specifying > -sandbox on on qemu command line. > > This patch detects this capability by searching for -sandbox in qemu > help output and runs qemu with -sandbox on if sandbox is set to non-zero > in qemu.conf. > > --- > Should this option be in qemu.conf, or would it be better to set it > per-domain in the XML? > --- > src/qemu/qemu.conf | 6 ++++++ > src/qemu/qemu_capabilities.c | 3 +++ > src/qemu/qemu_capabilities.h | 1 + > src/qemu/qemu_command.c | 3 +++ > src/qemu/qemu_conf.c | 5 +++++ > src/qemu/qemu_conf.h | 1 + > 6 files changed, 19 insertions(+), 0 deletions(-) > > diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf > index d3175fa..47e510e 100644 > --- a/src/qemu/qemu.conf > +++ b/src/qemu/qemu.conf > @@ -375,3 +375,9 @@ > # > #keepalive_interval = 5 > #keepalive_count = 5 > + > + > + > +# Enable this to use seccomp syscall whitelisting in QEMU. > +# > +#sandbox = 1 > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c > index 2ba7956..b0728e8 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -176,6 +176,7 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST, > "disable-s3", > > "disable-s4", /* 105 */ > + "sandbox" > ); > > struct qemu_feature_flags { > @@ -1139,6 +1140,8 @@ qemuCapsComputeCmdFlags(const char *help, > } > if (strstr(help, "-smbios type")) > qemuCapsSet(flags, QEMU_CAPS_SMBIOS_TYPE); > + if (strstr(help, "-sandbox")) > + qemuCapsSet(flags, QEMU_CAPS_SANDBOX); > > if ((netdev = strstr(help, "-netdev"))) { > /* Disable -netdev on 0.12 since although it exists, > diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h > index a7b3a06..0066901 100644 > --- a/src/qemu/qemu_capabilities.h > +++ b/src/qemu/qemu_capabilities.h > @@ -141,6 +141,7 @@ enum qemuCapsFlags { > QEMU_CAPS_IOLIMITS = 103, /* -device ...logical_block_size & co */ > QEMU_CAPS_DISABLE_S3 = 104, /* S3 BIOS Advertisement on/off */ > QEMU_CAPS_DISABLE_S4 = 105, /* S4 BIOS Advertisement on/off */ > + QEMU_CAPS_SANDBOX = 106, /* -sandbox */ > > QEMU_CAPS_LAST, /* this must always be the last item */ > }; > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index e739f34..737d4d9 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -6462,6 +6462,9 @@ qemuBuildCommandLine(virConnectPtr conn, > ? qemucmd->env_value[i] : ""); > } > > + if (driver->sandbox && qemuCapsGet(qemuCaps, QEMU_CAPS_SANDBOX)) > + virCommandAddArgList(cmd, "-sandbox", "on", NULL); > + > return cmd; > > no_memory: > diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c > index e9e15c5..a367fcd 100644 > --- a/src/qemu/qemu_conf.c > +++ b/src/qemu/qemu_conf.c > @@ -129,6 +129,7 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, > > driver->keepAliveInterval = 5; > driver->keepAliveCount = 5; > + driver->sandbox = false; > > /* Just check the file is readable before opening it, otherwise > * libvirt emits an error. > @@ -570,6 +571,10 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, > CHECK_TYPE("keepalive_count", VIR_CONF_LONG); > if (p) driver->keepAliveCount = p->l; > > + p = virConfGetValue(conf, "sandbox"); > + CHECK_TYPE("sandbox", VIR_CONF_LONG); > + if (p) driver->sandbox = p->l; > + > virConfFree (conf); > return 0; > } > diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h > index ac285f6..f1b6465 100644 > --- a/src/qemu/qemu_conf.h > +++ b/src/qemu/qemu_conf.h > @@ -152,6 +152,7 @@ struct qemud_driver { > > int keepAliveInterval; > unsigned int keepAliveCount; > + bool sandbox; > }; > > typedef struct _qemuDomainCmdlineDef qemuDomainCmdlineDef; As-is the patch looks fine to me, now the real question as you pointed out is do we want to enforce that at the guest level. In general, if available sandboxing should be turned on unless we hit a bug, so if it work as expected, it should always be on, which to me would be an indication to have that as a global default for the driver (and on by default). If you have to rely on the user explicit setting to activate it, it won't be activated, if security implementations are good enough they are better off as default settings IMHO,

So ACK to this, except I would change src/qemu/qemu.conf patch to enable it by default, i.e. remove the leading # ... then testing will tell us if we can keep it on. Daniel