Re: [RFC PATCH 06/10] qdev: Use qemu_security_policy_taint() API

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Sep 09, 2021 at 01:20:20AM +0200, Philippe Mathieu-Daudé wrote:
> Add DeviceClass::taints_security_policy field to allow an
> unsafe device to eventually taint the global security policy
> in DeviceRealize().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/hw/qdev-core.h |  6 ++++++
>  hw/core/qdev.c         | 11 +++++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index bafc311bfa1..ff9ce6671be 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -122,6 +122,12 @@ struct DeviceClass {
>       */
>      bool user_creatable;
>      bool hotpluggable;
> +    /*
> +     * %false if the device is within the QEMU security policy boundary,
> +     * %true if there is no guarantee this device can be used safely.
> +     * See: https://www.qemu.org/contribute/security-process/
> +     */
> +    bool taints_security_policy;
>  
>      /* callbacks */
>      /*

Although your use case is for devices, it probably makes more sense
to push this up into the Object base class.

I think it will need to be a tri-state value too, not a simple
bool. It isn't feasible to mark all devices with this property,
so initially we'll have no information about whether most
devices are secure or insecure. This patch gives the implication
that all devices are secure, except for the few that have been
marked otherwise, which is not a good default IMHO.

We want to be able to make it clear when introspecting, that we
have no information on security available for most devices ie

 - unset  => no information on security (the current default)
 - true => considered secure against malicious guest
 - false => considered insecure against malicious guest

Then we can also extend 'ObjectTypeInfo' to have a

   '*secure': 'bool'

to make 'qom-list-types' be able to introspect this upfront.



> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index cefc5eaa0a9..a5a00f3564c 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -31,6 +31,7 @@
>  #include "qapi/qmp/qerror.h"
>  #include "qapi/visitor.h"
>  #include "qemu/error-report.h"
> +#include "qemu-common.h"
>  #include "qemu/option.h"
>  #include "hw/hotplug.h"
>  #include "hw/irq.h"
> @@ -257,6 +258,13 @@ bool qdev_hotplug_allowed(DeviceState *dev, Error **errp)
>      MachineClass *mc;
>      Object *m_obj = qdev_get_machine();
>  
> +    if (qemu_security_policy_is_strict()
> +            && DEVICE_GET_CLASS(dev)->taints_security_policy) {
> +        error_setg(errp, "Device '%s' can not be hotplugged when"
> +                         " 'strict' security policy is in place",
> +                   object_get_typename(OBJECT(dev)));

Do you need a 'return' here to stop execution after reportig
the error ?

> +    }
> +
>      if (object_dynamic_cast(m_obj, TYPE_MACHINE)) {
>          machine = MACHINE(m_obj);
>          mc = MACHINE_GET_CLASS(machine);
> @@ -385,6 +393,9 @@ bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp)
>      } else {
>          assert(!DEVICE_GET_CLASS(dev)->bus_type);
>      }
> +    qemu_security_policy_taint(DEVICE_GET_CLASS(dev)->taints_security_policy,
> +                               "device type %s",
> +                               object_get_typename(OBJECT(dev)));
>  
>      return object_property_set_bool(OBJECT(dev), "realized", true, errp);
>  }
> -- 
> 2.31.1
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|