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

[Corey Bryant]

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
>