[RFC PATCH 00/10] security: Introduce qemu_security_policy_taint() API

[RFC PATCH 00/10] security: Introduce qemu_security_policy_taint() API

[Philippe Mathieu-Daudé]

Hi,

This series is experimental! The goal is to better limit the
boundary of what code is considerated security critical, and
what is less critical (but still important!).

This approach was quickly discussed few months ago with Markus
then Daniel. Instead of classifying the code on a file path
basis (see [1]), we insert (runtime) hints into the code
(which survive code movement). Offending unsafe code can
taint the global security policy. By default this policy
is 'none': the current behavior. It can be changed on the
command line to 'warn' to display warnings, and to 'strict'
to prohibit QEMU running with a tainted policy.

As examples I started implementing unsafe code taint from
3 different pieces of code:
- accelerators (KVM and Xen in allow-list)
- block drivers (vvfat and parcial null-co in deny-list)
- qdev (hobbyist devices regularly hit by fuzzer)

I don't want the security researchers to not fuzz QEMU unsafe
areas, but I'd like to make it clearer what the community
priority is (currently 47 opened issues on [3]).

Regards,

Phil.

[1] https://lore.kernel.org/qemu-devel/20200714083631.888605-2-ppandit@redhat.com/
[2] https://www.qemu.org/contribute/security-process/
[3] https://gitlab.com/qemu-project/qemu/-/issues?label_name[]=Fuzzer

Philippe Mathieu-Daudé (10):
  sysemu: Introduce qemu_security_policy_taint() API
  accel: Use qemu_security_policy_taint(), mark KVM and Xen as safe
  block: Use qemu_security_policy_taint() API
  block/vvfat: Mark the driver as unsafe
  block/null: Mark 'read-zeroes=off' option as unsafe
  qdev: Use qemu_security_policy_taint() API
  hw/display: Mark ATI and Artist devices as unsafe
  hw/misc: Mark testdev devices as unsafe
  hw/net: Mark Tulip device as unsafe
  hw/sd: Mark sdhci-pci device as unsafe

 qapi/run-state.json        | 16 +++++++++
 include/block/block_int.h  |  6 +++-
 include/hw/qdev-core.h     |  6 ++++
 include/qemu-common.h      | 19 +++++++++++
 include/qemu/accel.h       |  5 +++
 accel/kvm/kvm-all.c        |  1 +
 accel/xen/xen-all.c        |  1 +
 block.c                    |  6 ++++
 block/null.c               |  8 +++++
 block/vvfat.c              |  6 ++++
 hw/core/qdev.c             | 11 ++++++
 hw/display/artist.c        |  1 +
 hw/display/ati.c           |  1 +
 hw/hyperv/hyperv_testdev.c |  1 +
 hw/misc/pc-testdev.c       |  1 +
 hw/misc/pci-testdev.c      |  1 +
 hw/net/tulip.c             |  1 +
 hw/sd/sdhci-pci.c          |  1 +
 softmmu/vl.c               | 70 ++++++++++++++++++++++++++++++++++++++
 qemu-options.hx            | 17 +++++++++
 20 files changed, 178 insertions(+), 1 deletion(-)

-- 
2.31.1

[RFC PATCH 00/10] security: Introduce qemu_security_policy_taint() API

[Philippe Mathieu-Daudé]

Hi,

This series is experimental! The goal is to better limit the
boundary of what code is considerated security critical, and
what is less critical (but still important!).

This approach was quickly discussed few months ago with Markus
then Daniel. Instead of classifying the code on a file path
basis (see [1]), we insert (runtime) hints into the code
(which survive code movement). Offending unsafe code can
taint the global security policy. By default this policy
is 'none': the current behavior. It can be changed on the
command line to 'warn' to display warnings, and to 'strict'
to prohibit QEMU running with a tainted policy.

As examples I started implementing unsafe code taint from
3 different pieces of code:
- accelerators (KVM and Xen in allow-list)
- block drivers (vvfat and parcial null-co in deny-list)
- qdev (hobbyist devices regularly hit by fuzzer)

I don't want the security researchers to not fuzz QEMU unsafe
areas, but I'd like to make it clearer what the community
priority is (currently 47 opened issues on [3]).

Regards,

Phil.

[1] https://lore.kernel.org/qemu-devel/20200714083631.888605-2-ppandit@redhat.com/
[2] https://www.qemu.org/contribute/security-process/
[3] https://gitlab.com/qemu-project/qemu/-/issues?label_name[]=Fuzzer

Philippe Mathieu-Daudé (10):
  sysemu: Introduce qemu_security_policy_taint() API
  accel: Use qemu_security_policy_taint(), mark KVM and Xen as safe
  block: Use qemu_security_policy_taint() API
  block/vvfat: Mark the driver as unsafe
  block/null: Mark 'read-zeroes=off' option as unsafe
  qdev: Use qemu_security_policy_taint() API
  hw/display: Mark ATI and Artist devices as unsafe
  hw/misc: Mark testdev devices as unsafe
  hw/net: Mark Tulip device as unsafe
  hw/sd: Mark sdhci-pci device as unsafe

 qapi/run-state.json        | 16 +++++++++
 include/block/block_int.h  |  6 +++-
 include/hw/qdev-core.h     |  6 ++++
 include/qemu-common.h      | 19 +++++++++++
 include/qemu/accel.h       |  5 +++
 accel/kvm/kvm-all.c        |  1 +
 accel/xen/xen-all.c        |  1 +
 block.c                    |  6 ++++
 block/null.c               |  8 +++++
 block/vvfat.c              |  6 ++++
 hw/core/qdev.c             | 11 ++++++
 hw/display/artist.c        |  1 +
 hw/display/ati.c           |  1 +
 hw/hyperv/hyperv_testdev.c |  1 +
 hw/misc/pc-testdev.c       |  1 +
 hw/misc/pci-testdev.c      |  1 +
 hw/net/tulip.c             |  1 +
 hw/sd/sdhci-pci.c          |  1 +
 softmmu/vl.c               | 70 ++++++++++++++++++++++++++++++++++++++
 qemu-options.hx            | 17 +++++++++
 20 files changed, 178 insertions(+), 1 deletion(-)

-- 
2.31.1

[RFC PATCH 01/10] sysemu: Introduce qemu_security_policy_taint() API

[Philippe Mathieu-Daudé]

Introduce qemu_security_policy_taint() which allows unsafe (read
"not very maintained") code to 'taint' QEMU security policy.

The "security policy" is the @SecurityPolicy QAPI enum, composed of:
- "none"   (no policy, current behavior)
- "warn"   (display a warning when the policy is tainted, keep going)
- "strict" (once tainted, exit QEMU before starting the VM)

The qemu_security_policy_is_strict() helper is also provided, which
will be proved useful once a VM is started (example we do not want
to kill a running VM if an unsafe device is hot-added).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 qapi/run-state.json   | 16 +++++++++++
 include/qemu-common.h | 19 ++++++++++++
 softmmu/vl.c          | 67 +++++++++++++++++++++++++++++++++++++++++++
 qemu-options.hx       | 17 +++++++++++
 4 files changed, 119 insertions(+)

diff --git a/qapi/run-state.json b/qapi/run-state.json
index 43d66d700fc..b15a107fa01 100644
--- a/qapi/run-state.json
+++ b/qapi/run-state.json
@@ -638,3 +638,19 @@
 { 'struct': 'MemoryFailureFlags',
   'data': { 'action-required': 'bool',
             'recursive': 'bool'} }
+
+##
+# @SecurityPolicy:
+#
+# An enumeration of the actions taken when the security policy is tainted.
+#
+# @none: do nothing.
+#
+# @warn: display a warning.
+#
+# @strict: prohibit QEMU to start a VM.
+#
+# Since: 6.2
+##
+{ 'enum': 'SecurityPolicy',
+  'data': [ 'none', 'warn', 'strict' ] }
diff --git a/include/qemu-common.h b/include/qemu-common.h
index 73bcf763ed8..bf0b054bb66 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -139,4 +139,23 @@ void page_size_init(void);
  * returned. */
 bool dump_in_progress(void);
 
+/**
+ * qemu_security_policy_taint:
+ * @tainting whether any security policy is tainted (compromised).
+ * @fmt: taint reason format string
+ * ...: list of arguments to interpolate into @fmt, like printf().
+ *
+ * Allow unsafe code path to taint the global security policy.
+ * See #SecurityPolicy.
+ */
+void qemu_security_policy_taint(bool tainting, const char *fmt, ...)
+        GCC_FMT_ATTR(2, 3);
+
+/**
+ * qemu_security_policy_is_strict:
+ *
+ * Return %true if the global security policy is 'strict', %false otherwise.
+ */
+bool qemu_security_policy_is_strict(void);
+
 #endif
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 55ab70eb97f..92c05ac97ee 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -489,6 +489,20 @@ static QemuOptsList qemu_action_opts = {
     },
 };
 
+static QemuOptsList qemu_security_policy_opts = {
+    .name = "security-policy",
+    .implied_opt_name = "policy",
+    .merge_lists = true,
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_security_policy_opts.head),
+    .desc = {
+        {
+            .name = "policy",
+            .type = QEMU_OPT_STRING,
+        },
+        { /* end of list */ }
+    },
+};
+
 const char *qemu_get_vm_name(void)
 {
     return qemu_name;
@@ -600,6 +614,52 @@ static int cleanup_add_fd(void *opaque, QemuOpts *opts, Error **errp)
 }
 #endif
 
+static SecurityPolicy security_policy = SECURITY_POLICY_NONE;
+
+bool qemu_security_policy_is_strict(void)
+{
+    return security_policy == SECURITY_POLICY_STRICT;
+}
+
+static int select_security_policy(const char *p)
+{
+    int policy;
+    char *qapi_value;
+
+    qapi_value = g_ascii_strdown(p, -1);
+    policy = qapi_enum_parse(&SecurityPolicy_lookup, qapi_value, -1, NULL);
+    g_free(qapi_value);
+    if (policy < 0) {
+        return -1;
+    }
+    security_policy = policy;
+
+    return 0;
+}
+
+void qemu_security_policy_taint(bool tainting, const char *fmt, ...)
+{
+    va_list ap;
+    g_autofree char *efmt = NULL;
+
+    if (security_policy == SECURITY_POLICY_NONE || !tainting) {
+        return;
+    }
+
+    va_start(ap, fmt);
+    if (security_policy == SECURITY_POLICY_STRICT) {
+        efmt = g_strdup_printf("%s taints QEMU security policy, exiting.", fmt);
+        error_vreport(efmt, ap);
+        exit(EXIT_FAILURE);
+    } else if (security_policy == SECURITY_POLICY_WARN) {
+        efmt = g_strdup_printf("%s taints QEMU security policy.", fmt);
+        warn_vreport(efmt, ap);
+    } else {
+        g_assert_not_reached();
+    }
+    va_end(ap);
+}
+
 /***********************************************************/
 /* QEMU Block devices */
 
@@ -2764,6 +2824,7 @@ void qemu_init(int argc, char **argv, char **envp)
     qemu_add_opts(&qemu_semihosting_config_opts);
     qemu_add_opts(&qemu_fw_cfg_opts);
     qemu_add_opts(&qemu_action_opts);
+    qemu_add_opts(&qemu_security_policy_opts);
     module_call_init(MODULE_INIT_OPTS);
 
     error_init(argv[0]);
@@ -3230,6 +3291,12 @@ void qemu_init(int argc, char **argv, char **envp)
                     exit(1);
                 }
                 break;
+            case QEMU_OPTION_security_policy:
+                if (select_security_policy(optarg) == -1) {
+                    error_report("unknown -security-policy parameter");
+                    exit(1);
+                }
+                break;
             case QEMU_OPTION_parallel:
                 add_device_config(DEV_PARALLEL, optarg);
                 default_parallel = 0;
diff --git a/qemu-options.hx b/qemu-options.hx
index 8f603cc7e65..d9939f7ae1d 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4298,6 +4298,23 @@ SRST
 
 ERST
 
+DEF("security-policy", HAS_ARG, QEMU_OPTION_security_policy, \
+    "-security-policy none|warn|strict\n" \
+    "                 action when security policy is tainted [default=none]\n",
+    QEMU_ARCH_ALL)
+SRST
+``-security-policy policy``
+    The policy controls what QEMU will do when an unsecure feature is
+    used, tainting the process security. The default is ``none`` (do
+    nothing). Other possible actions are: ``warn`` (display a warning
+    and keep going) or ``strict`` (exits QEMU before launching a VM).
+
+    Examples:
+
+    ``-security-policy warn``; \ ``-security-policy strict``
+
+ERST
+
 DEF("echr", HAS_ARG, QEMU_OPTION_echr, \
     "-echr chr       set terminal escape character instead of ctrl-a\n",
     QEMU_ARCH_ALL)
-- 
2.31.1

[RFC PATCH 01/10] sysemu: Introduce qemu_security_policy_taint() API

[Philippe Mathieu-Daudé]

Introduce qemu_security_policy_taint() which allows unsafe (read
"not very maintained") code to 'taint' QEMU security policy.

The "security policy" is the @SecurityPolicy QAPI enum, composed of:
- "none"   (no policy, current behavior)
- "warn"   (display a warning when the policy is tainted, keep going)
- "strict" (once tainted, exit QEMU before starting the VM)

The qemu_security_policy_is_strict() helper is also provided, which
will be proved useful once a VM is started (example we do not want
to kill a running VM if an unsafe device is hot-added).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 qapi/run-state.json   | 16 +++++++++++
 include/qemu-common.h | 19 ++++++++++++
 softmmu/vl.c          | 67 +++++++++++++++++++++++++++++++++++++++++++
 qemu-options.hx       | 17 +++++++++++
 4 files changed, 119 insertions(+)

diff --git a/qapi/run-state.json b/qapi/run-state.json
index 43d66d700fc..b15a107fa01 100644
--- a/qapi/run-state.json
+++ b/qapi/run-state.json
@@ -638,3 +638,19 @@
 { 'struct': 'MemoryFailureFlags',
   'data': { 'action-required': 'bool',
             'recursive': 'bool'} }
+
+##
+# @SecurityPolicy:
+#
+# An enumeration of the actions taken when the security policy is tainted.
+#
+# @none: do nothing.
+#
+# @warn: display a warning.
+#
+# @strict: prohibit QEMU to start a VM.
+#
+# Since: 6.2
+##
+{ 'enum': 'SecurityPolicy',
+  'data': [ 'none', 'warn', 'strict' ] }
diff --git a/include/qemu-common.h b/include/qemu-common.h
index 73bcf763ed8..bf0b054bb66 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -139,4 +139,23 @@ void page_size_init(void);
  * returned. */
 bool dump_in_progress(void);
 
+/**
+ * qemu_security_policy_taint:
+ * @tainting whether any security policy is tainted (compromised).
+ * @fmt: taint reason format string
+ * ...: list of arguments to interpolate into @fmt, like printf().
+ *
+ * Allow unsafe code path to taint the global security policy.
+ * See #SecurityPolicy.
+ */
+void qemu_security_policy_taint(bool tainting, const char *fmt, ...)
+        GCC_FMT_ATTR(2, 3);
+
+/**
+ * qemu_security_policy_is_strict:
+ *
+ * Return %true if the global security policy is 'strict', %false otherwise.
+ */
+bool qemu_security_policy_is_strict(void);
+
 #endif
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 55ab70eb97f..92c05ac97ee 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -489,6 +489,20 @@ static QemuOptsList qemu_action_opts = {
     },
 };
 
+static QemuOptsList qemu_security_policy_opts = {
+    .name = "security-policy",
+    .implied_opt_name = "policy",
+    .merge_lists = true,
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_security_policy_opts.head),
+    .desc = {
+        {
+            .name = "policy",
+            .type = QEMU_OPT_STRING,
+        },
+        { /* end of list */ }
+    },
+};
+
 const char *qemu_get_vm_name(void)
 {
     return qemu_name;
@@ -600,6 +614,52 @@ static int cleanup_add_fd(void *opaque, QemuOpts *opts, Error **errp)
 }
 #endif
 
+static SecurityPolicy security_policy = SECURITY_POLICY_NONE;
+
+bool qemu_security_policy_is_strict(void)
+{
+    return security_policy == SECURITY_POLICY_STRICT;
+}
+
+static int select_security_policy(const char *p)
+{
+    int policy;
+    char *qapi_value;
+
+    qapi_value = g_ascii_strdown(p, -1);
+    policy = qapi_enum_parse(&SecurityPolicy_lookup, qapi_value, -1, NULL);
+    g_free(qapi_value);
+    if (policy < 0) {
+        return -1;
+    }
+    security_policy = policy;
+
+    return 0;
+}
+
+void qemu_security_policy_taint(bool tainting, const char *fmt, ...)
+{
+    va_list ap;
+    g_autofree char *efmt = NULL;
+
+    if (security_policy == SECURITY_POLICY_NONE || !tainting) {
+        return;
+    }
+
+    va_start(ap, fmt);
+    if (security_policy == SECURITY_POLICY_STRICT) {
+        efmt = g_strdup_printf("%s taints QEMU security policy, exiting.", fmt);
+        error_vreport(efmt, ap);
+        exit(EXIT_FAILURE);
+    } else if (security_policy == SECURITY_POLICY_WARN) {
+        efmt = g_strdup_printf("%s taints QEMU security policy.", fmt);
+        warn_vreport(efmt, ap);
+    } else {
+        g_assert_not_reached();
+    }
+    va_end(ap);
+}
+
 /***********************************************************/
 /* QEMU Block devices */
 
@@ -2764,6 +2824,7 @@ void qemu_init(int argc, char **argv, char **envp)
     qemu_add_opts(&qemu_semihosting_config_opts);
     qemu_add_opts(&qemu_fw_cfg_opts);
     qemu_add_opts(&qemu_action_opts);
+    qemu_add_opts(&qemu_security_policy_opts);
     module_call_init(MODULE_INIT_OPTS);
 
     error_init(argv[0]);
@@ -3230,6 +3291,12 @@ void qemu_init(int argc, char **argv, char **envp)
                     exit(1);
                 }
                 break;
+            case QEMU_OPTION_security_policy:
+                if (select_security_policy(optarg) == -1) {
+                    error_report("unknown -security-policy parameter");
+                    exit(1);
+                }
+                break;
             case QEMU_OPTION_parallel:
                 add_device_config(DEV_PARALLEL, optarg);
                 default_parallel = 0;
diff --git a/qemu-options.hx b/qemu-options.hx
index 8f603cc7e65..d9939f7ae1d 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4298,6 +4298,23 @@ SRST
 
 ERST
 
+DEF("security-policy", HAS_ARG, QEMU_OPTION_security_policy, \
+    "-security-policy none|warn|strict\n" \
+    "                 action when security policy is tainted [default=none]\n",
+    QEMU_ARCH_ALL)
+SRST
+``-security-policy policy``
+    The policy controls what QEMU will do when an unsecure feature is
+    used, tainting the process security. The default is ``none`` (do
+    nothing). Other possible actions are: ``warn`` (display a warning
+    and keep going) or ``strict`` (exits QEMU before launching a VM).
+
+    Examples:
+
+    ``-security-policy warn``; \ ``-security-policy strict``
+
+ERST
+
 DEF("echr", HAS_ARG, QEMU_OPTION_echr, \
     "-echr chr       set terminal escape character instead of ctrl-a\n",
     QEMU_ARCH_ALL)
-- 
2.31.1

[RFC PATCH 02/10] accel: Use qemu_security_policy_taint(), mark KVM and Xen as safe

[Philippe Mathieu-Daudé]

Add the AccelClass::secure_policy_supported field to classify
safe (within security boundary) vs unsafe accelerators.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qemu/accel.h | 5 +++++
 accel/kvm/kvm-all.c  | 1 +
 accel/xen/xen-all.c  | 1 +
 softmmu/vl.c         | 3 +++
 4 files changed, 10 insertions(+)

diff --git a/include/qemu/accel.h b/include/qemu/accel.h
index 4f4c283f6fc..895e30be0de 100644
--- a/include/qemu/accel.h
+++ b/include/qemu/accel.h
@@ -44,6 +44,11 @@ typedef struct AccelClass {
                        hwaddr start_addr, hwaddr size);
 #endif
     bool *allowed;
+    /*
+     * Whether the accelerator is withing QEMU security policy boundary.
+     * See: https://www.qemu.org/contribute/security-process/
+     */
+    bool secure_policy_supported;
     /*
      * Array of global properties that would be applied when specific
      * accelerator is chosen. It works like MachineClass.compat_props
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 0125c17edb8..eb6b9e44df2 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -3623,6 +3623,7 @@ static void kvm_accel_class_init(ObjectClass *oc, void *data)
     ac->init_machine = kvm_init;
     ac->has_memory = kvm_accel_has_memory;
     ac->allowed = &kvm_allowed;
+    ac->secure_policy_supported = true;
 
     object_class_property_add(oc, "kernel-irqchip", "on|off|split",
         NULL, kvm_set_kernel_irqchip,
diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
index 69aa7d018b2..57867af5faf 100644
--- a/accel/xen/xen-all.c
+++ b/accel/xen/xen-all.c
@@ -198,6 +198,7 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
     ac->setup_post = xen_setup_post;
     ac->allowed = &xen_allowed;
     ac->compat_props = g_ptr_array_new();
+    ac->secure_policy_supported = true;
 
     compat_props_add(ac->compat_props, compat, G_N_ELEMENTS(compat));
 
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 92c05ac97ee..e4f94e159c3 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2388,6 +2388,9 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
         return 0;
     }
 
+    qemu_security_policy_taint(!ac->secure_policy_supported,
+                               "%s accelerator", acc);
+
     return 1;
 }
 
-- 
2.31.1

[RFC PATCH 02/10] accel: Use qemu_security_policy_taint(), mark KVM and Xen as safe

[Philippe Mathieu-Daudé]

Add the AccelClass::secure_policy_supported field to classify
safe (within security boundary) vs unsafe accelerators.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qemu/accel.h | 5 +++++
 accel/kvm/kvm-all.c  | 1 +
 accel/xen/xen-all.c  | 1 +
 softmmu/vl.c         | 3 +++
 4 files changed, 10 insertions(+)

diff --git a/include/qemu/accel.h b/include/qemu/accel.h
index 4f4c283f6fc..895e30be0de 100644
--- a/include/qemu/accel.h
+++ b/include/qemu/accel.h
@@ -44,6 +44,11 @@ typedef struct AccelClass {
                        hwaddr start_addr, hwaddr size);
 #endif
     bool *allowed;
+    /*
+     * Whether the accelerator is withing QEMU security policy boundary.
+     * See: https://www.qemu.org/contribute/security-process/
+     */
+    bool secure_policy_supported;
     /*
      * Array of global properties that would be applied when specific
      * accelerator is chosen. It works like MachineClass.compat_props
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 0125c17edb8..eb6b9e44df2 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -3623,6 +3623,7 @@ static void kvm_accel_class_init(ObjectClass *oc, void *data)
     ac->init_machine = kvm_init;
     ac->has_memory = kvm_accel_has_memory;
     ac->allowed = &kvm_allowed;
+    ac->secure_policy_supported = true;
 
     object_class_property_add(oc, "kernel-irqchip", "on|off|split",
         NULL, kvm_set_kernel_irqchip,
diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
index 69aa7d018b2..57867af5faf 100644
--- a/accel/xen/xen-all.c
+++ b/accel/xen/xen-all.c
@@ -198,6 +198,7 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
     ac->setup_post = xen_setup_post;
     ac->allowed = &xen_allowed;
     ac->compat_props = g_ptr_array_new();
+    ac->secure_policy_supported = true;
 
     compat_props_add(ac->compat_props, compat, G_N_ELEMENTS(compat));
 
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 92c05ac97ee..e4f94e159c3 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2388,6 +2388,9 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
         return 0;
     }
 
+    qemu_security_policy_taint(!ac->secure_policy_supported,
+                               "%s accelerator", acc);
+
     return 1;
 }
 
-- 
2.31.1

[RFC PATCH 03/10] block: Use qemu_security_policy_taint() API

[Philippe Mathieu-Daudé]

Add the BlockDriver::bdrv_taints_security_policy() handler.
Drivers implementing it might taint the global QEMU security
policy.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/block/block_int.h | 6 +++++-
 block.c                   | 6 ++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index f1a54db0f8c..0ec0a5c06e9 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -169,7 +169,11 @@ struct BlockDriver {
     int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
                           Error **errp);
     void (*bdrv_close)(BlockDriverState *bs);
-
+    /*
+     * Return %true if the driver is withing QEMU security policy boundary,
+     * %false otherwise. See: https://www.qemu.org/contribute/security-process/
+     */
+    bool (*bdrv_taints_security_policy)(BlockDriverState *bs);
 
     int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
                                        Error **errp);
diff --git a/block.c b/block.c
index b2b66263f9a..696ba486001 100644
--- a/block.c
+++ b/block.c
@@ -49,6 +49,7 @@
 #include "qemu/timer.h"
 #include "qemu/cutils.h"
 #include "qemu/id.h"
+#include "qemu-common.h"
 #include "block/coroutines.h"
 
 #ifdef CONFIG_BSD
@@ -1587,6 +1588,11 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
         }
     }
 
+    if (drv->bdrv_taints_security_policy) {
+        qemu_security_policy_taint(drv->bdrv_taints_security_policy(bs),
+                                   "Block protocol '%s'", drv->format_name);
+    }
+
     return 0;
 open_failed:
     bs->drv = NULL;
-- 
2.31.1

[RFC PATCH 03/10] block: Use qemu_security_policy_taint() API

[Philippe Mathieu-Daudé]

Add the BlockDriver::bdrv_taints_security_policy() handler.
Drivers implementing it might taint the global QEMU security
policy.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/block/block_int.h | 6 +++++-
 block.c                   | 6 ++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index f1a54db0f8c..0ec0a5c06e9 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -169,7 +169,11 @@ struct BlockDriver {
     int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
                           Error **errp);
     void (*bdrv_close)(BlockDriverState *bs);
-
+    /*
+     * Return %true if the driver is withing QEMU security policy boundary,
+     * %false otherwise. See: https://www.qemu.org/contribute/security-process/
+     */
+    bool (*bdrv_taints_security_policy)(BlockDriverState *bs);
 
     int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
                                        Error **errp);
diff --git a/block.c b/block.c
index b2b66263f9a..696ba486001 100644
--- a/block.c
+++ b/block.c
@@ -49,6 +49,7 @@
 #include "qemu/timer.h"
 #include "qemu/cutils.h"
 #include "qemu/id.h"
+#include "qemu-common.h"
 #include "block/coroutines.h"
 
 #ifdef CONFIG_BSD
@@ -1587,6 +1588,11 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
         }
     }
 
+    if (drv->bdrv_taints_security_policy) {
+        qemu_security_policy_taint(drv->bdrv_taints_security_policy(bs),
+                                   "Block protocol '%s'", drv->format_name);
+    }
+
     return 0;
 open_failed:
     bs->drv = NULL;
-- 
2.31.1

[RFC PATCH 04/10] block/vvfat: Mark the driver as unsafe

[Philippe Mathieu-Daudé]

While being listed as 'supported' in MAINTAINERS, this driver
does not have many reviewers and contains various /* TODO */
unattended since various years. Not safe enough for production
environment, so have it taint the global security policy.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/vvfat.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/block/vvfat.c b/block/vvfat.c
index 34bf1e3a86e..993e40727d6 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3199,6 +3199,11 @@ static void vvfat_close(BlockDriverState *bs)
     }
 }
 
+static bool vvfat_taints_security_policy(BlockDriverState *bs)
+{
+    return true;
+}
+
 static const char *const vvfat_strong_runtime_opts[] = {
     "dir",
     "fat-type",
@@ -3219,6 +3224,7 @@ static BlockDriver bdrv_vvfat = {
     .bdrv_refresh_limits    = vvfat_refresh_limits,
     .bdrv_close             = vvfat_close,
     .bdrv_child_perm        = vvfat_child_perm,
+    .bdrv_taints_security_policy = vvfat_taints_security_policy,
 
     .bdrv_co_preadv         = vvfat_co_preadv,
     .bdrv_co_pwritev        = vvfat_co_pwritev,
-- 
2.31.1

[RFC PATCH 04/10] block/vvfat: Mark the driver as unsafe

[Philippe Mathieu-Daudé]

While being listed as 'supported' in MAINTAINERS, this driver
does not have many reviewers and contains various /* TODO */
unattended since various years. Not safe enough for production
environment, so have it taint the global security policy.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/vvfat.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/block/vvfat.c b/block/vvfat.c
index 34bf1e3a86e..993e40727d6 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3199,6 +3199,11 @@ static void vvfat_close(BlockDriverState *bs)
     }
 }
 
+static bool vvfat_taints_security_policy(BlockDriverState *bs)
+{
+    return true;
+}
+
 static const char *const vvfat_strong_runtime_opts[] = {
     "dir",
     "fat-type",
@@ -3219,6 +3224,7 @@ static BlockDriver bdrv_vvfat = {
     .bdrv_refresh_limits    = vvfat_refresh_limits,
     .bdrv_close             = vvfat_close,
     .bdrv_child_perm        = vvfat_child_perm,
+    .bdrv_taints_security_policy = vvfat_taints_security_policy,
 
     .bdrv_co_preadv         = vvfat_co_preadv,
     .bdrv_co_pwritev        = vvfat_co_pwritev,
-- 
2.31.1

[RFC PATCH 05/10] block/null: Mark 'read-zeroes=off' option as unsafe

[Philippe Mathieu-Daudé]

See commit b317006a3f1 ("docs/secure-coding-practices: Describe how
to use 'null-co' block driver") for rationale.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/null.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/block/null.c b/block/null.c
index cc9b1d4ea72..11e428f3cc2 100644
--- a/block/null.c
+++ b/block/null.c
@@ -99,6 +99,13 @@ static int null_file_open(BlockDriverState *bs, QDict *options, int flags,
     return ret;
 }
 
+static bool null_taints_security_policy(BlockDriverState *bs)
+{
+    BDRVNullState *s = bs->opaque;
+
+    return !s->read_zeroes;
+}
+
 static int64_t null_getlength(BlockDriverState *bs)
 {
     BDRVNullState *s = bs->opaque;
@@ -283,6 +290,7 @@ static BlockDriver bdrv_null_co = {
     .bdrv_parse_filename    = null_co_parse_filename,
     .bdrv_getlength         = null_getlength,
     .bdrv_get_allocated_file_size = null_allocated_file_size,
+    .bdrv_taints_security_policy = null_taints_security_policy,
 
     .bdrv_co_preadv         = null_co_preadv,
     .bdrv_co_pwritev        = null_co_pwritev,
-- 
2.31.1

[RFC PATCH 05/10] block/null: Mark 'read-zeroes=off' option as unsafe

[Philippe Mathieu-Daudé]

See commit b317006a3f1 ("docs/secure-coding-practices: Describe how
to use 'null-co' block driver") for rationale.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/null.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/block/null.c b/block/null.c
index cc9b1d4ea72..11e428f3cc2 100644
--- a/block/null.c
+++ b/block/null.c
@@ -99,6 +99,13 @@ static int null_file_open(BlockDriverState *bs, QDict *options, int flags,
     return ret;
 }
 
+static bool null_taints_security_policy(BlockDriverState *bs)
+{
+    BDRVNullState *s = bs->opaque;
+
+    return !s->read_zeroes;
+}
+
 static int64_t null_getlength(BlockDriverState *bs)
 {
     BDRVNullState *s = bs->opaque;
@@ -283,6 +290,7 @@ static BlockDriver bdrv_null_co = {
     .bdrv_parse_filename    = null_co_parse_filename,
     .bdrv_getlength         = null_getlength,
     .bdrv_get_allocated_file_size = null_allocated_file_size,
+    .bdrv_taints_security_policy = null_taints_security_policy,
 
     .bdrv_co_preadv         = null_co_preadv,
     .bdrv_co_pwritev        = null_co_pwritev,
-- 
2.31.1

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

[Philippe Mathieu-Daudé]

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

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

[Philippe Mathieu-Daudé]

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

[RFC PATCH 07/10] hw/display: Mark ATI and Artist devices as unsafe

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/display/artist.c | 1 +
 hw/display/ati.c    | 1 +
 2 files changed, 2 insertions(+)

diff --git a/hw/display/artist.c b/hw/display/artist.c
index 21b7fd1b440..067a4b2cb59 100644
--- a/hw/display/artist.c
+++ b/hw/display/artist.c
@@ -1482,6 +1482,7 @@ static void artist_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &vmstate_artist;
     dc->reset = artist_reset;
     device_class_set_props(dc, artist_properties);
+    dc->taints_security_policy = true;
 }
 
 static const TypeInfo artist_info = {
diff --git a/hw/display/ati.c b/hw/display/ati.c
index 31f22754dce..2f27ab69a87 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -1024,6 +1024,7 @@ static void ati_vga_class_init(ObjectClass *klass, void *data)
     device_class_set_props(dc, ati_vga_properties);
     dc->hotpluggable = false;
     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
+    dc->taints_security_policy = true;
 
     k->class_id = PCI_CLASS_DISPLAY_VGA;
     k->vendor_id = PCI_VENDOR_ID_ATI;
-- 
2.31.1

[RFC PATCH 07/10] hw/display: Mark ATI and Artist devices as unsafe

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/display/artist.c | 1 +
 hw/display/ati.c    | 1 +
 2 files changed, 2 insertions(+)

diff --git a/hw/display/artist.c b/hw/display/artist.c
index 21b7fd1b440..067a4b2cb59 100644
--- a/hw/display/artist.c
+++ b/hw/display/artist.c
@@ -1482,6 +1482,7 @@ static void artist_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &vmstate_artist;
     dc->reset = artist_reset;
     device_class_set_props(dc, artist_properties);
+    dc->taints_security_policy = true;
 }
 
 static const TypeInfo artist_info = {
diff --git a/hw/display/ati.c b/hw/display/ati.c
index 31f22754dce..2f27ab69a87 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -1024,6 +1024,7 @@ static void ati_vga_class_init(ObjectClass *klass, void *data)
     device_class_set_props(dc, ati_vga_properties);
     dc->hotpluggable = false;
     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
+    dc->taints_security_policy = true;
 
     k->class_id = PCI_CLASS_DISPLAY_VGA;
     k->vendor_id = PCI_VENDOR_ID_ATI;
-- 
2.31.1

[RFC PATCH 08/10] hw/misc: Mark testdev devices as unsafe

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/hyperv/hyperv_testdev.c | 1 +
 hw/misc/pc-testdev.c       | 1 +
 hw/misc/pci-testdev.c      | 1 +
 3 files changed, 3 insertions(+)

diff --git a/hw/hyperv/hyperv_testdev.c b/hw/hyperv/hyperv_testdev.c
index 9a56ddf83fe..6a75c350389 100644
--- a/hw/hyperv/hyperv_testdev.c
+++ b/hw/hyperv/hyperv_testdev.c
@@ -310,6 +310,7 @@ static void hv_test_dev_class_init(ObjectClass *klass, void *data)
 
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     dc->realize = hv_test_dev_realizefn;
+    dc->taints_security_policy = true;
 }
 
 static const TypeInfo hv_test_dev_info = {
diff --git a/hw/misc/pc-testdev.c b/hw/misc/pc-testdev.c
index e3896518694..6294b80ec1b 100644
--- a/hw/misc/pc-testdev.c
+++ b/hw/misc/pc-testdev.c
@@ -199,6 +199,7 @@ static void testdev_class_init(ObjectClass *klass, void *data)
 
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     dc->realize = testdev_realizefn;
+    dc->taints_security_policy = true;
 }
 
 static const TypeInfo testdev_info = {
diff --git a/hw/misc/pci-testdev.c b/hw/misc/pci-testdev.c
index 03845c8de34..189eb9bf1bb 100644
--- a/hw/misc/pci-testdev.c
+++ b/hw/misc/pci-testdev.c
@@ -340,6 +340,7 @@ static void pci_testdev_class_init(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     dc->reset = qdev_pci_testdev_reset;
     device_class_set_props(dc, pci_testdev_properties);
+    dc->taints_security_policy = true;
 }
 
 static const TypeInfo pci_testdev_info = {
-- 
2.31.1

[RFC PATCH 08/10] hw/misc: Mark testdev devices as unsafe

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/hyperv/hyperv_testdev.c | 1 +
 hw/misc/pc-testdev.c       | 1 +
 hw/misc/pci-testdev.c      | 1 +
 3 files changed, 3 insertions(+)

diff --git a/hw/hyperv/hyperv_testdev.c b/hw/hyperv/hyperv_testdev.c
index 9a56ddf83fe..6a75c350389 100644
--- a/hw/hyperv/hyperv_testdev.c
+++ b/hw/hyperv/hyperv_testdev.c
@@ -310,6 +310,7 @@ static void hv_test_dev_class_init(ObjectClass *klass, void *data)
 
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     dc->realize = hv_test_dev_realizefn;
+    dc->taints_security_policy = true;
 }
 
 static const TypeInfo hv_test_dev_info = {
diff --git a/hw/misc/pc-testdev.c b/hw/misc/pc-testdev.c
index e3896518694..6294b80ec1b 100644
--- a/hw/misc/pc-testdev.c
+++ b/hw/misc/pc-testdev.c
@@ -199,6 +199,7 @@ static void testdev_class_init(ObjectClass *klass, void *data)
 
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     dc->realize = testdev_realizefn;
+    dc->taints_security_policy = true;
 }
 
 static const TypeInfo testdev_info = {
diff --git a/hw/misc/pci-testdev.c b/hw/misc/pci-testdev.c
index 03845c8de34..189eb9bf1bb 100644
--- a/hw/misc/pci-testdev.c
+++ b/hw/misc/pci-testdev.c
@@ -340,6 +340,7 @@ static void pci_testdev_class_init(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     dc->reset = qdev_pci_testdev_reset;
     device_class_set_props(dc, pci_testdev_properties);
+    dc->taints_security_policy = true;
 }
 
 static const TypeInfo pci_testdev_info = {
-- 
2.31.1

[RFC PATCH 09/10] hw/net: Mark Tulip device as unsafe

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/net/tulip.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/net/tulip.c b/hw/net/tulip.c
index ca69f7ea5e1..eaad3266212 100644
--- a/hw/net/tulip.c
+++ b/hw/net/tulip.c
@@ -1025,6 +1025,7 @@ static void tulip_class_init(ObjectClass *klass, void *data)
     device_class_set_props(dc, tulip_properties);
     dc->reset = tulip_qdev_reset;
     set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
+    dc->taints_security_policy = true;
 }
 
 static const TypeInfo tulip_info = {
-- 
2.31.1

[RFC PATCH 09/10] hw/net: Mark Tulip device as unsafe

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/net/tulip.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/net/tulip.c b/hw/net/tulip.c
index ca69f7ea5e1..eaad3266212 100644
--- a/hw/net/tulip.c
+++ b/hw/net/tulip.c
@@ -1025,6 +1025,7 @@ static void tulip_class_init(ObjectClass *klass, void *data)
     device_class_set_props(dc, tulip_properties);
     dc->reset = tulip_qdev_reset;
     set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
+    dc->taints_security_policy = true;
 }
 
 static const TypeInfo tulip_info = {
-- 
2.31.1

[RFC PATCH 10/10] hw/sd: Mark sdhci-pci device as unsafe

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/sd/sdhci-pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/sd/sdhci-pci.c b/hw/sd/sdhci-pci.c
index c737c8b930e..7a36f88fd87 100644
--- a/hw/sd/sdhci-pci.c
+++ b/hw/sd/sdhci-pci.c
@@ -64,6 +64,7 @@ static void sdhci_pci_class_init(ObjectClass *klass, void *data)
     k->device_id = PCI_DEVICE_ID_REDHAT_SDHCI;
     k->class_id = PCI_CLASS_SYSTEM_SDHCI;
     device_class_set_props(dc, sdhci_pci_properties);
+    dc->taints_security_policy = true;
 
     sdhci_common_class_init(klass, data);
 }
-- 
2.31.1

[RFC PATCH 10/10] hw/sd: Mark sdhci-pci device as unsafe

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/sd/sdhci-pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/sd/sdhci-pci.c b/hw/sd/sdhci-pci.c
index c737c8b930e..7a36f88fd87 100644
--- a/hw/sd/sdhci-pci.c
+++ b/hw/sd/sdhci-pci.c
@@ -64,6 +64,7 @@ static void sdhci_pci_class_init(ObjectClass *klass, void *data)
     k->device_id = PCI_DEVICE_ID_REDHAT_SDHCI;
     k->class_id = PCI_CLASS_SYSTEM_SDHCI;
     device_class_set_props(dc, sdhci_pci_properties);
+    dc->taints_security_policy = true;
 
     sdhci_common_class_init(klass, data);
 }
-- 
2.31.1

Re: [RFC PATCH 03/10] block: Use qemu_security_policy_taint() API

[Philippe Mathieu-Daudé]

On 9/9/21 1:20 AM, Philippe Mathieu-Daudé wrote:
> Add the BlockDriver::bdrv_taints_security_policy() handler.
> Drivers implementing it might taint the global QEMU security
> policy.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/block/block_int.h | 6 +++++-
>  block.c                   | 6 ++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index f1a54db0f8c..0ec0a5c06e9 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -169,7 +169,11 @@ struct BlockDriver {
>      int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
>                            Error **errp);
>      void (*bdrv_close)(BlockDriverState *bs);
> -
> +    /*
> +     * Return %true if the driver is withing QEMU security policy boundary,
> +     * %false otherwise. See: https://www.qemu.org/contribute/security-process/
> +     */
> +    bool (*bdrv_taints_security_policy)(BlockDriverState *bs);
>  
>      int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
>                                         Error **errp);
> diff --git a/block.c b/block.c
> index b2b66263f9a..696ba486001 100644
> --- a/block.c
> +++ b/block.c
> @@ -49,6 +49,7 @@
>  #include "qemu/timer.h"
>  #include "qemu/cutils.h"
>  #include "qemu/id.h"
> +#include "qemu-common.h"
>  #include "block/coroutines.h"
>  
>  #ifdef CONFIG_BSD
> @@ -1587,6 +1588,11 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
>          }
>      }
>  
> +    if (drv->bdrv_taints_security_policy) {
> +        qemu_security_policy_taint(drv->bdrv_taints_security_policy(bs),
> +                                   "Block protocol '%s'", drv->format_name);

Hmm I should check for phase_check(PHASE_MACHINE_READY)
and qemu_security_policy_is_strict() somehow, to refuse
adding unsafe drv at runtime instead of exiting...

> +    }
> +
>      return 0;
>  open_failed:
>      bs->drv = NULL;
> 

Re: [RFC PATCH 03/10] block: Use qemu_security_policy_taint() API

[Philippe Mathieu-Daudé]

On 9/9/21 1:20 AM, Philippe Mathieu-Daudé wrote:
> Add the BlockDriver::bdrv_taints_security_policy() handler.
> Drivers implementing it might taint the global QEMU security
> policy.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/block/block_int.h | 6 +++++-
>  block.c                   | 6 ++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index f1a54db0f8c..0ec0a5c06e9 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -169,7 +169,11 @@ struct BlockDriver {
>      int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
>                            Error **errp);
>      void (*bdrv_close)(BlockDriverState *bs);
> -
> +    /*
> +     * Return %true if the driver is withing QEMU security policy boundary,
> +     * %false otherwise. See: https://www.qemu.org/contribute/security-process/
> +     */
> +    bool (*bdrv_taints_security_policy)(BlockDriverState *bs);
>  
>      int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
>                                         Error **errp);
> diff --git a/block.c b/block.c
> index b2b66263f9a..696ba486001 100644
> --- a/block.c
> +++ b/block.c
> @@ -49,6 +49,7 @@
>  #include "qemu/timer.h"
>  #include "qemu/cutils.h"
>  #include "qemu/id.h"
> +#include "qemu-common.h"
>  #include "block/coroutines.h"
>  
>  #ifdef CONFIG_BSD
> @@ -1587,6 +1588,11 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
>          }
>      }
>  
> +    if (drv->bdrv_taints_security_policy) {
> +        qemu_security_policy_taint(drv->bdrv_taints_security_policy(bs),
> +                                   "Block protocol '%s'", drv->format_name);

Hmm I should check for phase_check(PHASE_MACHINE_READY)
and qemu_security_policy_is_strict() somehow, to refuse
adding unsafe drv at runtime instead of exiting...

> +    }
> +
>      return 0;
>  open_failed:
>      bs->drv = NULL;
> 

Re: [RFC PATCH 01/10] sysemu: Introduce qemu_security_policy_taint() API

[Paolo Bonzini]

On 09/09/21 01:20, Philippe Mathieu-Daudé wrote:
> +static QemuOptsList qemu_security_policy_opts = {
> +    .name = "security-policy",
> +    .implied_opt_name = "policy",
> +    .merge_lists = true,
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_security_policy_opts.head),
> +    .desc = {
> +        {
> +            .name = "policy",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        { /* end of list */ }
> +    },
> +};

No new command line options please.  You could rename -compat-policy to 
just -policy, and make this a "security" suboption.

Paolo

Re: [RFC PATCH 00/10] security: Introduce qemu_security_policy_taint() API

[Daniel P. Berrangé]

On Thu, Sep 09, 2021 at 01:20:14AM +0200, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> This series is experimental! The goal is to better limit the
> boundary of what code is considerated security critical, and
> what is less critical (but still important!).
> 
> This approach was quickly discussed few months ago with Markus
> then Daniel. Instead of classifying the code on a file path
> basis (see [1]), we insert (runtime) hints into the code
> (which survive code movement). Offending unsafe code can
> taint the global security policy. By default this policy
> is 'none': the current behavior. It can be changed on the
> command line to 'warn' to display warnings, and to 'strict'
> to prohibit QEMU running with a tainted policy.

Ok, so I infer that you based this idea on the "--compat-policy"
arg used to control behaviour wrt to deprecations.

With the deprecation support, the QAPI introspection data can
report deprecations for machines / CPUs (and in theory devices
later).  Libvirt records this deprecation info and can report
it to the user before even starting a guest, so they can avoid
using a deprecated device in the first place.  We also use this
info to mark a guest as tainted + deprecation at the libvirt
level and let mgmt apps query this status.

The --compat-policy support has been integrated into libvirt
but it is not something we expect real world deployments to
use - rather it is targeted as a testing framework.

Essentially I see the security reporting as needing to operate
in a pretty similar manner.

IOW, the reporting via QAPI introspetion is much more important
for libvirt and mgmt apps, than any custom cli arg / printfs
at the QEMU level.


In terms of QAPI design we currently have

   'deprecated': 'bool'

field against MachineInfo and CpuDefinitionInfo types.

it feels like we need

   'secure': 'bool'

field against the relevant types here too, though I think
maybe we might need to make it an optional field  to let
us distinguish lack of information, since it will take a
long time to annotate all areas. eg

   '*secure': 'bool'

 - not set  => no info available on security characteristics
 - true => device is considered secure wrt malicious guest
 - false => device is not considered secure wrt malicious guest


> As examples I started implementing unsafe code taint from
> 3 different pieces of code:
> - accelerators (KVM and Xen in allow-list)
> - block drivers (vvfat and parcial null-co in deny-list)
> - qdev (hobbyist devices regularly hit by fuzzer)
> 
> I don't want the security researchers to not fuzz QEMU unsafe
> areas, but I'd like to make it clearer what the community
> priority is (currently 47 opened issues on [3]).

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 :|

Re: [RFC PATCH 00/10] security: Introduce qemu_security_policy_taint() API

[Daniel P. Berrangé]

On Thu, Sep 09, 2021 at 01:20:14AM +0200, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> This series is experimental! The goal is to better limit the
> boundary of what code is considerated security critical, and
> what is less critical (but still important!).
> 
> This approach was quickly discussed few months ago with Markus
> then Daniel. Instead of classifying the code on a file path
> basis (see [1]), we insert (runtime) hints into the code
> (which survive code movement). Offending unsafe code can
> taint the global security policy. By default this policy
> is 'none': the current behavior. It can be changed on the
> command line to 'warn' to display warnings, and to 'strict'
> to prohibit QEMU running with a tainted policy.

Ok, so I infer that you based this idea on the "--compat-policy"
arg used to control behaviour wrt to deprecations.

With the deprecation support, the QAPI introspection data can
report deprecations for machines / CPUs (and in theory devices
later).  Libvirt records this deprecation info and can report
it to the user before even starting a guest, so they can avoid
using a deprecated device in the first place.  We also use this
info to mark a guest as tainted + deprecation at the libvirt
level and let mgmt apps query this status.

The --compat-policy support has been integrated into libvirt
but it is not something we expect real world deployments to
use - rather it is targeted as a testing framework.

Essentially I see the security reporting as needing to operate
in a pretty similar manner.

IOW, the reporting via QAPI introspetion is much more important
for libvirt and mgmt apps, than any custom cli arg / printfs
at the QEMU level.


In terms of QAPI design we currently have

   'deprecated': 'bool'

field against MachineInfo and CpuDefinitionInfo types.

it feels like we need

   'secure': 'bool'

field against the relevant types here too, though I think
maybe we might need to make it an optional field  to let
us distinguish lack of information, since it will take a
long time to annotate all areas. eg

   '*secure': 'bool'

 - not set  => no info available on security characteristics
 - true => device is considered secure wrt malicious guest
 - false => device is not considered secure wrt malicious guest


> As examples I started implementing unsafe code taint from
> 3 different pieces of code:
> - accelerators (KVM and Xen in allow-list)
> - block drivers (vvfat and parcial null-co in deny-list)
> - qdev (hobbyist devices regularly hit by fuzzer)
> 
> I don't want the security researchers to not fuzz QEMU unsafe
> areas, but I'd like to make it clearer what the community
> priority is (currently 47 opened issues on [3]).

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 :|

Re: [RFC PATCH 02/10] accel: Use qemu_security_policy_taint(), mark KVM and Xen as safe

[Daniel P. Berrangé]

On Thu, Sep 09, 2021 at 01:20:16AM +0200, Philippe Mathieu-Daudé wrote:
> Add the AccelClass::secure_policy_supported field to classify
> safe (within security boundary) vs unsafe accelerators.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/qemu/accel.h | 5 +++++
>  accel/kvm/kvm-all.c  | 1 +
>  accel/xen/xen-all.c  | 1 +
>  softmmu/vl.c         | 3 +++
>  4 files changed, 10 insertions(+)
> 
> diff --git a/include/qemu/accel.h b/include/qemu/accel.h
> index 4f4c283f6fc..895e30be0de 100644
> --- a/include/qemu/accel.h
> +++ b/include/qemu/accel.h
> @@ -44,6 +44,11 @@ typedef struct AccelClass {
>                         hwaddr start_addr, hwaddr size);
>  #endif
>      bool *allowed;
> +    /*
> +     * Whether the accelerator is withing QEMU security policy boundary.
> +     * See: https://www.qemu.org/contribute/security-process/
> +     */
> +    bool secure_policy_supported;

The security handling policy is a high level concept that is
open to variation over time and also by downstream distro
vendors.

At a code level we should be dealing in a more fundamental
concept. At an accelerator level we should really jsut
declare whether or not the accelerator impl is considered
to be secure against malicious guest code.

eg

    /* Whether this accelerator is secure against execution
     * of malciious guest machine code */
    bool secure;


>      /*
>       * Array of global properties that would be applied when specific
>       * accelerator is chosen. It works like MachineClass.compat_props
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 0125c17edb8..eb6b9e44df2 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -3623,6 +3623,7 @@ static void kvm_accel_class_init(ObjectClass *oc, void *data)
>      ac->init_machine = kvm_init;
>      ac->has_memory = kvm_accel_has_memory;
>      ac->allowed = &kvm_allowed;
> +    ac->secure_policy_supported = true;
>  
>      object_class_property_add(oc, "kernel-irqchip", "on|off|split",
>          NULL, kvm_set_kernel_irqchip,
> diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
> index 69aa7d018b2..57867af5faf 100644
> --- a/accel/xen/xen-all.c
> +++ b/accel/xen/xen-all.c
> @@ -198,6 +198,7 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
>      ac->setup_post = xen_setup_post;
>      ac->allowed = &xen_allowed;
>      ac->compat_props = g_ptr_array_new();
> +    ac->secure_policy_supported = true;
>  
>      compat_props_add(ac->compat_props, compat, G_N_ELEMENTS(compat));
>  
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 92c05ac97ee..e4f94e159c3 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2388,6 +2388,9 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>          return 0;
>      }
>  
> +    qemu_security_policy_taint(!ac->secure_policy_supported,
> +                               "%s accelerator", acc);

We need this information to be introspectable, becuase stuff printed
to stderr is essentially opaque to libvirt and mgmt apps above.

We don't have a convenient "query-accel" command but I think this
could possibly fit into 'query-target'. ie the TargetInfo struct
gain a field:
 

  ##
  # @TargetInfo:
  #
  # Information describing the QEMU target.
  #
  # @arch: the target architecture
  # @secure: Whether the currently active accelerator for this target
  #          is secure against execution of malicous guest code
  #
  # Since: 1.2
  ##
  { 'struct': 'TargetInfo',
    'data': { 'arch': 'SysEmuTarget',
              'secure': 'bool'} }

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 :|

Re: [RFC PATCH 02/10] accel: Use qemu_security_policy_taint(), mark KVM and Xen as safe

[Daniel P. Berrangé]

On Thu, Sep 09, 2021 at 01:20:16AM +0200, Philippe Mathieu-Daudé wrote:
> Add the AccelClass::secure_policy_supported field to classify
> safe (within security boundary) vs unsafe accelerators.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/qemu/accel.h | 5 +++++
>  accel/kvm/kvm-all.c  | 1 +
>  accel/xen/xen-all.c  | 1 +
>  softmmu/vl.c         | 3 +++
>  4 files changed, 10 insertions(+)
> 
> diff --git a/include/qemu/accel.h b/include/qemu/accel.h
> index 4f4c283f6fc..895e30be0de 100644
> --- a/include/qemu/accel.h
> +++ b/include/qemu/accel.h
> @@ -44,6 +44,11 @@ typedef struct AccelClass {
>                         hwaddr start_addr, hwaddr size);
>  #endif
>      bool *allowed;
> +    /*
> +     * Whether the accelerator is withing QEMU security policy boundary.
> +     * See: https://www.qemu.org/contribute/security-process/
> +     */
> +    bool secure_policy_supported;

The security handling policy is a high level concept that is
open to variation over time and also by downstream distro
vendors.

At a code level we should be dealing in a more fundamental
concept. At an accelerator level we should really jsut
declare whether or not the accelerator impl is considered
to be secure against malicious guest code.

eg

    /* Whether this accelerator is secure against execution
     * of malciious guest machine code */
    bool secure;


>      /*
>       * Array of global properties that would be applied when specific
>       * accelerator is chosen. It works like MachineClass.compat_props
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 0125c17edb8..eb6b9e44df2 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -3623,6 +3623,7 @@ static void kvm_accel_class_init(ObjectClass *oc, void *data)
>      ac->init_machine = kvm_init;
>      ac->has_memory = kvm_accel_has_memory;
>      ac->allowed = &kvm_allowed;
> +    ac->secure_policy_supported = true;
>  
>      object_class_property_add(oc, "kernel-irqchip", "on|off|split",
>          NULL, kvm_set_kernel_irqchip,
> diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
> index 69aa7d018b2..57867af5faf 100644
> --- a/accel/xen/xen-all.c
> +++ b/accel/xen/xen-all.c
> @@ -198,6 +198,7 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
>      ac->setup_post = xen_setup_post;
>      ac->allowed = &xen_allowed;
>      ac->compat_props = g_ptr_array_new();
> +    ac->secure_policy_supported = true;
>  
>      compat_props_add(ac->compat_props, compat, G_N_ELEMENTS(compat));
>  
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 92c05ac97ee..e4f94e159c3 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2388,6 +2388,9 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>          return 0;
>      }
>  
> +    qemu_security_policy_taint(!ac->secure_policy_supported,
> +                               "%s accelerator", acc);

We need this information to be introspectable, becuase stuff printed
to stderr is essentially opaque to libvirt and mgmt apps above.

We don't have a convenient "query-accel" command but I think this
could possibly fit into 'query-target'. ie the TargetInfo struct
gain a field:
 

  ##
  # @TargetInfo:
  #
  # Information describing the QEMU target.
  #
  # @arch: the target architecture
  # @secure: Whether the currently active accelerator for this target
  #          is secure against execution of malicous guest code
  #
  # Since: 1.2
  ##
  { 'struct': 'TargetInfo',
    'data': { 'arch': 'SysEmuTarget',
              'secure': 'bool'} }

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 :|

Re: [RFC PATCH 03/10] block: Use qemu_security_policy_taint() API

[Daniel P. Berrangé]

On Thu, Sep 09, 2021 at 01:20:17AM +0200, Philippe Mathieu-Daudé wrote:
> Add the BlockDriver::bdrv_taints_security_policy() handler.
> Drivers implementing it might taint the global QEMU security
> policy.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/block/block_int.h | 6 +++++-
>  block.c                   | 6 ++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index f1a54db0f8c..0ec0a5c06e9 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -169,7 +169,11 @@ struct BlockDriver {
>      int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
>                            Error **errp);
>      void (*bdrv_close)(BlockDriverState *bs);
> -
> +    /*
> +     * Return %true if the driver is withing QEMU security policy boundary,
> +     * %false otherwise. See: https://www.qemu.org/contribute/security-process/
> +     */
> +    bool (*bdrv_taints_security_policy)(BlockDriverState *bs);
>  
>      int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
>                                         Error **errp);
> diff --git a/block.c b/block.c
> index b2b66263f9a..696ba486001 100644
> --- a/block.c
> +++ b/block.c
> @@ -49,6 +49,7 @@
>  #include "qemu/timer.h"
>  #include "qemu/cutils.h"
>  #include "qemu/id.h"
> +#include "qemu-common.h"
>  #include "block/coroutines.h"
>  
>  #ifdef CONFIG_BSD
> @@ -1587,6 +1588,11 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
>          }
>      }
>  
> +    if (drv->bdrv_taints_security_policy) {
> +        qemu_security_policy_taint(drv->bdrv_taints_security_policy(bs),
> +                                   "Block protocol '%s'", drv->format_name);
> +    }
> +
>      return 0;
>  open_failed:
>      bs->drv = NULL;

Again we need a way to report this via QAPI, but we don't have a natural
place is hang this off for introspection before starting a guest.

The best we can do is report the information after a block backend has
been instantiated. eg  Modify "BlockInfo" struct to gain

   '*secure': 'bool'

Note I made this an optional field, since unless we mark every single
block driver impl straight away, we'll need to cope with the absence
of information.

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 :|

Re: [RFC PATCH 03/10] block: Use qemu_security_policy_taint() API

[Daniel P. Berrangé]

On Thu, Sep 09, 2021 at 01:20:17AM +0200, Philippe Mathieu-Daudé wrote:
> Add the BlockDriver::bdrv_taints_security_policy() handler.
> Drivers implementing it might taint the global QEMU security
> policy.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/block/block_int.h | 6 +++++-
>  block.c                   | 6 ++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index f1a54db0f8c..0ec0a5c06e9 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -169,7 +169,11 @@ struct BlockDriver {
>      int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
>                            Error **errp);
>      void (*bdrv_close)(BlockDriverState *bs);
> -
> +    /*
> +     * Return %true if the driver is withing QEMU security policy boundary,
> +     * %false otherwise. See: https://www.qemu.org/contribute/security-process/
> +     */
> +    bool (*bdrv_taints_security_policy)(BlockDriverState *bs);
>  
>      int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
>                                         Error **errp);
> diff --git a/block.c b/block.c
> index b2b66263f9a..696ba486001 100644
> --- a/block.c
> +++ b/block.c
> @@ -49,6 +49,7 @@
>  #include "qemu/timer.h"
>  #include "qemu/cutils.h"
>  #include "qemu/id.h"
> +#include "qemu-common.h"
>  #include "block/coroutines.h"
>  
>  #ifdef CONFIG_BSD
> @@ -1587,6 +1588,11 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
>          }
>      }
>  
> +    if (drv->bdrv_taints_security_policy) {
> +        qemu_security_policy_taint(drv->bdrv_taints_security_policy(bs),
> +                                   "Block protocol '%s'", drv->format_name);
> +    }
> +
>      return 0;
>  open_failed:
>      bs->drv = NULL;

Again we need a way to report this via QAPI, but we don't have a natural
place is hang this off for introspection before starting a guest.

The best we can do is report the information after a block backend has
been instantiated. eg  Modify "BlockInfo" struct to gain

   '*secure': 'bool'

Note I made this an optional field, since unless we mark every single
block driver impl straight away, we'll need to cope with the absence
of information.

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 :|

Re: [RFC PATCH 03/10] block: Use qemu_security_policy_taint() API

[Daniel P. Berrangé]

On Thu, Sep 09, 2021 at 11:40:07AM +0100, Daniel P. Berrangé wrote:
> On Thu, Sep 09, 2021 at 01:20:17AM +0200, Philippe Mathieu-Daudé wrote:
> > Add the BlockDriver::bdrv_taints_security_policy() handler.
> > Drivers implementing it might taint the global QEMU security
> > policy.
> > 
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> >  include/block/block_int.h | 6 +++++-
> >  block.c                   | 6 ++++++
> >  2 files changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/block/block_int.h b/include/block/block_int.h
> > index f1a54db0f8c..0ec0a5c06e9 100644
> > --- a/include/block/block_int.h
> > +++ b/include/block/block_int.h
> > @@ -169,7 +169,11 @@ struct BlockDriver {
> >      int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
> >                            Error **errp);
> >      void (*bdrv_close)(BlockDriverState *bs);
> > -
> > +    /*
> > +     * Return %true if the driver is withing QEMU security policy boundary,
> > +     * %false otherwise. See: https://www.qemu.org/contribute/security-process/
> > +     */
> > +    bool (*bdrv_taints_security_policy)(BlockDriverState *bs);

Also as with previous comments, I think we should not refer
to tainting or the security policy here, but instead simply
document whether we consider the bdrv to be secure or not.

Tainting is merely one action that is taken in accordance with
the security policy, as a result of the information presented.

> >      int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
> >                                         Error **errp);
> > diff --git a/block.c b/block.c
> > index b2b66263f9a..696ba486001 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -49,6 +49,7 @@
> >  #include "qemu/timer.h"
> >  #include "qemu/cutils.h"
> >  #include "qemu/id.h"
> > +#include "qemu-common.h"
> >  #include "block/coroutines.h"
> >  
> >  #ifdef CONFIG_BSD
> > @@ -1587,6 +1588,11 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
> >          }
> >      }
> >  
> > +    if (drv->bdrv_taints_security_policy) {
> > +        qemu_security_policy_taint(drv->bdrv_taints_security_policy(bs),
> > +                                   "Block protocol '%s'", drv->format_name);
> > +    }
> > +
> >      return 0;
> >  open_failed:
> >      bs->drv = NULL;
> 
> Again we need a way to report this via QAPI, but we don't have a natural
> place is hang this off for introspection before starting a guest.
> 
> The best we can do is report the information after a block backend has
> been instantiated. eg  Modify "BlockInfo" struct to gain
> 
>    '*secure': 'bool'
> 
> Note I made this an optional field, since unless we mark every single
> block driver impl straight away, we'll need to cope with the absence
> of information.

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 :|

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

[Daniel P. Berrangé]

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 :|

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

[Daniel P. Berrangé]

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 :|

Re: [RFC PATCH 00/10] security: Introduce qemu_security_policy_taint() API

[Alexander Bulekov]

On 210909 0120, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> This series is experimental! The goal is to better limit the
> boundary of what code is considerated security critical, and
> what is less critical (but still important!).
> 
> This approach was quickly discussed few months ago with Markus
> then Daniel. Instead of classifying the code on a file path
> basis (see [1]), we insert (runtime) hints into the code
> (which survive code movement). Offending unsafe code can
> taint the global security policy. By default this policy
> is 'none': the current behavior. It can be changed on the
> command line to 'warn' to display warnings, and to 'strict'
> to prohibit QEMU running with a tainted policy.
> 
> As examples I started implementing unsafe code taint from
> 3 different pieces of code:
> - accelerators (KVM and Xen in allow-list)
> - block drivers (vvfat and parcial null-co in deny-list)
> - qdev (hobbyist devices regularly hit by fuzzer)

Just looking through the list of hci, storage, network and graphics
devices available on i386 to see which others are potential good
candidates for this tag. Obviously a lot of guesswork here:

USB devices:
name "ich9-usb-ehci1", bus PCI
name "ich9-usb-ehci2", bus PCI
name "ich9-usb-uhci1", bus PCI
name "ich9-usb-uhci2", bus PCI
name "ich9-usb-uhci3", bus PCI
name "ich9-usb-uhci4", bus PCI
name "ich9-usb-uhci5", bus PCI
name "ich9-usb-uhci6", bus PCI
name "nec-usb-xhci", bus PCI
name "pci-ohci", bus PCI, desc "Apple USB Controller"
name "piix3-usb-uhci", bus PCI
name "piix4-usb-uhci", bus PCI
name "qemu-xhci", bus PCI
name "usb-ehci", bus PCI

Not sure about these. Maybe ohci isn't sensitive?

Storage devices:
=== Sensitive ===
name "floppy", bus floppy-bus, desc "virtual floppy drive"
name "ide-cd", bus IDE, desc "virtual IDE CD-ROM"
name "ide-hd", bus IDE, desc "virtual IDE disk"
name "isa-fdc", bus ISA, desc "virtual floppy controller"
name "isa-ide", bus ISA
name "piix3-ide", bus PCI
name "piix3-ide-xen", bus PCI
name "piix4-ide", bus PCI
name "scsi-block", bus SCSI, desc "SCSI block device passthrough"
name "scsi-cd", bus SCSI, desc "virtual SCSI CD-ROM"
name "scsi-generic", bus SCSI, desc "pass through generic scsi device (/dev/sg*)"
name "scsi-hd", bus SCSI, desc "virtual SCSI disk"
name "vhost-scsi", bus virtio-bus
name "vhost-scsi-pci", bus PCI
name "vhost-scsi-pci-non-transitional", bus PCI
name "vhost-scsi-pci-transitional", bus PCI
name "vhost-user-blk", bus virtio-bus
name "vhost-user-blk-pci", bus PCI
name "vhost-user-blk-pci-non-transitional", bus PCI
name "vhost-user-blk-pci-transitional", bus PCI
name "vhost-user-fs-device", bus virtio-bus
name "vhost-user-fs-pci", bus PCI
name "vhost-user-scsi", bus virtio-bus
name "vhost-user-scsi-pci", bus PCI
name "vhost-user-scsi-pci-non-transitional", bus PCI
name "vhost-user-scsi-pci-transitional", bus PCI
name "virtio-9p-device", bus virtio-bus
name "virtio-9p-pci", bus PCI, alias "virtio-9p"
name "virtio-9p-pci-non-transitional", bus PCI
name "virtio-9p-pci-transitional", bus PCI
name "virtio-blk-device", bus virtio-bus
name "virtio-blk-pci", bus PCI, alias "virtio-blk"
name "virtio-blk-pci-non-transitional", bus PCI
name "virtio-blk-pci-transitional", bus PCI
name "virtio-pmem", bus virtio-bus
name "virtio-scsi-device", bus virtio-bus
name "virtio-scsi-pci", bus PCI, alias "virtio-scsi"
name "virtio-scsi-pci-non-transitional", bus PCI
name "virtio-scsi-pci-transitional", bus PCI

=== Tainting/Not Sensitive ===
name "am53c974", bus PCI, desc "AMD Am53c974 PCscsi-PCI SCSI adapter"
name "dc390", bus PCI, desc "Tekram DC-390 SCSI adapter"
name "ich9-ahci", bus PCI, alias "ahci"
name "lsi53c810", bus PCI
name "lsi53c895a", bus PCI, alias "lsi"
name "megasas", bus PCI, desc "LSI MegaRAID SAS 1078"
name "megasas-gen2", bus PCI, desc "LSI MegaRAID SAS 2108"
name "mptsas1068", bus PCI, desc "LSI SAS 1068"
name "nvdimm", desc "DIMM memory module"
name "nvme", bus PCI, desc "Non-Volatile Memory Express"
name "nvme-ns", bus nvme-bus, desc "Virtual NVMe namespace"
name "nvme-subsys", desc "Virtual NVMe subsystem"
name "pvscsi", bus PCI
name "sd-card", bus sd-bus
name "sdhci-pci", bus PCI
name "usb-bot", bus usb-bus
name "usb-mtp", bus usb-bus, desc "USB Media Transfer Protocol device"
name "usb-storage", bus usb-bus
name "usb-uas", bus usb-bus

Network devices:
=== Sensitive ===
name "e1000", bus PCI, alias "e1000-82540em", desc "Intel Gigabit Ethernet"
name "e1000e", bus PCI, desc "Intel 82574L GbE Controller"
name "virtio-net-device", bus virtio-bus
name "virtio-net-pci", bus PCI, alias "virtio-net"
name "virtio-net-pci-non-transitional", bus PCI
name "virtio-net-pci-transitional", bus PCI

=== Tainting/Not Sensitive ===
name "e1000-82544gc", bus PCI, desc "Intel Gigabit Ethernet"
name "e1000-82545em", bus PCI, desc "Intel Gigabit Ethernet"
name "i82550", bus PCI, desc "Intel i82550 Ethernet"
name "i82551", bus PCI, desc "Intel i82551 Ethernet"
name "i82557a", bus PCI, desc "Intel i82557A Ethernet"
name "i82557b", bus PCI, desc "Intel i82557B Ethernet"
name "i82557c", bus PCI, desc "Intel i82557C Ethernet"
name "i82558a", bus PCI, desc "Intel i82558A Ethernet"
name "i82558b", bus PCI, desc "Intel i82558B Ethernet"
name "i82559a", bus PCI, desc "Intel i82559A Ethernet"
name "i82559b", bus PCI, desc "Intel i82559B Ethernet"
name "i82559c", bus PCI, desc "Intel i82559C Ethernet"
name "i82559er", bus PCI, desc "Intel i82559ER Ethernet"
name "i82562", bus PCI, desc "Intel i82562 Ethernet"
name "i82801", bus PCI, desc "Intel i82801 Ethernet"
name "ne2k_isa", bus ISA
name "ne2k_pci", bus PCI
name "pcnet", bus PCI
name "rocker", bus PCI, desc "Rocker Switch"
name "rtl8139", bus PCI
name "tulip", bus PCI
name "usb-net", bus usb-bus
name "vmxnet3", bus PCI, desc "VMWare Paravirtualized Ethernet v3"

Display devices:
=== Sensitive ===
name "isa-vga", bus ISA
name "qxl", bus PCI, desc "Spice QXL GPU (secondary)"
name "qxl-vga", bus PCI, desc "Spice QXL GPU (primary, vga compatible)"
name "vhost-user-gpu", bus virtio-bus
name "vhost-user-gpu-pci", bus PCI
name "vhost-user-vga", bus PCI
name "virtio-gpu-device", bus virtio-bus
name "virtio-gpu-pci", bus PCI, alias "virtio-gpu"
name "virtio-vga", bus PCI
name "VGA", bus PCI

=== Tainting/Not Sensitive ===
name "ati-vga", bus PCI
name "bochs-display", bus PCI
name "cirrus-vga", bus PCI, desc "Cirrus CLGD 54xx VGA"
name "isa-cirrus-vga", bus ISA
name "ramfb", bus System, desc "ram framebuffer standalone device"
name "secondary-vga", bus PCI
name "sga", bus ISA, desc "Serial Graphics Adapter"
name "vmware-svga", bus PCI

Sound devices:
=== Sensitive ===
name "hda-duplex", bus HDA, desc "HDA Audio Codec, duplex (line-out, line-in)"
name "hda-micro", bus HDA, desc "HDA Audio Codec, duplex (speaker, microphone)"
name "hda-output", bus HDA, desc "HDA Audio Codec, output-only (line-out)"
name "ich9-intel-hda", bus PCI, desc "Intel HD Audio Controller (ich9)"

=== Tainting/Not Sensitive ===
name "AC97", bus PCI, alias "ac97", desc "Intel 82801AA AC97 Audio"
name "adlib", bus ISA, desc "Yamaha YM3812 (OPL2)"
name "cs4231a", bus ISA, desc "Crystal Semiconductor CS4231A"
name "ES1370", bus PCI, alias "es1370", desc "ENSONIQ AudioPCI ES1370"
name "gus", bus ISA, desc "Gravis Ultrasound GF1"
name "intel-hda", bus PCI, desc "Intel HD Audio Controller (ich6)"
name "sb16", bus ISA, desc "Creative Sound Blaster 16"
name "usb-audio", bus usb-bus

Re: [RFC PATCH 00/10] security: Introduce qemu_security_policy_taint() API

[Alexander Bulekov]

On 210909 0120, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> This series is experimental! The goal is to better limit the
> boundary of what code is considerated security critical, and
> what is less critical (but still important!).
> 
> This approach was quickly discussed few months ago with Markus
> then Daniel. Instead of classifying the code on a file path
> basis (see [1]), we insert (runtime) hints into the code
> (which survive code movement). Offending unsafe code can
> taint the global security policy. By default this policy
> is 'none': the current behavior. It can be changed on the
> command line to 'warn' to display warnings, and to 'strict'
> to prohibit QEMU running with a tainted policy.
> 
> As examples I started implementing unsafe code taint from
> 3 different pieces of code:
> - accelerators (KVM and Xen in allow-list)
> - block drivers (vvfat and parcial null-co in deny-list)
> - qdev (hobbyist devices regularly hit by fuzzer)

Just looking through the list of hci, storage, network and graphics
devices available on i386 to see which others are potential good
candidates for this tag. Obviously a lot of guesswork here:

USB devices:
name "ich9-usb-ehci1", bus PCI
name "ich9-usb-ehci2", bus PCI
name "ich9-usb-uhci1", bus PCI
name "ich9-usb-uhci2", bus PCI
name "ich9-usb-uhci3", bus PCI
name "ich9-usb-uhci4", bus PCI
name "ich9-usb-uhci5", bus PCI
name "ich9-usb-uhci6", bus PCI
name "nec-usb-xhci", bus PCI
name "pci-ohci", bus PCI, desc "Apple USB Controller"
name "piix3-usb-uhci", bus PCI
name "piix4-usb-uhci", bus PCI
name "qemu-xhci", bus PCI
name "usb-ehci", bus PCI

Not sure about these. Maybe ohci isn't sensitive?

Storage devices:
=== Sensitive ===
name "floppy", bus floppy-bus, desc "virtual floppy drive"
name "ide-cd", bus IDE, desc "virtual IDE CD-ROM"
name "ide-hd", bus IDE, desc "virtual IDE disk"
name "isa-fdc", bus ISA, desc "virtual floppy controller"
name "isa-ide", bus ISA
name "piix3-ide", bus PCI
name "piix3-ide-xen", bus PCI
name "piix4-ide", bus PCI
name "scsi-block", bus SCSI, desc "SCSI block device passthrough"
name "scsi-cd", bus SCSI, desc "virtual SCSI CD-ROM"
name "scsi-generic", bus SCSI, desc "pass through generic scsi device (/dev/sg*)"
name "scsi-hd", bus SCSI, desc "virtual SCSI disk"
name "vhost-scsi", bus virtio-bus
name "vhost-scsi-pci", bus PCI
name "vhost-scsi-pci-non-transitional", bus PCI
name "vhost-scsi-pci-transitional", bus PCI
name "vhost-user-blk", bus virtio-bus
name "vhost-user-blk-pci", bus PCI
name "vhost-user-blk-pci-non-transitional", bus PCI
name "vhost-user-blk-pci-transitional", bus PCI
name "vhost-user-fs-device", bus virtio-bus
name "vhost-user-fs-pci", bus PCI
name "vhost-user-scsi", bus virtio-bus
name "vhost-user-scsi-pci", bus PCI
name "vhost-user-scsi-pci-non-transitional", bus PCI
name "vhost-user-scsi-pci-transitional", bus PCI
name "virtio-9p-device", bus virtio-bus
name "virtio-9p-pci", bus PCI, alias "virtio-9p"
name "virtio-9p-pci-non-transitional", bus PCI
name "virtio-9p-pci-transitional", bus PCI
name "virtio-blk-device", bus virtio-bus
name "virtio-blk-pci", bus PCI, alias "virtio-blk"
name "virtio-blk-pci-non-transitional", bus PCI
name "virtio-blk-pci-transitional", bus PCI
name "virtio-pmem", bus virtio-bus
name "virtio-scsi-device", bus virtio-bus
name "virtio-scsi-pci", bus PCI, alias "virtio-scsi"
name "virtio-scsi-pci-non-transitional", bus PCI
name "virtio-scsi-pci-transitional", bus PCI

=== Tainting/Not Sensitive ===
name "am53c974", bus PCI, desc "AMD Am53c974 PCscsi-PCI SCSI adapter"
name "dc390", bus PCI, desc "Tekram DC-390 SCSI adapter"
name "ich9-ahci", bus PCI, alias "ahci"
name "lsi53c810", bus PCI
name "lsi53c895a", bus PCI, alias "lsi"
name "megasas", bus PCI, desc "LSI MegaRAID SAS 1078"
name "megasas-gen2", bus PCI, desc "LSI MegaRAID SAS 2108"
name "mptsas1068", bus PCI, desc "LSI SAS 1068"
name "nvdimm", desc "DIMM memory module"
name "nvme", bus PCI, desc "Non-Volatile Memory Express"
name "nvme-ns", bus nvme-bus, desc "Virtual NVMe namespace"
name "nvme-subsys", desc "Virtual NVMe subsystem"
name "pvscsi", bus PCI
name "sd-card", bus sd-bus
name "sdhci-pci", bus PCI
name "usb-bot", bus usb-bus
name "usb-mtp", bus usb-bus, desc "USB Media Transfer Protocol device"
name "usb-storage", bus usb-bus
name "usb-uas", bus usb-bus

Network devices:
=== Sensitive ===
name "e1000", bus PCI, alias "e1000-82540em", desc "Intel Gigabit Ethernet"
name "e1000e", bus PCI, desc "Intel 82574L GbE Controller"
name "virtio-net-device", bus virtio-bus
name "virtio-net-pci", bus PCI, alias "virtio-net"
name "virtio-net-pci-non-transitional", bus PCI
name "virtio-net-pci-transitional", bus PCI

=== Tainting/Not Sensitive ===
name "e1000-82544gc", bus PCI, desc "Intel Gigabit Ethernet"
name "e1000-82545em", bus PCI, desc "Intel Gigabit Ethernet"
name "i82550", bus PCI, desc "Intel i82550 Ethernet"
name "i82551", bus PCI, desc "Intel i82551 Ethernet"
name "i82557a", bus PCI, desc "Intel i82557A Ethernet"
name "i82557b", bus PCI, desc "Intel i82557B Ethernet"
name "i82557c", bus PCI, desc "Intel i82557C Ethernet"
name "i82558a", bus PCI, desc "Intel i82558A Ethernet"
name "i82558b", bus PCI, desc "Intel i82558B Ethernet"
name "i82559a", bus PCI, desc "Intel i82559A Ethernet"
name "i82559b", bus PCI, desc "Intel i82559B Ethernet"
name "i82559c", bus PCI, desc "Intel i82559C Ethernet"
name "i82559er", bus PCI, desc "Intel i82559ER Ethernet"
name "i82562", bus PCI, desc "Intel i82562 Ethernet"
name "i82801", bus PCI, desc "Intel i82801 Ethernet"
name "ne2k_isa", bus ISA
name "ne2k_pci", bus PCI
name "pcnet", bus PCI
name "rocker", bus PCI, desc "Rocker Switch"
name "rtl8139", bus PCI
name "tulip", bus PCI
name "usb-net", bus usb-bus
name "vmxnet3", bus PCI, desc "VMWare Paravirtualized Ethernet v3"

Display devices:
=== Sensitive ===
name "isa-vga", bus ISA
name "qxl", bus PCI, desc "Spice QXL GPU (secondary)"
name "qxl-vga", bus PCI, desc "Spice QXL GPU (primary, vga compatible)"
name "vhost-user-gpu", bus virtio-bus
name "vhost-user-gpu-pci", bus PCI
name "vhost-user-vga", bus PCI
name "virtio-gpu-device", bus virtio-bus
name "virtio-gpu-pci", bus PCI, alias "virtio-gpu"
name "virtio-vga", bus PCI
name "VGA", bus PCI

=== Tainting/Not Sensitive ===
name "ati-vga", bus PCI
name "bochs-display", bus PCI
name "cirrus-vga", bus PCI, desc "Cirrus CLGD 54xx VGA"
name "isa-cirrus-vga", bus ISA
name "ramfb", bus System, desc "ram framebuffer standalone device"
name "secondary-vga", bus PCI
name "sga", bus ISA, desc "Serial Graphics Adapter"
name "vmware-svga", bus PCI

Sound devices:
=== Sensitive ===
name "hda-duplex", bus HDA, desc "HDA Audio Codec, duplex (line-out, line-in)"
name "hda-micro", bus HDA, desc "HDA Audio Codec, duplex (speaker, microphone)"
name "hda-output", bus HDA, desc "HDA Audio Codec, output-only (line-out)"
name "ich9-intel-hda", bus PCI, desc "Intel HD Audio Controller (ich9)"

=== Tainting/Not Sensitive ===
name "AC97", bus PCI, alias "ac97", desc "Intel 82801AA AC97 Audio"
name "adlib", bus ISA, desc "Yamaha YM3812 (OPL2)"
name "cs4231a", bus ISA, desc "Crystal Semiconductor CS4231A"
name "ES1370", bus PCI, alias "es1370", desc "ENSONIQ AudioPCI ES1370"
name "gus", bus ISA, desc "Gravis Ultrasound GF1"
name "intel-hda", bus PCI, desc "Intel HD Audio Controller (ich6)"
name "sb16", bus ISA, desc "Creative Sound Blaster 16"
name "usb-audio", bus usb-bus

Re: [RFC PATCH 01/10] sysemu: Introduce qemu_security_policy_taint() API

[Eric Blake]

On Thu, Sep 09, 2021 at 01:20:15AM +0200, Philippe Mathieu-Daudé wrote:
> Introduce qemu_security_policy_taint() which allows unsafe (read
> "not very maintained") code to 'taint' QEMU security policy.
> 
> The "security policy" is the @SecurityPolicy QAPI enum, composed of:
> - "none"   (no policy, current behavior)
> - "warn"   (display a warning when the policy is tainted, keep going)
> - "strict" (once tainted, exit QEMU before starting the VM)
> 
> The qemu_security_policy_is_strict() helper is also provided, which
> will be proved useful once a VM is started (example we do not want

s/be proved/prove/

> to kill a running VM if an unsafe device is hot-added).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  qapi/run-state.json   | 16 +++++++++++
>  include/qemu-common.h | 19 ++++++++++++
>  softmmu/vl.c          | 67 +++++++++++++++++++++++++++++++++++++++++++
>  qemu-options.hx       | 17 +++++++++++
>  4 files changed, 119 insertions(+)
> 
> diff --git a/qapi/run-state.json b/qapi/run-state.json
> index 43d66d700fc..b15a107fa01 100644
> --- a/qapi/run-state.json
> +++ b/qapi/run-state.json
> @@ -638,3 +638,19 @@
>  { 'struct': 'MemoryFailureFlags',
>    'data': { 'action-required': 'bool',
>              'recursive': 'bool'} }
> +
> +##
> +# @SecurityPolicy:
> +#
> +# An enumeration of the actions taken when the security policy is tainted.
> +#
> +# @none: do nothing.
> +#
> +# @warn: display a warning.
> +#
> +# @strict: prohibit QEMU to start a VM.

s/to start/from starting/

> +#
> +# Since: 6.2
> +##
> +{ 'enum': 'SecurityPolicy',
> +  'data': [ 'none', 'warn', 'strict' ] }

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [RFC PATCH 01/10] sysemu: Introduce qemu_security_policy_taint() API

[Eric Blake]

On Thu, Sep 09, 2021 at 01:20:15AM +0200, Philippe Mathieu-Daudé wrote:
> Introduce qemu_security_policy_taint() which allows unsafe (read
> "not very maintained") code to 'taint' QEMU security policy.
> 
> The "security policy" is the @SecurityPolicy QAPI enum, composed of:
> - "none"   (no policy, current behavior)
> - "warn"   (display a warning when the policy is tainted, keep going)
> - "strict" (once tainted, exit QEMU before starting the VM)
> 
> The qemu_security_policy_is_strict() helper is also provided, which
> will be proved useful once a VM is started (example we do not want

s/be proved/prove/

> to kill a running VM if an unsafe device is hot-added).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  qapi/run-state.json   | 16 +++++++++++
>  include/qemu-common.h | 19 ++++++++++++
>  softmmu/vl.c          | 67 +++++++++++++++++++++++++++++++++++++++++++
>  qemu-options.hx       | 17 +++++++++++
>  4 files changed, 119 insertions(+)
> 
> diff --git a/qapi/run-state.json b/qapi/run-state.json
> index 43d66d700fc..b15a107fa01 100644
> --- a/qapi/run-state.json
> +++ b/qapi/run-state.json
> @@ -638,3 +638,19 @@
>  { 'struct': 'MemoryFailureFlags',
>    'data': { 'action-required': 'bool',
>              'recursive': 'bool'} }
> +
> +##
> +# @SecurityPolicy:
> +#
> +# An enumeration of the actions taken when the security policy is tainted.
> +#
> +# @none: do nothing.
> +#
> +# @warn: display a warning.
> +#
> +# @strict: prohibit QEMU to start a VM.

s/to start/from starting/

> +#
> +# Since: 6.2
> +##
> +{ 'enum': 'SecurityPolicy',
> +  'data': [ 'none', 'warn', 'strict' ] }

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [RFC PATCH 02/10] accel: Use qemu_security_policy_taint(), mark KVM and Xen as safe

[Eric Blake]

On Thu, Sep 09, 2021 at 01:20:16AM +0200, Philippe Mathieu-Daudé wrote:
> Add the AccelClass::secure_policy_supported field to classify
> safe (within security boundary) vs unsafe accelerators.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/qemu/accel.h | 5 +++++
>  accel/kvm/kvm-all.c  | 1 +
>  accel/xen/xen-all.c  | 1 +
>  softmmu/vl.c         | 3 +++
>  4 files changed, 10 insertions(+)
> 
> diff --git a/include/qemu/accel.h b/include/qemu/accel.h
> index 4f4c283f6fc..895e30be0de 100644
> --- a/include/qemu/accel.h
> +++ b/include/qemu/accel.h
> @@ -44,6 +44,11 @@ typedef struct AccelClass {
>                         hwaddr start_addr, hwaddr size);
>  #endif
>      bool *allowed;
> +    /*
> +     * Whether the accelerator is withing QEMU security policy boundary.

within

> +     * See: https://www.qemu.org/contribute/security-process/
> +     */
> +    bool secure_policy_supported;
>      /*
>       * Array of global properties that would be applied when specific
>       * accelerator is chosen. It works like MachineClass.compat_props

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [RFC PATCH 02/10] accel: Use qemu_security_policy_taint(), mark KVM and Xen as safe

[Eric Blake]

On Thu, Sep 09, 2021 at 01:20:16AM +0200, Philippe Mathieu-Daudé wrote:
> Add the AccelClass::secure_policy_supported field to classify
> safe (within security boundary) vs unsafe accelerators.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/qemu/accel.h | 5 +++++
>  accel/kvm/kvm-all.c  | 1 +
>  accel/xen/xen-all.c  | 1 +
>  softmmu/vl.c         | 3 +++
>  4 files changed, 10 insertions(+)
> 
> diff --git a/include/qemu/accel.h b/include/qemu/accel.h
> index 4f4c283f6fc..895e30be0de 100644
> --- a/include/qemu/accel.h
> +++ b/include/qemu/accel.h
> @@ -44,6 +44,11 @@ typedef struct AccelClass {
>                         hwaddr start_addr, hwaddr size);
>  #endif
>      bool *allowed;
> +    /*
> +     * Whether the accelerator is withing QEMU security policy boundary.

within

> +     * See: https://www.qemu.org/contribute/security-process/
> +     */
> +    bool secure_policy_supported;
>      /*
>       * Array of global properties that would be applied when specific
>       * accelerator is chosen. It works like MachineClass.compat_props

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [RFC PATCH 03/10] block: Use qemu_security_policy_taint() API

[Eric Blake]

On Thu, Sep 09, 2021 at 01:20:17AM +0200, Philippe Mathieu-Daudé wrote:
> Add the BlockDriver::bdrv_taints_security_policy() handler.
> Drivers implementing it might taint the global QEMU security
> policy.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/block/block_int.h | 6 +++++-
>  block.c                   | 6 ++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index f1a54db0f8c..0ec0a5c06e9 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -169,7 +169,11 @@ struct BlockDriver {
>      int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
>                            Error **errp);
>      void (*bdrv_close)(BlockDriverState *bs);
> -
> +    /*
> +     * Return %true if the driver is withing QEMU security policy boundary,

within

> +     * %false otherwise. See: https://www.qemu.org/contribute/security-process/
> +     */
> +    bool (*bdrv_taints_security_policy)(BlockDriverState *bs);
>  
>      int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
>                                         Error **errp);

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [RFC PATCH 03/10] block: Use qemu_security_policy_taint() API

[Eric Blake]

On Thu, Sep 09, 2021 at 01:20:17AM +0200, Philippe Mathieu-Daudé wrote:
> Add the BlockDriver::bdrv_taints_security_policy() handler.
> Drivers implementing it might taint the global QEMU security
> policy.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/block/block_int.h | 6 +++++-
>  block.c                   | 6 ++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index f1a54db0f8c..0ec0a5c06e9 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -169,7 +169,11 @@ struct BlockDriver {
>      int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
>                            Error **errp);
>      void (*bdrv_close)(BlockDriverState *bs);
> -
> +    /*
> +     * Return %true if the driver is withing QEMU security policy boundary,

within

> +     * %false otherwise. See: https://www.qemu.org/contribute/security-process/
> +     */
> +    bool (*bdrv_taints_security_policy)(BlockDriverState *bs);
>  
>      int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
>                                         Error **errp);

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [RFC PATCH 00/10] security: Introduce qemu_security_policy_taint() API

[P J P]

Hello Philippe, all

>On Thursday, 9 September, 2021, 03:58:40 pm IST, Daniel P. Berrangé <berrange@redhat.com> wrote:
>On Thu, Sep 09, 2021 at 01:20:14AM +0200, Philippe Mathieu-Daudé wrote:
>> This series is experimental! The goal is to better limit the
>> boundary of what code is considerated security critical, and
>> what is less critical (but still important!).
>>
>> This approach was quickly discussed few months ago with Markus
>> then Daniel. Instead of classifying the code on a file path
>> basis (see [1]), we insert (runtime) hints into the code
>> (which survive code movement). Offending unsafe code can
>> taint the global security policy. By default this policy
>> is 'none': the current behavior. It can be changed on the
>> command line to 'warn' to display warnings, and to 'strict'
>> to prohibit QEMU running with a tainted policy.
>

* Thanks so much for restarting this thread. I've been at it intermittently last few
  months, thinking about how could we annotate the source/module objects.

   -> [*] https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg04642.html

* Last time we discussed about having both compile and run time options for users
  to be able to select the qualified objects/backends/devices as desired.

* To confirm: How/Where is the security policy defined? Is it device/module specific OR QEMU project wide?

> IOW, the reporting via QAPI introspetion is much more important
> for libvirt and mgmt apps, than any custom cli arg / printfs
> at the QEMU level.
>

* True, while it makes sense to have a solution that is conversant with
  the management/libvirt layers, It'll be great if we could address qemu/cli
  other use cases too.

>it feels like we need
>  'secure': 'bool'

* Though we started the (above [*]) discussion with '--security' option in mind,
  I wonder if 'secure' annotation is much specific. And if we could widen its scope.
--- x ---


Source annotations: I've been thinking over following approaches
===================

1) Segregate the QEMU sources under

      ../staging/      <= devel/experimental, not for production usage
      ../src/          <= good for production usage, hence security relevant
      ../deprecated/   <= Bad for production usage, not security relevant 

   - This is similar to Linux staging drivers' tree.
   - Staging drivers are not considered for production usage and hence CVE allocation.
   - At build time by default we only build sources under ../src/ tree.
   - But we can still have options to build /staging/ and /deprecated/ trees.  
   - It's readily understandable to end users.

2) pkgconfig(1) way:
   - If we could define per device/backend a configuration (.pc) file which is then used
     at build/run time to decide which sources are suitable for the build or usage.

   - I'm trying to experiment with this.

3) We annotate QEMU devices/backends/modules with macros which define its status.
   It can then be used at build/run times to decide if it's suitable for usage.
   For ex:

    $ cat annotsrc.h

    #include <inttypes.h>

    enum SRCSTATUS {
        DEVEL = 0,
        STAGING,
        PRODUCTION,
        DEPRECATED
    };

    uint8_t get_src_status(void);
    ===

    $ cat libx.c

    #include <annotsrc.h>

    #define SRC_STATUS DEPRECATED

    uint8_t
    get_src_status(void)
    {
        return SRC_STATUS;
    }
    ===

    $ cat testlibx.c

    #include <stdio.h>
    #include <annotsrc.h>

    int
    main (int argc, char *argv[])
    {
        printf("LibX status: %d\n", get_src_status());
        return 0;
    }
--- x ---



* Approach 3) above is similar to the _security_policy_taint() API,
  but works at the source/object file level, rather than specific 'struct type' field.
  

* Does adding a field to struct type (ex. DeviceClass) scale to all objects/modules/backends etc?
  Does it have any limitations to include/cover other sources/objects?


* I'd really appreciate any feedback/inputs/suggestions you may have.


Thank you.
---
  -P J P
http://feedmug.com

Re: [RFC PATCH 00/10] security: Introduce qemu_security_policy_taint() API

[P J P]

Hello Philippe, all

>On Thursday, 9 September, 2021, 03:58:40 pm IST, Daniel P. Berrangé <berrange@redhat.com> wrote:
>On Thu, Sep 09, 2021 at 01:20:14AM +0200, Philippe Mathieu-Daudé wrote:
>> This series is experimental! The goal is to better limit the
>> boundary of what code is considerated security critical, and
>> what is less critical (but still important!).
>>
>> This approach was quickly discussed few months ago with Markus
>> then Daniel. Instead of classifying the code on a file path
>> basis (see [1]), we insert (runtime) hints into the code
>> (which survive code movement). Offending unsafe code can
>> taint the global security policy. By default this policy
>> is 'none': the current behavior. It can be changed on the
>> command line to 'warn' to display warnings, and to 'strict'
>> to prohibit QEMU running with a tainted policy.
>

* Thanks so much for restarting this thread. I've been at it intermittently last few
  months, thinking about how could we annotate the source/module objects.

   -> [*] https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg04642.html

* Last time we discussed about having both compile and run time options for users
  to be able to select the qualified objects/backends/devices as desired.

* To confirm: How/Where is the security policy defined? Is it device/module specific OR QEMU project wide?

> IOW, the reporting via QAPI introspetion is much more important
> for libvirt and mgmt apps, than any custom cli arg / printfs
> at the QEMU level.
>

* True, while it makes sense to have a solution that is conversant with
  the management/libvirt layers, It'll be great if we could address qemu/cli
  other use cases too.

>it feels like we need
>  'secure': 'bool'

* Though we started the (above [*]) discussion with '--security' option in mind,
  I wonder if 'secure' annotation is much specific. And if we could widen its scope.
--- x ---


Source annotations: I've been thinking over following approaches
===================

1) Segregate the QEMU sources under

      ../staging/      <= devel/experimental, not for production usage
      ../src/          <= good for production usage, hence security relevant
      ../deprecated/   <= Bad for production usage, not security relevant 

   - This is similar to Linux staging drivers' tree.
   - Staging drivers are not considered for production usage and hence CVE allocation.
   - At build time by default we only build sources under ../src/ tree.
   - But we can still have options to build /staging/ and /deprecated/ trees.  
   - It's readily understandable to end users.

2) pkgconfig(1) way:
   - If we could define per device/backend a configuration (.pc) file which is then used
     at build/run time to decide which sources are suitable for the build or usage.

   - I'm trying to experiment with this.

3) We annotate QEMU devices/backends/modules with macros which define its status.
   It can then be used at build/run times to decide if it's suitable for usage.
   For ex:

    $ cat annotsrc.h

    #include <inttypes.h>

    enum SRCSTATUS {
        DEVEL = 0,
        STAGING,
        PRODUCTION,
        DEPRECATED
    };

    uint8_t get_src_status(void);
    ===

    $ cat libx.c

    #include <annotsrc.h>

    #define SRC_STATUS DEPRECATED

    uint8_t
    get_src_status(void)
    {
        return SRC_STATUS;
    }
    ===

    $ cat testlibx.c

    #include <stdio.h>
    #include <annotsrc.h>

    int
    main (int argc, char *argv[])
    {
        printf("LibX status: %d\n", get_src_status());
        return 0;
    }
--- x ---



* Approach 3) above is similar to the _security_policy_taint() API,
  but works at the source/object file level, rather than specific 'struct type' field.
  

* Does adding a field to struct type (ex. DeviceClass) scale to all objects/modules/backends etc?
  Does it have any limitations to include/cover other sources/objects?


* I'd really appreciate any feedback/inputs/suggestions you may have.


Thank you.
---
  -P J P
http://feedmug.com

Re: [RFC PATCH 00/10] security: Introduce qemu_security_policy_taint() API

[P J P]

On Tuesday, 14 September, 2021, 07:00:27 pm IST, P J P <pjp@fedoraproject.org> wrote:
>* Thanks so much for restarting this thread. I've been at it intermittently last few
> months, thinking about how could we annotate the source/module objects.
>
> -> [*] https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg04642.html
>
>* Last time we discussed about having both compile and run time options for users
> to be able to select the qualified objects/backends/devices as desired.
>
>* To confirm: How/Where is the security policy defined? Is it device/module specific OR QEMU project wide?
>
>>> it feels like we need
>> 'secure': 'bool'
>
>* Though we started the (above [*]) discussion with '--security' option in mind,
>  I wonder if 'secure' annotation is much specific. And if we could widen its scope.
>
>
>Source annotations: I've been thinking over following approaches
>===================
>
>1) Segregate the QEMU sources under
>
>  ../staging/ <= devel/experimental, not for production usage
>  ../src/ <= good for production usage, hence security relevant
>  ../deprecated/ <= Bad for production usage, not security relevant
>
>  - This is similar to Linux staging drivers' tree.
>  - Staging drivers are not considered for production usage and hence CVE allocation.
>  - At build time by default we only build sources under ../src/ tree.
>  - But we can still have options to build /staging/ and /deprecated/ trees.
>  - It's readily understandable to end users.
>
>2) pkgconfig(1) way:
>  - If we could define per device/backend a configuration (.pc) file which is then used
>  at build/run time to decide which sources are suitable for the build or usage.
>
>  - I'm trying to experiment with this.
>
>3) We annotate QEMU devices/backends/modules with macros which define its status.
>  It can then be used at build/run times to decide if it's suitable for usage.
>  For ex:
>
>  $ cat annotsrc.h
>
>  #include <inttypes.h>
>
>  enum SRCSTATUS {
>  DEVEL = 0,
>  STAGING,
>  PRODUCTION,
>  DEPRECATED
>  };
>
...
>
>
>* Approach 3) above is similar to the _security_policy_taint() API,
>  but works at the source/object file level, rather than specific 'struct type' field.
> 
>* Does adding a field to struct type (ex. DeviceClass) scale to all objects/modules/backends etc?
>  Does it have any limitations to include/cover other sources/objects?
>
>* I'd really appreciate your feedback/inputs/suggestions.


Ping...!?
---
  -P J P
http://feedmug.com

Re: [RFC PATCH 00/10] security: Introduce qemu_security_policy_taint() API

[P J P]

On Tuesday, 14 September, 2021, 07:00:27 pm IST, P J P <pjp@fedoraproject.org> wrote:
>* Thanks so much for restarting this thread. I've been at it intermittently last few
> months, thinking about how could we annotate the source/module objects.
>
> -> [*] https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg04642.html
>
>* Last time we discussed about having both compile and run time options for users
> to be able to select the qualified objects/backends/devices as desired.
>
>* To confirm: How/Where is the security policy defined? Is it device/module specific OR QEMU project wide?
>
>>> it feels like we need
>> 'secure': 'bool'
>
>* Though we started the (above [*]) discussion with '--security' option in mind,
>  I wonder if 'secure' annotation is much specific. And if we could widen its scope.
>
>
>Source annotations: I've been thinking over following approaches
>===================
>
>1) Segregate the QEMU sources under
>
>  ../staging/ <= devel/experimental, not for production usage
>  ../src/ <= good for production usage, hence security relevant
>  ../deprecated/ <= Bad for production usage, not security relevant
>
>  - This is similar to Linux staging drivers' tree.
>  - Staging drivers are not considered for production usage and hence CVE allocation.
>  - At build time by default we only build sources under ../src/ tree.
>  - But we can still have options to build /staging/ and /deprecated/ trees.
>  - It's readily understandable to end users.
>
>2) pkgconfig(1) way:
>  - If we could define per device/backend a configuration (.pc) file which is then used
>  at build/run time to decide which sources are suitable for the build or usage.
>
>  - I'm trying to experiment with this.
>
>3) We annotate QEMU devices/backends/modules with macros which define its status.
>  It can then be used at build/run times to decide if it's suitable for usage.
>  For ex:
>
>  $ cat annotsrc.h
>
>  #include <inttypes.h>
>
>  enum SRCSTATUS {
>  DEVEL = 0,
>  STAGING,
>  PRODUCTION,
>  DEPRECATED
>  };
>
...
>
>
>* Approach 3) above is similar to the _security_policy_taint() API,
>  but works at the source/object file level, rather than specific 'struct type' field.
> 
>* Does adding a field to struct type (ex. DeviceClass) scale to all objects/modules/backends etc?
>  Does it have any limitations to include/cover other sources/objects?
>
>* I'd really appreciate your feedback/inputs/suggestions.


Ping...!?
---
  -P J P
http://feedmug.com

Re: [RFC PATCH 00/10] security: Introduce qemu_security_policy_taint() API

[Daniel P. Berrangé]

On Tue, Sep 14, 2021 at 01:30:27PM +0000, P J P wrote:
> Hello Philippe, all
> 
> >On Thursday, 9 September, 2021, 03:58:40 pm IST, Daniel P. Berrangé <berrange@redhat.com> wrote:
> >On Thu, Sep 09, 2021 at 01:20:14AM +0200, Philippe Mathieu-Daudé wrote:
> >> This series is experimental! The goal is to better limit the
> >> boundary of what code is considerated security critical, and
> >> what is less critical (but still important!).
> >>
> >> This approach was quickly discussed few months ago with Markus
> >> then Daniel. Instead of classifying the code on a file path
> >> basis (see [1]), we insert (runtime) hints into the code
> >> (which survive code movement). Offending unsafe code can
> >> taint the global security policy. By default this policy
> >> is 'none': the current behavior. It can be changed on the
> >> command line to 'warn' to display warnings, and to 'strict'
> >> to prohibit QEMU running with a tainted policy.
> >
> 
> * Thanks so much for restarting this thread. I've been at it intermittently last few
>   months, thinking about how could we annotate the source/module objects.
> 
>    -> [*] https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg04642.html
> 
> * Last time we discussed about having both compile and run time options for users
>   to be able to select the qualified objects/backends/devices as desired.

Right, we have multiple different use cases here

 - People building QEMU who want to cut down what they ship to
   minimize the stuff they support, which is outside the security
   guarantee. This can be OS distros, but also any other consumer
   of QEMU
   
   eg. RHEL wants to cut out almost all non-virtualization stuff.
   There is a quirk here though, that RHEL still includes TCG
   which is considered outside the security guarantee. So a
   simple build time "--secure on|off" doesn't do the job on
   its own.

   We need something to let people understand the consequences
   of the build options they are enabling.

   NB, when I talk of build options, I include both configure/meson
   args, and also the CONFIG_* options set in configs/**/*.mak


 - Application developers want to check that they're not using
   stuff outside the security guarantee, even if the distro
   has enable it.  These need to be able to query whether the
   VM they've launched has undesirable configuration choices.


Some people fall into both groups, some people fall into neither
group.


> * To confirm: How/Where is the security policy defined? Is it
>   device/module specific OR QEMU project wide?

Currently our only definition is in the docs

  https://qemu-project.gitlab.io/qemu/system/security.html#security-requirements

Philippe's patch is proposing tagging against internal QEMU objects
of various types.  I further proposed that we expose this in QMP
so it is introspectable.

I think there's scope for doing stuff at build time with configure
args and *mak CONFIG_* options, but haven't thought what that might
look like.

> > IOW, the reporting via QAPI introspetion is much more important
> > for libvirt and mgmt apps, than any custom cli arg / printfs
> > at the QEMU level.
> >
> 
> * True, while it makes sense to have a solution that is conversant with
>   the management/libvirt layers, It'll be great if we could address qemu/cli
>   other use cases too.
> 
> >it feels like we need
> >  'secure': 'bool'
> 
> * Though we started the (above [*]) discussion with '--security' option in mind,
>   I wonder if 'secure' annotation is much specific. And if we could widen its scope.
> --- x ---
> 
> 
> Source annotations: I've been thinking over following approaches
> ===================
> 
> 1) Segregate the QEMU sources under
> 
>       ../staging/      <= devel/experimental, not for production usage
>       ../src/          <= good for production usage, hence security relevant
>       ../deprecated/   <= Bad for production usage, not security relevant 
> 
>    - This is similar to Linux staging drivers' tree.
>    - Staging drivers are not considered for production usage and hence CVE allocation.
>    - At build time by default we only build sources under ../src/ tree.
>    - But we can still have options to build /staging/ and /deprecated/ trees.  
>    - It's readily understandable to end users.

I don't think we should be working in terms of source files at all.
Some files contain multiple distinct bits of functionality that are
not easily separated, and will have distinct security levels. Also
IMHO it is unpleasant to be moving files around in git to when code
switches between levels.  Also there are distinct criteria here,
both security levels, and support levels - there can be stuff which
is fully supported but considered insecure, and stuff that is
deprecated but considered secure. 

> 2) pkgconfig(1) way:
>    - If we could define per device/backend a configuration (.pc) file which is then used
>      at build/run time to decide which sources are suitable for the build or usage.
> 
>    - I'm trying to experiment with this.

For build time configuration, we have a pretty clear set of
toggles between the configure/meson options, and the CONFIG_*
make options.  I don't think we need to complicate things by
trying to add pkg-config into the mix here.

> 3) We annotate QEMU devices/backends/modules with macros which define its status.
>    It can then be used at build/run times to decide if it's suitable for usage.

What is what Philippe's patches are doing in essence

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 :|

Re: [RFC PATCH 00/10] security: Introduce qemu_security_policy_taint() API

[Daniel P. Berrangé]

On Tue, Sep 14, 2021 at 01:30:27PM +0000, P J P wrote:
> Hello Philippe, all
> 
> >On Thursday, 9 September, 2021, 03:58:40 pm IST, Daniel P. Berrangé <berrange@redhat.com> wrote:
> >On Thu, Sep 09, 2021 at 01:20:14AM +0200, Philippe Mathieu-Daudé wrote:
> >> This series is experimental! The goal is to better limit the
> >> boundary of what code is considerated security critical, and
> >> what is less critical (but still important!).
> >>
> >> This approach was quickly discussed few months ago with Markus
> >> then Daniel. Instead of classifying the code on a file path
> >> basis (see [1]), we insert (runtime) hints into the code
> >> (which survive code movement). Offending unsafe code can
> >> taint the global security policy. By default this policy
> >> is 'none': the current behavior. It can be changed on the
> >> command line to 'warn' to display warnings, and to 'strict'
> >> to prohibit QEMU running with a tainted policy.
> >
> 
> * Thanks so much for restarting this thread. I've been at it intermittently last few
>   months, thinking about how could we annotate the source/module objects.
> 
>    -> [*] https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg04642.html
> 
> * Last time we discussed about having both compile and run time options for users
>   to be able to select the qualified objects/backends/devices as desired.

Right, we have multiple different use cases here

 - People building QEMU who want to cut down what they ship to
   minimize the stuff they support, which is outside the security
   guarantee. This can be OS distros, but also any other consumer
   of QEMU
   
   eg. RHEL wants to cut out almost all non-virtualization stuff.
   There is a quirk here though, that RHEL still includes TCG
   which is considered outside the security guarantee. So a
   simple build time "--secure on|off" doesn't do the job on
   its own.

   We need something to let people understand the consequences
   of the build options they are enabling.

   NB, when I talk of build options, I include both configure/meson
   args, and also the CONFIG_* options set in configs/**/*.mak


 - Application developers want to check that they're not using
   stuff outside the security guarantee, even if the distro
   has enable it.  These need to be able to query whether the
   VM they've launched has undesirable configuration choices.


Some people fall into both groups, some people fall into neither
group.


> * To confirm: How/Where is the security policy defined? Is it
>   device/module specific OR QEMU project wide?

Currently our only definition is in the docs

  https://qemu-project.gitlab.io/qemu/system/security.html#security-requirements

Philippe's patch is proposing tagging against internal QEMU objects
of various types.  I further proposed that we expose this in QMP
so it is introspectable.

I think there's scope for doing stuff at build time with configure
args and *mak CONFIG_* options, but haven't thought what that might
look like.

> > IOW, the reporting via QAPI introspetion is much more important
> > for libvirt and mgmt apps, than any custom cli arg / printfs
> > at the QEMU level.
> >
> 
> * True, while it makes sense to have a solution that is conversant with
>   the management/libvirt layers, It'll be great if we could address qemu/cli
>   other use cases too.
> 
> >it feels like we need
> >  'secure': 'bool'
> 
> * Though we started the (above [*]) discussion with '--security' option in mind,
>   I wonder if 'secure' annotation is much specific. And if we could widen its scope.
> --- x ---
> 
> 
> Source annotations: I've been thinking over following approaches
> ===================
> 
> 1) Segregate the QEMU sources under
> 
>       ../staging/      <= devel/experimental, not for production usage
>       ../src/          <= good for production usage, hence security relevant
>       ../deprecated/   <= Bad for production usage, not security relevant 
> 
>    - This is similar to Linux staging drivers' tree.
>    - Staging drivers are not considered for production usage and hence CVE allocation.
>    - At build time by default we only build sources under ../src/ tree.
>    - But we can still have options to build /staging/ and /deprecated/ trees.  
>    - It's readily understandable to end users.

I don't think we should be working in terms of source files at all.
Some files contain multiple distinct bits of functionality that are
not easily separated, and will have distinct security levels. Also
IMHO it is unpleasant to be moving files around in git to when code
switches between levels.  Also there are distinct criteria here,
both security levels, and support levels - there can be stuff which
is fully supported but considered insecure, and stuff that is
deprecated but considered secure. 

> 2) pkgconfig(1) way:
>    - If we could define per device/backend a configuration (.pc) file which is then used
>      at build/run time to decide which sources are suitable for the build or usage.
> 
>    - I'm trying to experiment with this.

For build time configuration, we have a pretty clear set of
toggles between the configure/meson options, and the CONFIG_*
make options.  I don't think we need to complicate things by
trying to add pkg-config into the mix here.

> 3) We annotate QEMU devices/backends/modules with macros which define its status.
>    It can then be used at build/run times to decide if it's suitable for usage.

What is what Philippe's patches are doing in essence

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 :|

Re: [RFC PATCH 02/10] accel: Use qemu_security_policy_taint(), mark KVM and Xen as safe

[Markus Armbruster]

It's been a while...

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Sep 09, 2021 at 01:20:16AM +0200, Philippe Mathieu-Daudé wrote:
>> Add the AccelClass::secure_policy_supported field to classify
>> safe (within security boundary) vs unsafe accelerators.
>> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  include/qemu/accel.h | 5 +++++
>>  accel/kvm/kvm-all.c  | 1 +
>>  accel/xen/xen-all.c  | 1 +
>>  softmmu/vl.c         | 3 +++
>>  4 files changed, 10 insertions(+)
>> 
>> diff --git a/include/qemu/accel.h b/include/qemu/accel.h
>> index 4f4c283f6fc..895e30be0de 100644
>> --- a/include/qemu/accel.h
>> +++ b/include/qemu/accel.h
>> @@ -44,6 +44,11 @@ typedef struct AccelClass {
>>                         hwaddr start_addr, hwaddr size);
>>  #endif
>>      bool *allowed;
>> +    /*
>> +     * Whether the accelerator is withing QEMU security policy boundary.
>> +     * See: https://www.qemu.org/contribute/security-process/
>> +     */
>> +    bool secure_policy_supported;
>
> The security handling policy is a high level concept that is
> open to variation over time and also by downstream distro
> vendors.

Moreover, the concept of "tainting" isn't limited to "because
security".

> At a code level we should be dealing in a more fundamental
> concept. At an accelerator level we should really jsut
> declare whether or not the accelerator impl is considered
> to be secure against malicious guest code.
>
> eg
>
>     /* Whether this accelerator is secure against execution
>      * of malciious guest machine code */
>     bool secure;

What I'd like to see is a separation of "assertions", like "not meant to
serve as security boundary", and policy.  Yes, this is vague.  Take it
as food for thought.

>>      /*
>>       * Array of global properties that would be applied when specific
>>       * accelerator is chosen. It works like MachineClass.compat_props
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index 0125c17edb8..eb6b9e44df2 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -3623,6 +3623,7 @@ static void kvm_accel_class_init(ObjectClass *oc, void *data)
>>      ac->init_machine = kvm_init;
>>      ac->has_memory = kvm_accel_has_memory;
>>      ac->allowed = &kvm_allowed;
>> +    ac->secure_policy_supported = true;
>>  
>>      object_class_property_add(oc, "kernel-irqchip", "on|off|split",
>>          NULL, kvm_set_kernel_irqchip,
>> diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
>> index 69aa7d018b2..57867af5faf 100644
>> --- a/accel/xen/xen-all.c
>> +++ b/accel/xen/xen-all.c
>> @@ -198,6 +198,7 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
>>      ac->setup_post = xen_setup_post;
>>      ac->allowed = &xen_allowed;
>>      ac->compat_props = g_ptr_array_new();
>> +    ac->secure_policy_supported = true;
>>  
>>      compat_props_add(ac->compat_props, compat, G_N_ELEMENTS(compat));
>>  
>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> index 92c05ac97ee..e4f94e159c3 100644
>> --- a/softmmu/vl.c
>> +++ b/softmmu/vl.c
>> @@ -2388,6 +2388,9 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>>          return 0;
>>      }
>>  
>> +    qemu_security_policy_taint(!ac->secure_policy_supported,
>> +                               "%s accelerator", acc);
>
> We need this information to be introspectable, becuase stuff printed
> to stderr is essentially opaque to libvirt and mgmt apps above.
>
> We don't have a convenient "query-accel" command but I think this
> could possibly fit into 'query-target'. ie the TargetInfo struct
> gain a field:
>  
>
>   ##
>   # @TargetInfo:
>   #
>   # Information describing the QEMU target.
>   #
>   # @arch: the target architecture
>   # @secure: Whether the currently active accelerator for this target
>   #          is secure against execution of malicous guest code
>   #
>   # Since: 1.2
>   ##
>   { 'struct': 'TargetInfo',
>     'data': { 'arch': 'SysEmuTarget',
>               'secure': 'bool'} }

My preferred means of introspection is QAPI schema introspection.  For
QMP, that's query-qmp-schema.  For CLI, it doesn't exist, yet.

If it did, then it would tell us that (QAPIfied) -accel takes an
argument @accel of a certain enumeration type.  We could then tack
suitable feature flags to the enumeration type's values.

If we make the feature flags "special", i.e. known to QAPI, we can then
tie them to policy, like special feature flag 'deprecated' is tied to
policy configured with -compat deprecated-{input,output}=...

Alternatively, leave policy to the management application.

QAPI schema feature flags plus policy are is not a *complete* solution,
just like feature flag 'deprecated' and -compat are not a complete
solution for handling use of deprecated interfaces: we can and do
deprecate usage that isn't tied to a syntactic element in the QAPI
schema.  Example: commit a9b305ba291 deprecated use of socket chardev
option wait together with server=true.

It is, however, a solution for a sizable part of the problem with useful
properties:

* In QEMU, the code is generic (handling of feature flags, policy), and
  the non-generic stuff is declarative (feature flags in the QAPI
  schema).

* No new introspection mechanism: feature flags already exist in QAPI
  schema introspection.

Re: [RFC PATCH 02/10] accel: Use qemu_security_policy_taint(), mark KVM and Xen as safe

[Markus Armbruster]

It's been a while...

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Sep 09, 2021 at 01:20:16AM +0200, Philippe Mathieu-Daudé wrote:
>> Add the AccelClass::secure_policy_supported field to classify
>> safe (within security boundary) vs unsafe accelerators.
>> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  include/qemu/accel.h | 5 +++++
>>  accel/kvm/kvm-all.c  | 1 +
>>  accel/xen/xen-all.c  | 1 +
>>  softmmu/vl.c         | 3 +++
>>  4 files changed, 10 insertions(+)
>> 
>> diff --git a/include/qemu/accel.h b/include/qemu/accel.h
>> index 4f4c283f6fc..895e30be0de 100644
>> --- a/include/qemu/accel.h
>> +++ b/include/qemu/accel.h
>> @@ -44,6 +44,11 @@ typedef struct AccelClass {
>>                         hwaddr start_addr, hwaddr size);
>>  #endif
>>      bool *allowed;
>> +    /*
>> +     * Whether the accelerator is withing QEMU security policy boundary.
>> +     * See: https://www.qemu.org/contribute/security-process/
>> +     */
>> +    bool secure_policy_supported;
>
> The security handling policy is a high level concept that is
> open to variation over time and also by downstream distro
> vendors.

Moreover, the concept of "tainting" isn't limited to "because
security".

> At a code level we should be dealing in a more fundamental
> concept. At an accelerator level we should really jsut
> declare whether or not the accelerator impl is considered
> to be secure against malicious guest code.
>
> eg
>
>     /* Whether this accelerator is secure against execution
>      * of malciious guest machine code */
>     bool secure;

What I'd like to see is a separation of "assertions", like "not meant to
serve as security boundary", and policy.  Yes, this is vague.  Take it
as food for thought.

>>      /*
>>       * Array of global properties that would be applied when specific
>>       * accelerator is chosen. It works like MachineClass.compat_props
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index 0125c17edb8..eb6b9e44df2 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -3623,6 +3623,7 @@ static void kvm_accel_class_init(ObjectClass *oc, void *data)
>>      ac->init_machine = kvm_init;
>>      ac->has_memory = kvm_accel_has_memory;
>>      ac->allowed = &kvm_allowed;
>> +    ac->secure_policy_supported = true;
>>  
>>      object_class_property_add(oc, "kernel-irqchip", "on|off|split",
>>          NULL, kvm_set_kernel_irqchip,
>> diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
>> index 69aa7d018b2..57867af5faf 100644
>> --- a/accel/xen/xen-all.c
>> +++ b/accel/xen/xen-all.c
>> @@ -198,6 +198,7 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
>>      ac->setup_post = xen_setup_post;
>>      ac->allowed = &xen_allowed;
>>      ac->compat_props = g_ptr_array_new();
>> +    ac->secure_policy_supported = true;
>>  
>>      compat_props_add(ac->compat_props, compat, G_N_ELEMENTS(compat));
>>  
>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> index 92c05ac97ee..e4f94e159c3 100644
>> --- a/softmmu/vl.c
>> +++ b/softmmu/vl.c
>> @@ -2388,6 +2388,9 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>>          return 0;
>>      }
>>  
>> +    qemu_security_policy_taint(!ac->secure_policy_supported,
>> +                               "%s accelerator", acc);
>
> We need this information to be introspectable, becuase stuff printed
> to stderr is essentially opaque to libvirt and mgmt apps above.
>
> We don't have a convenient "query-accel" command but I think this
> could possibly fit into 'query-target'. ie the TargetInfo struct
> gain a field:
>  
>
>   ##
>   # @TargetInfo:
>   #
>   # Information describing the QEMU target.
>   #
>   # @arch: the target architecture
>   # @secure: Whether the currently active accelerator for this target
>   #          is secure against execution of malicous guest code
>   #
>   # Since: 1.2
>   ##
>   { 'struct': 'TargetInfo',
>     'data': { 'arch': 'SysEmuTarget',
>               'secure': 'bool'} }

My preferred means of introspection is QAPI schema introspection.  For
QMP, that's query-qmp-schema.  For CLI, it doesn't exist, yet.

If it did, then it would tell us that (QAPIfied) -accel takes an
argument @accel of a certain enumeration type.  We could then tack
suitable feature flags to the enumeration type's values.

If we make the feature flags "special", i.e. known to QAPI, we can then
tie them to policy, like special feature flag 'deprecated' is tied to
policy configured with -compat deprecated-{input,output}=...

Alternatively, leave policy to the management application.

QAPI schema feature flags plus policy are is not a *complete* solution,
just like feature flag 'deprecated' and -compat are not a complete
solution for handling use of deprecated interfaces: we can and do
deprecate usage that isn't tied to a syntactic element in the QAPI
schema.  Example: commit a9b305ba291 deprecated use of socket chardev
option wait together with server=true.

It is, however, a solution for a sizable part of the problem with useful
properties:

* In QEMU, the code is generic (handling of feature flags, policy), and
  the non-generic stuff is declarative (feature flags in the QAPI
  schema).

* No new introspection mechanism: feature flags already exist in QAPI
  schema introspection.