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

Commit Message

Philippe Mathieu-Daudé Sept. 8, 2021, 11:20 p.m. UTC

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é <[e-mail]>
---
 include/hw/qdev-core.h |  6 ++++++
 hw/core/qdev.c         | 11 +++++++++++
 2 files changed, 17 insertions(+)

Comments

Daniel P. Berrangé Sept. 9, 2021, 11:03 a.m. UTC | #1

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é <[e-mail]>
> ---
>  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

Patch

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 */
     /*
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)));
+    }
+
     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);
 }