[RFC,v4,06/20] core: add new security-policy object

The object can be used to define global security policy for the guest.

object provides two properties:

 1) debug: can be used to disable guest memory access from hypervisor.

   e.g to disable guest memory debug accesses

    # $QEMU \
          -object security-policy,debug=false,id=mypolicy \
          -machine ...,security-policy=mypolicy

 2) memory-encryption: if hypervisor supports memory encryption then this
    property can be used to define object for encryption.

    # $QEMU \
        -object sev-guest,id=sev0 \
        -object security-policy,id=memory-encryption=sev0,id=mypolicy \
        -machine ...,security-policy=mypolicy

The memory-encryption property will be used for enabling AMD's SEV feature.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 exec.c                           |    7 ++
 hw/core/Makefile.objs            |    1 
 hw/core/machine.c                |   22 +++++
 hw/core/security-policy.c        |  165 ++++++++++++++++++++++++++++++++++++++
 include/hw/boards.h              |    1 
 include/sysemu/security-policy.h |   75 +++++++++++++++++
 qemu-options.hx                  |   21 +++++
 7 files changed, 292 insertions(+)
 create mode 100644 hw/core/security-policy.c
 create mode 100644 include/sysemu/security-policy.h

On Wed, Mar 08, 2017 at 03:52:09PM -0500, Brijesh Singh wrote:
> The object can be used to define global security policy for the guest.

"security-policy" is very vague.  Lots of parts of QEMU have security
related options (e.g. VNC display, networking, etc).

I'd prefer a
-machine memory-encryption=on|off,memory-encryption-debug=on|off
or -m encryption=on|off,encryption-debug=on|off switch instead of a new
security policy object with questionable scope.

> object provides two properties:
> 
>  1) debug: can be used to disable guest memory access from hypervisor.
> 
>    e.g to disable guest memory debug accesses
> 
>     # $QEMU \
>           -object security-policy,debug=false,id=mypolicy \
>           -machine ...,security-policy=mypolicy
> 
>  2) memory-encryption: if hypervisor supports memory encryption then this
>     property can be used to define object for encryption.
> 
>     # $QEMU \
>         -object sev-guest,id=sev0 \
>         -object security-policy,id=memory-encryption=sev0,id=mypolicy \

s/id=memory-encryption=/id=mypolicy,memory-encryption=/

>         -machine ...,security-policy=mypolicy
> 
> The memory-encryption property will be used for enabling AMD's SEV feature.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  exec.c                           |    7 ++
>  hw/core/Makefile.objs            |    1 
>  hw/core/machine.c                |   22 +++++
>  hw/core/security-policy.c        |  165 ++++++++++++++++++++++++++++++++++++++
>  include/hw/boards.h              |    1 
>  include/sysemu/security-policy.h |   75 +++++++++++++++++
>  qemu-options.hx                  |   21 +++++
>  7 files changed, 292 insertions(+)
>  create mode 100644 hw/core/security-policy.c
>  create mode 100644 include/sysemu/security-policy.h
> 
> diff --git a/exec.c b/exec.c
> index 772a959..2c7c891 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -40,6 +40,7 @@
>  #else /* !CONFIG_USER_ONLY */
>  #include "hw/hw.h"
>  #include "exec/memory.h"
> +#include "sysemu/security-policy.h"
>  #include "exec/ioport.h"
>  #include "sysemu/dma.h"
>  #include "sysemu/numa.h"
> @@ -2926,6 +2927,12 @@ static inline void cpu_physical_memory_rw_debug_internal(AddressSpace *as,
>      hwaddr addr1;
>      MemoryRegion *mr;
>  
> +    /* Check if debug accesses is allowed */
> +    if (attrs.debug &&
> +        !security_policy_debug_allowed(current_machine->security_policy)) {
> +        return;
> +    }
> +
>      rcu_read_lock();
>      while (len > 0) {
>          l = len;
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index 91450b2..3c413b1 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -18,6 +18,7 @@ common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
>  common-obj-$(CONFIG_SOFTMMU) += register.o
>  common-obj-$(CONFIG_SOFTMMU) += or-irq.o
>  common-obj-$(CONFIG_PLATFORM_BUS) += platform-bus.o
> +common-obj-$(CONFIG_SOFTMMU) += security-policy.o
>  
>  obj-$(CONFIG_SOFTMMU) += generic-loader.o
>  obj-$(CONFIG_SOFTMMU) += null-machine.o
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 0699750..c14f59c 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -332,6 +332,23 @@ static bool machine_get_enforce_config_section(Object *obj, Error **errp)
>      return ms->enforce_config_section;
>  }
>  
> +static char *machine_get_security_policy(Object *obj, Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +
> +    return g_strdup(ms->security_policy);
> +}
> +
> +static void machine_set_security_policy(Object *obj,
> +                                        const char *value, Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +
> +    g_free(ms->security_policy);
> +    ms->security_policy = g_strdup(value);
> +}
> +
> +
>  static void error_on_sysbus_device(SysBusDevice *sbdev, void *opaque)
>  {
>      error_report("Option '-device %s' cannot be handled by this machine",
> @@ -493,6 +510,11 @@ static void machine_class_init(ObjectClass *oc, void *data)
>          &error_abort);
>      object_class_property_set_description(oc, "enforce-config-section",
>          "Set on to enforce configuration section migration", &error_abort);
> +
> +    object_class_property_add_str(oc, "security-policy",
> +            machine_get_security_policy, machine_set_security_policy, NULL);
> +    object_class_property_set_description(oc, "security-policy",
> +            "Set the security policy for the machine", NULL);
>  }
>  
>  static void machine_class_base_init(ObjectClass *oc, void *data)
> diff --git a/hw/core/security-policy.c b/hw/core/security-policy.c
> new file mode 100644
> index 0000000..4d4658e
> --- /dev/null
> +++ b/hw/core/security-policy.c
> @@ -0,0 +1,165 @@
> +/*
> + * QEMU security policy support
> + *
> + * Copyright (c) 2016 Advanced Micro Devices
> + *
> + * Author:
> + *      Brijesh Singh <brijesh.singh@amd.com>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qom/object_interfaces.h"
> +#include "qemu/base64.h"
> +
> +#include "sysemu/security-policy.h"
> +
> +static SecurityPolicy *
> +find_security_policy_obj(const char *name)
> +{
> +    Object *obj;
> +    SecurityPolicy *policy;
> +
> +    if (!name) {
> +        return NULL;
> +    }
> +
> +    obj = object_resolve_path_component(
> +        object_get_objects_root(), name);
> +    if (!obj) {
> +        return NULL;
> +    }
> +
> +    policy = (SecurityPolicy *)
> +        object_dynamic_cast(obj,
> +                            TYPE_SECURITY_POLICY);
> +    if (!policy) {
> +        return NULL;
> +    }
> +
> +    return policy;
> +}
> +
> +bool
> +security_policy_debug_allowed(const char *secure_policy_id)
> +{
> +    SecurityPolicy *policy = find_security_policy_obj(secure_policy_id);
> +
> +    /* if id is not a valid security policy then we return true */
> +    return policy ? policy->debug : true;
> +}
> +
> +char *
> +security_policy_get_memory_encryption_id(const char *secure_policy_id)
> +{
> +    SecurityPolicy *policy = find_security_policy_obj(secure_policy_id);
> +
> +    return policy ? g_strdup(policy->memory_encryption) : NULL;
> +}
> +
> +static bool
> +security_policy_prop_get_debug(Object *obj,
> +                               Error **errp G_GNUC_UNUSED)
> +{
> +    SecurityPolicy *policy = SECURITY_POLICY(obj);
> +
> +    return policy->debug;
> +}
> +
> +
> +static void
> +security_policy_prop_set_debug(Object *obj,
> +                               bool value,
> +                               Error **errp G_GNUC_UNUSED)
> +{
> +    SecurityPolicy *policy = SECURITY_POLICY(obj);
> +
> +    policy->debug = value;
> +}
> +
> +static char *
> +sev_launch_get_memory_encryption(Object *obj, Error **errp)
> +{
> +    SecurityPolicy *policy = SECURITY_POLICY(obj);
> +
> +    return g_strdup(policy->memory_encryption);
> +}
> +
> +static void
> +sev_launch_set_memory_encryption(Object *obj, const char *value,
> +                                 Error **errp)
> +{
> +    SecurityPolicy *policy = SECURITY_POLICY(obj);
> +
> +    policy->memory_encryption = g_strdup(value);
> +}
> +
> +static void
> +security_policy_init(Object *obj)
> +{
> +    SecurityPolicy *policy = SECURITY_POLICY(obj);
> +
> +    policy->debug = true;
> +}
> +
> +static void
> +security_policy_finalize(Object *obj)
> +{
> +}
> +
> +static void
> +security_policy_class_init(ObjectClass *oc, void *data)
> +{
> +    object_class_property_add_bool(oc, "debug",
> +                                   security_policy_prop_get_debug,
> +                                   security_policy_prop_set_debug,
> +                                   NULL);
> +    object_class_property_set_description(oc, "debug",
> +            "Set on/off if debugging is allowed on this guest (default on)",
> +            NULL);
> +    object_class_property_add_str(oc, "memory-encryption",
> +                                  sev_launch_get_memory_encryption,
> +                                  sev_launch_set_memory_encryption,
> +                                  NULL);
> +    object_class_property_set_description(oc, "memory-encryption",
> +            "Set memory encryption object id (if supported by hardware)",
> +            NULL);
> +}
> +
> +static const TypeInfo security_policy_info = {
> +    .parent = TYPE_OBJECT,
> +    .name = TYPE_SECURITY_POLICY,
> +    .instance_size = sizeof(SecurityPolicy),
> +    .instance_init = security_policy_init,
> +    .instance_finalize = security_policy_finalize,
> +    .class_size = sizeof(SecurityPolicyClass),
> +    .class_init = security_policy_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_USER_CREATABLE },
> +        { }
> +    }
> +};
> +
> +
> +static void
> +security_policy_register_types(void)
> +{
> +    type_register_static(&security_policy_info);
> +}
> +
> +
> +type_init(security_policy_register_types);
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 269d0ba..a1c99a0 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -153,6 +153,7 @@ struct MachineState {
>      /*< public >*/
>  
>      char *accel;
> +    char *security_policy;
>      bool kernel_irqchip_allowed;
>      bool kernel_irqchip_required;
>      bool kernel_irqchip_split;
> diff --git a/include/sysemu/security-policy.h b/include/sysemu/security-policy.h
> new file mode 100644
> index 0000000..6d3789d
> --- /dev/null
> +++ b/include/sysemu/security-policy.h
> @@ -0,0 +1,75 @@
> +/*
> + * QEMU security policy support
> + *
> + * Copyright (c) 2016 Advanced Micro Devices
> + *
> + * Author:
> + *      Brijesh Singh <brijesh.singh@amd.com>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#ifndef SECURITY_POLICY_H
> +#define SECURITY_POLICY_H
> +
> +#include "qom/object.h"
> +
> +#define TYPE_SECURITY_POLICY "security-policy"
> +#define SECURITY_POLICY(obj)                  \
> +    OBJECT_CHECK(SecurityPolicy, (obj), TYPE_SECURITY_POLICY)
> +
> +typedef struct SecurityPolicy SecurityPolicy;
> +typedef struct SecurityPolicyClass SecurityPolicyClass;
> +
> +/**
> + * SecurityPolicy:
> + *
> + * The SecurityPolicy object provides method to define
> + * various security releated policies for guest machine.
> + *
> + * e.g
> + * When launching QEMU, user can create a security policy
> + * to disallow memory dump and debug of guest
> + *
> + *  # $QEMU \
> + *      -object security-policy,id=mypolicy,debug=off \
> + *      -machine ...,security-policy=mypolicy
> + *
> + * If hardware supports memory encryption then user can set
> + * encryption policy of guest
> + *
> + * # $QEMU \
> + *    -object encrypt-policy,key=xxx,flags=xxxx,id=encrypt \
> + *    -object security-policy,debug=off,memory-encryption=encrypt,id=mypolicy \
> + *    -machine ...,security-policy=mypolicy
> + *
> + */
> +
> +struct SecurityPolicy {
> +    Object parent_obj;
> +
> +    bool debug;
> +    char *memory_encryption;
> +};
> +
> +
> +struct SecurityPolicyClass {
> +    ObjectClass parent_class;
> +};
> +
> +bool security_policy_debug_allowed(const char *name);
> +char *security_policy_get_memory_encryption_id(const char *name);
> +
> +#endif /* SECURITY_POLICY_H */
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 2292438..536db1b 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4140,6 +4140,27 @@ contents of @code{iv.b64} to the second secret
>  
>  @end table
>  
> +@item -object security-policy,id=@var{id}[,debug=@var{bool}][,memory-encryption=@var{string}]
> +
> +Create a security policy object, which can be used to define guest security.
> +The id parameter is a unique ID that will be used to reference this
> +object when security-policy is applied via -machine argument.
> +
> +The 'debug' parameter can be defined to tell whether the debugging or memory
> +dump is allowed through qemu monitor console.
> +
> +e.g to disable the guest memory dump
> +@example
> + # $QEMU \
> +     -object security-policy,id=secure0,debug=off \
> +     -machine ...,security-policy=secure0
> +@end example
> +
> +if hardware support guest memory encrytion, then 'memory-encryption' parameter
> +can be set to the unquie ID of memory encryption object.
> +
> +On AMD processor, memory encryption is supported via 'sev-guest' object.
> +
>  ETEXI
>  
>  
> 
>

Hi Stefan,


On 03/23/2017 06:35 AM, Stefan Hajnoczi wrote:
> On Wed, Mar 08, 2017 at 03:52:09PM -0500, Brijesh Singh wrote:
>> The object can be used to define global security policy for the guest.
>
> "security-policy" is very vague.  Lots of parts of QEMU have security
> related options (e.g. VNC display, networking, etc).
>
> I'd prefer a
> -machine memory-encryption=on|off,memory-encryption-debug=on|off
> or -m encryption=on|off,encryption-debug=on|off switch instead of a new
> security policy object with questionable scope.
>

In v1 [1], I had something similar but not exactly the same. I had a new command
line switch but the overall feedback was to consider creating new security object
which can be used to define a machine security policy.

[1] http://marc.info/?t=147378617700002&r=1&w=2

some more discussion here [2]

[2] http://marc.info/?t=147378241700011&r=1&w=2

IMHO, a new object is helpful because it provide options to launch a guest without
memory encryption support but still can take a advantage of disabling the debug
feature. e.g on non SEV platform we can launch guest with "-object security-policy,id=secure0,debug=off'
which will reject the guest memory accesses via gdbstub or qemu monitor command line interface.

-Brijesh

On Thu, Mar 23, 2017 at 01:59:48PM -0500, Brijesh Singh wrote:
> Hi Stefan,
> 
> 
> On 03/23/2017 06:35 AM, Stefan Hajnoczi wrote:
> > On Wed, Mar 08, 2017 at 03:52:09PM -0500, Brijesh Singh wrote:
> > > The object can be used to define global security policy for the guest.
> > 
> > "security-policy" is very vague.  Lots of parts of QEMU have security
> > related options (e.g. VNC display, networking, etc).
> > 
> > I'd prefer a
> > -machine memory-encryption=on|off,memory-encryption-debug=on|off
> > or -m encryption=on|off,encryption-debug=on|off switch instead of a new
> > security policy object with questionable scope.
> > 
> 
> In v1 [1], I had something similar but not exactly the same. I had a new command
> line switch but the overall feedback was to consider creating new security object
> which can be used to define a machine security policy.
> 
> [1] http://marc.info/?t=147378617700002&r=1&w=2
> 
> some more discussion here [2]
> 
> [2] http://marc.info/?t=147378241700011&r=1&w=2
> 
> IMHO, a new object is helpful because it provide options to launch a guest without
> memory encryption support but still can take a advantage of disabling the debug
> feature. e.g on non SEV platform we can launch guest with "-object security-policy,id=secure0,debug=off'
> which will reject the guest memory accesses via gdbstub or qemu monitor command line interface.

Having one security policy doesn't make sense to me.  As mentioned,
there are many different areas of QEMU that have security relevant
configuration.  They are all unrelated so combining them into one object
with vague parameter names like "debug" makes for a confusing
command-line interface.

If the object is called sev-security-policy then I'm happy.

Stefan

On 03/24/2017 10:40 AM, Stefan Hajnoczi wrote:

>
> Having one security policy doesn't make sense to me.  As mentioned,
> there are many different areas of QEMU that have security relevant
> configuration.  They are all unrelated so combining them into one object
> with vague parameter names like "debug" makes for a confusing
> command-line interface.
>
> If the object is called sev-security-policy then I'm happy.
>

Works for with me but one of the feedback was to use security-policy [1].
IIRC, the main reason for using 'security-policy' instead of 'sev-security-policy'
was to add a layer of abstraction so that in future if other platforms supports
memory encryption in slightly different way then all we need to do is to create
new object without needing to add a new parameter in -machine.

[1] http://marc.info/?l=qemu-devel&m=147388592213137&w=2

How about using 'memory-encryption-id' instead of security-policy ? If user wants
to launch SEV guest then memory-encryption-id should be set SEV specific object.
Something like this:

-machine ..,memory-encryption-id=sev0 \
-object sev-guest,id=sev,debug=off,launch=launch0 \
-object sev-launch-info,id=launch0 \


Thanks
Brijesh

On Fri, Mar 24, 2017 at 02:42:47PM -0500, Brijesh Singh wrote:
> 
> On 03/24/2017 10:40 AM, Stefan Hajnoczi wrote:
> 
> > 
> > Having one security policy doesn't make sense to me.  As mentioned,
> > there are many different areas of QEMU that have security relevant
> > configuration.  They are all unrelated so combining them into one object
> > with vague parameter names like "debug" makes for a confusing
> > command-line interface.
> > 
> > If the object is called sev-security-policy then I'm happy.
> > 
> 
> Works for with me but one of the feedback was to use security-policy [1].
> IIRC, the main reason for using 'security-policy' instead of 'sev-security-policy'
> was to add a layer of abstraction so that in future if other platforms supports
> memory encryption in slightly different way then all we need to do is to create
> new object without needing to add a new parameter in -machine.
> 
> [1] http://marc.info/?l=qemu-devel&m=147388592213137&w=2
> 
> How about using 'memory-encryption-id' instead of security-policy ? If user wants
> to launch SEV guest then memory-encryption-id should be set SEV specific object.
> Something like this:
> 
> -machine ..,memory-encryption-id=sev0 \
> -object sev-guest,id=sev,debug=off,launch=launch0 \
> -object sev-launch-info,id=launch0 \

Something like that sounds good.  I think "-id" typically isn't included
in the option name.

So just the following is fine:

 -machine memory-encryption=sev0 \
 ...

Other examples: -device virtio-blk-pci,drive=drive0 and -device
e1000,netdev=netdev0.

Stefan

On 03/27/2017 07:04 AM, Stefan Hajnoczi wrote:
> On Fri, Mar 24, 2017 at 02:42:47PM -0500, Brijesh Singh wrote:
>>
>> On 03/24/2017 10:40 AM, Stefan Hajnoczi wrote:
>>
>>>
>>> Having one security policy doesn't make sense to me.  As mentioned,
>>> there are many different areas of QEMU that have security relevant
>>> configuration.  They are all unrelated so combining them into one object
>>> with vague parameter names like "debug" makes for a confusing
>>> command-line interface.
>>>
>>> If the object is called sev-security-policy then I'm happy.
>>>
>>
>> Works for with me but one of the feedback was to use security-policy [1].
>> IIRC, the main reason for using 'security-policy' instead of 'sev-security-policy'
>> was to add a layer of abstraction so that in future if other platforms supports
>> memory encryption in slightly different way then all we need to do is to create
>> new object without needing to add a new parameter in -machine.
>>
>> [1] http://marc.info/?l=qemu-devel&m=147388592213137&w=2
>>
>> How about using 'memory-encryption-id' instead of security-policy ? If user wants
>> to launch SEV guest then memory-encryption-id should be set SEV specific object.
>> Something like this:
>>
>> -machine ..,memory-encryption-id=sev0 \
>> -object sev-guest,id=sev,debug=off,launch=launch0 \
>> -object sev-launch-info,id=launch0 \
>
> Something like that sounds good.  I think "-id" typically isn't included
> in the option name.
>
> So just the following is fine:
>
>  -machine memory-encryption=sev0 \
>  ...
>
> Other examples: -device virtio-blk-pci,drive=drive0 and -device
> e1000,netdev=netdev0.


Sounds good. I will take care of this in next rev. thanks for review

-Brijesh