Re: [PATCH v0] qemu: Add sandbox support.

[Corey Bryant <coreyb@xxxxxxxxxxxxxxxxxx>]

On 09/06/2012 02:27 AM, Daniel Veillard wrote:
> 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,

I agree, it should be on by default and turned off only if there's a 
problem with the syscall whitelist.  However, note what I said in my 
previous email that we don't plan to default to on until QEMU 1.3.

--
Regards,
Corey

>    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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list