[PATCH 0/8] qapi: last minute improvements/changes to -compat insecure-types

[PATCH 0/8] qapi: last minute improvements/changes to -compat insecure-types

[Daniel P. Berrangé]

This addresses various points raised by Markus about the current
impl/design, that would need to be addressed before this is
included in the release.

NB, the 'warn' option will repeatedly warn on every violation
which can be somewhat unpleasant. Figuring out a mechanism to
make it a one-time warning is an exercise for the future, given
limited time before release.

Daniel P. Berrangé (8):
  qapi: make 'CompatPolicy' the first arg in compat_policy_input_ok
  qapi: add 'warn' option to CompatPolicyInput
  qapi: rename 'insecure-types' to 'insecure-input'
  qapi: drop CompatPolicySecurity and use CompatPolicyInput
  qapi: rename compat_policy_check_security method
  qapi: reverse arg order in compat_policy_security_ok
  qemu-options: document -compat insecure-input parameter
  qapi: print a message before aborting for -compat violations

 docs/system/security.rst     |  2 +-
 include/qapi/compat-policy.h | 12 ++++-----
 qapi/compat.json             | 24 ++++-------------
 qapi/qapi-util.c             | 52 ++++++++++++++++--------------------
 qapi/qapi-visit-core.c       |  4 +--
 qapi/qmp-dispatch.c          |  2 +-
 qapi/qobject-input-visitor.c |  2 +-
 qemu-options.hx              | 34 +++++++++++++++++++----
 qom/object.c                 |  8 +++---
 9 files changed, 72 insertions(+), 68 deletions(-)

-- 
2.56.0

[PATCH 1/8] qapi: make 'CompatPolicy' the first arg in compat_policy_input_ok

[Daniel P. Berrangé]

The typical API design pattern is for a method to be named after
the type of the first arg passed into the method. For the
compat_policy_input_ok method, this points towards 'CompatPolicy'
being the first arg, matching compat_policy_check_security.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/qapi/compat-policy.h |  4 ++--
 qapi/qapi-util.c             | 16 ++++++++--------
 qapi/qapi-visit-core.c       |  4 ++--
 qapi/qmp-dispatch.c          |  2 +-
 qapi/qobject-input-visitor.c |  2 +-
 5 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/include/qapi/compat-policy.h b/include/qapi/compat-policy.h
index f5af2090692..df87d716439 100644
--- a/include/qapi/compat-policy.h
+++ b/include/qapi/compat-policy.h
@@ -18,8 +18,8 @@
 
 extern CompatPolicy compat_policy;
 
-bool compat_policy_input_ok(uint64_t features,
-                            const CompatPolicy *policy,
+bool compat_policy_input_ok(const CompatPolicy *policy,
+                            uint64_t features,
                             ErrorClass error_class,
                             const char *kind, const char *name,
                             Error **errp);
diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
index 38b1cec7a41..93898f57a05 100644
--- a/qapi/qapi-util.c
+++ b/qapi/qapi-util.c
@@ -19,8 +19,8 @@
 
 CompatPolicy compat_policy;
 
-static bool compat_policy_input_ok1(const char *adjective,
-                                    CompatPolicyInput policy,
+static bool compat_policy_input_ok1(CompatPolicyInput policy,
+                                    const char *adjective,
                                     ErrorClass error_class,
                                     const char *kind, const char *name,
                                     Error **errp)
@@ -38,21 +38,21 @@ static bool compat_policy_input_ok1(const char *adjective,
     }
 }
 
-bool compat_policy_input_ok(uint64_t features,
-                            const CompatPolicy *policy,
+bool compat_policy_input_ok(const CompatPolicy *policy,
+                            uint64_t features,
                             ErrorClass error_class,
                             const char *kind, const char *name,
                             Error **errp)
 {
     if ((features & 1u << QAPI_DEPRECATED)
-        && !compat_policy_input_ok1("Deprecated",
-                                    policy->deprecated_input,
+        && !compat_policy_input_ok1(policy->deprecated_input,
+                                    "Deprecated",
                                     error_class, kind, name, errp)) {
         return false;
     }
     if ((features & (1u << QAPI_UNSTABLE))
-        && !compat_policy_input_ok1("Unstable",
-                                    policy->unstable_input,
+        && !compat_policy_input_ok1(policy->unstable_input,
+                                    "Unstable",
                                     error_class, kind, name, errp)) {
         return false;
     }
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 706c61e0260..66d57dd447a 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -410,8 +410,8 @@ static bool input_type_enum(Visitor *v, const char *name, int *obj,
     }
 
     if (lookup->features
-        && !compat_policy_input_ok(lookup->features[value],
-                                   &v->compat_policy,
+        && !compat_policy_input_ok(&v->compat_policy,
+                                   lookup->features[value],
                                    ERROR_CLASS_GENERIC_ERROR,
                                    "value", enum_str, errp)) {
         return false;
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 965cad64998..f2fb8f7976c 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -185,7 +185,7 @@ QDict *coroutine_mixed_fn qmp_dispatch(const QmpCommandList *cmds, QObject *requ
                   "The command %s has not been found", command);
         goto out;
     }
-    if (!compat_policy_input_ok(cmd->features, &compat_policy,
+    if (!compat_policy_input_ok(&compat_policy, cmd->features,
                                 ERROR_CLASS_COMMAND_NOT_FOUND,
                                 "command", command, &err)) {
         goto out;
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index c52d36997df..17d91746d3a 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -667,7 +667,7 @@ static bool qobject_input_policy_reject(Visitor *v, const char *name,
                                         uint64_t features,
                                         Error **errp)
 {
-    return !compat_policy_input_ok(features, &v->compat_policy,
+    return !compat_policy_input_ok(&v->compat_policy, features,
                                    ERROR_CLASS_GENERIC_ERROR,
                                    "parameter", name, errp);
 }
-- 
2.56.0

[PATCH 2/8] qapi: add 'warn' option to CompatPolicyInput

[Daniel P. Berrangé]

This prints a warning on stderr every time a policy violation
is encountered, but accepts the input.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qapi/compat.json |  4 +++-
 qapi/qapi-util.c |  4 ++++
 qemu-options.hx  | 12 ++++++++----
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/qapi/compat.json b/qapi/compat.json
index 0f90ea1f524..dfa3fea0577 100644
--- a/qapi/compat.json
+++ b/qapi/compat.json
@@ -17,10 +17,12 @@
 #
 # @crash: abort() the process
 #
+# @warn: Accept with a warning printed to stderr (Since: 11.2)
+#
 # Since: 6.0
 ##
 { 'enum': 'CompatPolicyInput',
-  'data': [ 'accept', 'reject', 'crash' ] }
+  'data': [ 'accept', 'reject', 'crash', 'warn' ] }
 
 ##
 # @CompatPolicyOutput:
diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
index 93898f57a05..f81a171a5b1 100644
--- a/qapi/qapi-util.c
+++ b/qapi/qapi-util.c
@@ -28,6 +28,10 @@ static bool compat_policy_input_ok1(CompatPolicyInput policy,
     switch (policy) {
     case COMPAT_POLICY_INPUT_ACCEPT:
         return true;
+    case COMPAT_POLICY_INPUT_WARN:
+        warn_report("%s %s %s disabled by policy",
+                    adjective, kind, name);
+        return true;
     case COMPAT_POLICY_INPUT_REJECT:
         error_set(errp, error_class, "%s %s %s disabled by policy",
                   adjective, kind, name);
diff --git a/qemu-options.hx b/qemu-options.hx
index f72f30e246d..16d9c6c9072 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4693,9 +4693,9 @@ DEFHEADING()
 DEFHEADING(Debug/Expert options:)
 
 DEF("compat", HAS_ARG, QEMU_OPTION_compat,
-    "-compat [deprecated-input=accept|reject|crash][,deprecated-output=accept|hide]\n"
+    "-compat [deprecated-input=accept|reject|crash|warn][,deprecated-output=accept|hide]\n"
     "                Policy for handling deprecated management interfaces\n"
-    "-compat [unstable-input=accept|reject|crash][,unstable-output=accept|hide]\n"
+    "-compat [unstable-input=accept|reject|crash|warn][,unstable-output=accept|hide]\n"
     "                Policy for handling unstable management interfaces\n",
     QEMU_ARCH_ALL)
 SRST
@@ -4703,7 +4703,9 @@ SRST
     Set policy for handling deprecated management interfaces (experimental):
 
     ``deprecated-input=accept`` (default)
-        Accept deprecated commands and arguments
+        Accept deprecated commands and arguments silently
+    ``deprecated-input=warn``
+        Accept deprecated commands and arguments with warning on stderr
     ``deprecated-input=reject``
         Reject deprecated commands and arguments
     ``deprecated-input=crash``
@@ -4719,7 +4721,9 @@ SRST
     Set policy for handling unstable management interfaces (experimental):
 
     ``unstable-input=accept`` (default)
-        Accept unstable commands and arguments
+        Accept unstable commands and arguments silently
+    ``unstable-input=warn``
+        Accept unstable commands and arguments with warning on stderr
     ``unstable-input=reject``
         Reject unstable commands and arguments
     ``unstable-input=crash``
-- 
2.56.0

[PATCH 3/8] qapi: rename 'insecure-types' to 'insecure-input'

[Daniel P. Berrangé]

This provides better conceptual naming alignment with other
-compat flags.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 docs/system/security.rst | 2 +-
 qapi/compat.json         | 4 ++--
 qapi/qapi-util.c         | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/docs/system/security.rst b/docs/system/security.rst
index 75e39caedea..e211fdd2344 100644
--- a/docs/system/security.rst
+++ b/docs/system/security.rst
@@ -168,7 +168,7 @@ eligible for CVE assignment. Annotations will be extended to other backend
 and object types over time, to make their security status explicit.
 
 It is possible to control or identify the usage of types that do not offer
-an explicit security boundary using the ``insecure-types`` parameter to the
+an explicit security boundary using the ``insecure-input`` parameter to the
 ``-compat`` argument, which accepts three values:
 
  * accept: usage of any type will be permitted. This is the current
diff --git a/qapi/compat.json b/qapi/compat.json
index dfa3fea0577..f28c24149c8 100644
--- a/qapi/compat.json
+++ b/qapi/compat.json
@@ -77,7 +77,7 @@
 # @unstable-output: how to handle unstable output (default 'accept')
 #     (since 6.2)
 #
-# @insecure-types: how to handle types that are not declared secure
+# @insecure-input: how to handle types that are not declared secure
 #     (default 'accept') (since 11.2)
 #
 # Since: 6.0
@@ -87,4 +87,4 @@
             '*deprecated-output': 'CompatPolicyOutput',
             '*unstable-input': 'CompatPolicyInput',
             '*unstable-output': 'CompatPolicyOutput',
-            '*insecure-types': 'CompatPolicySecurity' } }
+            '*insecure-input': 'CompatPolicySecurity' } }
diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
index f81a171a5b1..e6f3006b16c 100644
--- a/qapi/qapi-util.c
+++ b/qapi/qapi-util.c
@@ -72,7 +72,7 @@ bool compat_policy_check_security(const CompatPolicy *policy,
         return true;
     }
 
-    switch (policy->insecure_types) {
+    switch (policy->insecure_input) {
     case COMPAT_POLICY_SECURITY_ACCEPT:
         return true;
 
-- 
2.56.0

[PATCH 4/8] qapi: drop CompatPolicySecurity and use CompatPolicyInput

[Daniel P. Berrangé]

With CompatPolicyInput gaining a "warn" option, the CompatPolicySecurity
type is redundant.

Note the error message for insecure-input changes from

   Type 'rtl8139' does not provide a security boundary
   to protect against untrusted data or actions

To

   Insecure QOM type rtl8139 disabled by policy

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qapi/compat.json | 18 +-----------------
 qapi/qapi-util.c | 21 ++++-----------------
 2 files changed, 5 insertions(+), 34 deletions(-)

diff --git a/qapi/compat.json b/qapi/compat.json
index f28c24149c8..7790a7896a8 100644
--- a/qapi/compat.json
+++ b/qapi/compat.json
@@ -37,22 +37,6 @@
 { 'enum': 'CompatPolicyOutput',
   'data': [ 'accept', 'hide' ] }
 
-##
-# @CompatPolicySecurity:
-#     Policy for handling any devices or backends which do not provide
-#     a security boundary to protect against untrusted environments
-#
-# @accept: Allow any usage
-#
-# @reject: Reject with an error reported
-#
-# @warn: Allow any usage, with a warning reported
-#
-# Since: 11.2
-##
-{ 'enum': 'CompatPolicySecurity',
-  'data': [ 'accept', 'reject', 'warn' ] }
-
 ##
 # @CompatPolicy:
 #     Policy for handling deprecated management interfaces.
@@ -87,4 +71,4 @@
             '*deprecated-output': 'CompatPolicyOutput',
             '*unstable-input': 'CompatPolicyInput',
             '*unstable-output': 'CompatPolicyOutput',
-            '*insecure-input': 'CompatPolicySecurity' } }
+            '*insecure-input': 'CompatPolicyInput' } }
diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
index e6f3006b16c..36926b5003d 100644
--- a/qapi/qapi-util.c
+++ b/qapi/qapi-util.c
@@ -72,23 +72,10 @@ bool compat_policy_check_security(const CompatPolicy *policy,
         return true;
     }
 
-    switch (policy->insecure_input) {
-    case COMPAT_POLICY_SECURITY_ACCEPT:
-        return true;
-
-    case COMPAT_POLICY_SECURITY_REJECT:
-        error_setg(errp, "Type '%s' does not provide a security boundary "
-                   "to protect against untrusted data or actions", typename);
-        return false;
-
-    case COMPAT_POLICY_SECURITY_WARN:
-        warn_report("Type '%s' does not provide a security boundary "
-                    "to protect against untrusted data or actions", typename);
-        return true;
-
-    default:
-        g_assert_not_reached();
-    }
+    return compat_policy_input_ok1(policy->insecure_input,
+                                   "Insecure",
+                                   ERROR_CLASS_GENERIC_ERROR,
+                                   "QOM type", typename, errp);
 }
 
 
-- 
2.56.0

[PATCH 5/8] qapi: rename compat_policy_check_security method

[Daniel P. Berrangé]

Rename to compat_policy_security_ok to match the naming convention
established by compat_policy_input_ok.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/qapi/compat-policy.h | 8 ++++----
 qapi/qapi-util.c             | 8 ++++----
 qom/object.c                 | 8 ++++----
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/include/qapi/compat-policy.h b/include/qapi/compat-policy.h
index df87d716439..b7b543ffe74 100644
--- a/include/qapi/compat-policy.h
+++ b/include/qapi/compat-policy.h
@@ -24,10 +24,10 @@ bool compat_policy_input_ok(const CompatPolicy *policy,
                             const char *kind, const char *name,
                             Error **errp);
 
-bool compat_policy_check_security(const CompatPolicy *policy,
-                                  const char *typename,
-                                  bool is_secure,
-                                  Error **errp);
+bool compat_policy_security_ok(const CompatPolicy *policy,
+                               const char *typename,
+                               bool is_secure,
+                               Error **errp);
 
 /*
  * Create a QObject input visitor for @obj for use with QMP
diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
index 36926b5003d..f78d45a1a51 100644
--- a/qapi/qapi-util.c
+++ b/qapi/qapi-util.c
@@ -63,10 +63,10 @@ bool compat_policy_input_ok(const CompatPolicy *policy,
     return true;
 }
 
-bool compat_policy_check_security(const CompatPolicy *policy,
-                                  const char *typename,
-                                  bool is_secure,
-                                  Error **errp)
+bool compat_policy_security_ok(const CompatPolicy *policy,
+                               const char *typename,
+                               bool is_secure,
+                               Error **errp)
 {
     if (is_secure) {
         return true;
diff --git a/qom/object.c b/qom/object.c
index 96498f3d001..ceb8e9eebec 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -3276,10 +3276,10 @@ void object_class_property_set_description(ObjectClass *klass,
 
 bool object_class_check_security(ObjectClass *klass, Error **errp)
 {
-    return compat_policy_check_security(&compat_policy,
-                                        object_class_get_name(klass),
-                                        object_class_is_secure(klass),
-                                        errp);
+    return compat_policy_security_ok(&compat_policy,
+                                     object_class_get_name(klass),
+                                     object_class_is_secure(klass),
+                                     errp);
 }
 
 static void object_class_init(ObjectClass *klass, const void *data)
-- 
2.56.0

[PATCH 6/8] qapi: reverse arg order in compat_policy_security_ok

[Daniel P. Berrangé]

Match the compat_policy_input_ok design more closely by having
the "relevant facts" being checked as the 2nd argument.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/qapi/compat-policy.h | 2 +-
 qapi/qapi-util.c             | 2 +-
 qom/object.c                 | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/qapi/compat-policy.h b/include/qapi/compat-policy.h
index b7b543ffe74..53c96ce1a26 100644
--- a/include/qapi/compat-policy.h
+++ b/include/qapi/compat-policy.h
@@ -25,8 +25,8 @@ bool compat_policy_input_ok(const CompatPolicy *policy,
                             Error **errp);
 
 bool compat_policy_security_ok(const CompatPolicy *policy,
-                               const char *typename,
                                bool is_secure,
+                               const char *typename,
                                Error **errp);
 
 /*
diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
index f78d45a1a51..c845cadbdf2 100644
--- a/qapi/qapi-util.c
+++ b/qapi/qapi-util.c
@@ -64,8 +64,8 @@ bool compat_policy_input_ok(const CompatPolicy *policy,
 }
 
 bool compat_policy_security_ok(const CompatPolicy *policy,
-                               const char *typename,
                                bool is_secure,
+                               const char *typename,
                                Error **errp)
 {
     if (is_secure) {
diff --git a/qom/object.c b/qom/object.c
index ceb8e9eebec..30699f91279 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -3277,8 +3277,8 @@ void object_class_property_set_description(ObjectClass *klass,
 bool object_class_check_security(ObjectClass *klass, Error **errp)
 {
     return compat_policy_security_ok(&compat_policy,
-                                     object_class_get_name(klass),
                                      object_class_is_secure(klass),
+                                     object_class_get_name(klass),
                                      errp);
 }
 
-- 
2.56.0

[PATCH 7/8] qemu-options: document -compat insecure-input parameter

[Daniel P. Berrangé]

This adds documentation for the previously added 'insecure-input'
parameter to -compat, which controls policy for insecure
configuration requests.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qemu-options.hx | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 16d9c6c9072..0487e74bb3e 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4696,7 +4696,9 @@ DEF("compat", HAS_ARG, QEMU_OPTION_compat,
     "-compat [deprecated-input=accept|reject|crash|warn][,deprecated-output=accept|hide]\n"
     "                Policy for handling deprecated management interfaces\n"
     "-compat [unstable-input=accept|reject|crash|warn][,unstable-output=accept|hide]\n"
-    "                Policy for handling unstable management interfaces\n",
+    "                Policy for handling unstable management interfaces\n"
+    "-compat [insecure-input=accept|reject|crash|warn]\n"
+    "                Policy for handling requests for insecure configuration requests\n",
     QEMU_ARCH_ALL)
 SRST
 ``-compat [deprecated-input=<input-policy>][,deprecated-output=<output-policy>]``
@@ -4734,6 +4736,24 @@ SRST
         Suppress unstable command results and events
 
     Limitation: covers only syntactic aspects of QMP.
+
+``-compat [insecure-input=<input-policy>]
+    Set policy for handling requests for insecure configuration
+    requests:
+
+    ``insecure-input=accept`` (default)
+        Accept insecure configurations requests silently
+    ``insecure-input=warn``
+        Accept insecure configuration requests with warning on stderr
+    ``insecure-input=reject``
+        Reject insecure configuration requests
+    ``insecure-input=crash``
+        Crash on insecure configuration requests
+
+    Limitation: an insecure configuration is one where QEMU has
+    not intended to provide a security boundary to protect against
+    malicious usage. Covers only QOM types for devices, machine types
+    and accelerators.
 ERST
 
 DEF("fw_cfg", HAS_ARG, QEMU_OPTION_fwcfg,
-- 
2.56.0

[PATCH 8/8] qapi: print a message before aborting for -compat violations

[Daniel P. Berrangé]

When using the 'crash' policy with -compat options, QEMU will
abort() on a policy violation. All the user sees is a core
dump though and while the stack trace will ultimately show
what the violation was, it can be hard to reverse engineer
the cause when variables are optimized out. If the crash is
within a CI system, it can be even harder to determine what
the cause was as a core dump may not be available.

Printing a message when crashing is easier to diagnose when
limited context is available.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qapi/qapi-util.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
index c845cadbdf2..1141813747b 100644
--- a/qapi/qapi-util.c
+++ b/qapi/qapi-util.c
@@ -37,6 +37,9 @@ static bool compat_policy_input_ok1(CompatPolicyInput policy,
                   adjective, kind, name);
         return false;
     case COMPAT_POLICY_INPUT_CRASH:
+        error_report("%s %s %s disabled by policy, aborting",
+                    adjective, kind, name);
+        /* fallthrough */
     default:
         abort();
     }
-- 
2.56.0

Re: [PATCH 4/8] qapi: drop CompatPolicySecurity and use CompatPolicyInput

[Markus Armbruster]

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

> With CompatPolicyInput gaining a "warn" option, the CompatPolicySecurity
> type is redundant.
>
> Note the error message for insecure-input changes from
>
>    Type 'rtl8139' does not provide a security boundary
>    to protect against untrusted data or actions
>
> To
>
>    Insecure QOM type rtl8139 disabled by policy

This loses a bit of information.  Do we care?  I show a way to retain it
below.

>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  qapi/compat.json | 18 +-----------------
>  qapi/qapi-util.c | 21 ++++-----------------
>  2 files changed, 5 insertions(+), 34 deletions(-)
>
> diff --git a/qapi/compat.json b/qapi/compat.json
> index f28c24149c8..7790a7896a8 100644
> --- a/qapi/compat.json
> +++ b/qapi/compat.json
> @@ -37,22 +37,6 @@
>  { 'enum': 'CompatPolicyOutput',
>    'data': [ 'accept', 'hide' ] }
>  
> -##
> -# @CompatPolicySecurity:
> -#     Policy for handling any devices or backends which do not provide
> -#     a security boundary to protect against untrusted environments
> -#
> -# @accept: Allow any usage
> -#
> -# @reject: Reject with an error reported
> -#
> -# @warn: Allow any usage, with a warning reported
> -#
> -# Since: 11.2
> -##
> -{ 'enum': 'CompatPolicySecurity',
> -  'data': [ 'accept', 'reject', 'warn' ] }
> -
>  ##
>  # @CompatPolicy:
>  #     Policy for handling deprecated management interfaces.
> @@ -87,4 +71,4 @@
>              '*deprecated-output': 'CompatPolicyOutput',
>              '*unstable-input': 'CompatPolicyInput',
>              '*unstable-output': 'CompatPolicyOutput',
> -            '*insecure-input': 'CompatPolicySecurity' } }
> +            '*insecure-input': 'CompatPolicyInput' } }

This adds policy value "crash", doesn't it?  The commit message should
mention it.

> diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
> index e6f3006b16c..36926b5003d 100644
> --- a/qapi/qapi-util.c
> +++ b/qapi/qapi-util.c
> @@ -72,23 +72,10 @@ bool compat_policy_check_security(const CompatPolicy *policy,
>          return true;
>      }
>  
> -    switch (policy->insecure_input) {
> -    case COMPAT_POLICY_SECURITY_ACCEPT:
> -        return true;
> -
> -    case COMPAT_POLICY_SECURITY_REJECT:
> -        error_setg(errp, "Type '%s' does not provide a security boundary "
> -                   "to protect against untrusted data or actions", typename);
> -        return false;
> -
> -    case COMPAT_POLICY_SECURITY_WARN:
> -        warn_report("Type '%s' does not provide a security boundary "
> -                    "to protect against untrusted data or actions", typename);
> -        return true;
> -
> -    default:
> -        g_assert_not_reached();
> -    }
> +    return compat_policy_input_ok1(policy->insecure_input,
> +                                   "Insecure",
> +                                   ERROR_CLASS_GENERIC_ERROR,
> +                                   "QOM type", typename, errp);
>  }

... then try something like this:

       if (!compat_policy_input_ok1(...) {
           error_append_hint(errp, ...);
       }

Don't forget ERRP_GUARD() then, error_append_hint() needs it.

Re: [PATCH 7/8] qemu-options: document -compat insecure-input parameter

[Markus Armbruster]

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

> This adds documentation for the previously added 'insecure-input'
> parameter to -compat, which controls policy for insecure
> configuration requests.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  qemu-options.hx | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 16d9c6c9072..0487e74bb3e 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4696,7 +4696,9 @@ DEF("compat", HAS_ARG, QEMU_OPTION_compat,
>      "-compat [deprecated-input=accept|reject|crash|warn][,deprecated-output=accept|hide]\n"
>      "                Policy for handling deprecated management interfaces\n"
>      "-compat [unstable-input=accept|reject|crash|warn][,unstable-output=accept|hide]\n"
> -    "                Policy for handling unstable management interfaces\n",
> +    "                Policy for handling unstable management interfaces\n"
> +    "-compat [insecure-input=accept|reject|crash|warn]\n"
> +    "                Policy for handling requests for insecure configuration requests\n",
>      QEMU_ARCH_ALL)
>  SRST
>  ``-compat [deprecated-input=<input-policy>][,deprecated-output=<output-policy>]``
> @@ -4734,6 +4736,24 @@ SRST
>          Suppress unstable command results and events
>  
>      Limitation: covers only syntactic aspects of QMP.
> +
> +``-compat [insecure-input=<input-policy>]
> +    Set policy for handling requests for insecure configuration
> +    requests:
> +
> +    ``insecure-input=accept`` (default)
> +        Accept insecure configurations requests silently
> +    ``insecure-input=warn``
> +        Accept insecure configuration requests with warning on stderr
> +    ``insecure-input=reject``
> +        Reject insecure configuration requests
> +    ``insecure-input=crash``
> +        Crash on insecure configuration requests
> +
> +    Limitation: an insecure configuration is one where QEMU has
> +    not intended to provide a security boundary to protect against

"Where QEMU has not intended to provide" sounds odd to me.  Mind, I'm
not a native speaker.  "Where QEMU does not provide".

> +    malicious usage. Covers only QOM types for devices, machine types
> +    and accelerators.

The policy check is in

* qemu_create_machine(), which covers machine creation.

* do_configure_accelerator(), which covers accelerator creation.

* qdev_device_add_from_qdict(), which covers *some* device creation:

  * device_add and -device always

  * Half of -audio model=hda: covers the "intel-hda" device, but not the
    "hda-duplex" device.  See intel_hda_and_codec_init().

  * The "primary device" created in failover_hide_primary_device() for
    the virtio-net standby feature.

  * The "usb-host" device device created in xen-usb.c for some reason.

  There's plenty of device creation it doesn't cover.  Isn't this
  problematic?  I certainly expected it to cover *all* device creation!

  Say board code creates a certain onboard device with the usual low
  level interfaces like qdev_new() and qdev_realize().  Say we mark the
  onboard device insecure.  The policy check doesn't apply.  We need to
  manually mark the machine type insecure as well.  Feels awfully prone
  to human error.

  Moreover, what if the onboard device is configurable?  Then the
  machine type may be secure unless you configure this insecure device.
  Marking the machine type would be too coarse.

  Could we check policy in qdev_realize() instead?

>  ERST
>  
>  DEF("fw_cfg", HAS_ARG, QEMU_OPTION_fwcfg,

Re: [PATCH 2/8] qapi: add 'warn' option to CompatPolicyInput

[Markus Armbruster]

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

> This prints a warning on stderr every time a policy violation
> is encountered, but accepts the input.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

"Every time" can be annoying.  It's commonly easy to avoid, e.g. with
warn_report_once(), but not here, so we don't.  Because it's annoying,
we don't make it the default, unlike other deprecation warnings.

If you agree with the above, please work it into the commit message, and
consider whether documentation should mention it, too.

Re: [PATCH 2/8] qapi: add 'warn' option to CompatPolicyInput

[Daniel P. Berrangé]

On Thu, Oct 29, 2026 at 07:56:52AM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > This prints a warning on stderr every time a policy violation
> > is encountered, but accepts the input.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> "Every time" can be annoying.  It's commonly easy to avoid, e.g. with
> warn_report_once(), but not here, so we don't.  Because it's annoying,
> we don't make it the default, unlike other deprecation warnings.
> 
> If you agree with the above, please work it into the commit message, and
> consider whether documentation should mention it, too.

Ok.

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH 4/8] qapi: drop CompatPolicySecurity and use CompatPolicyInput

[Daniel P. Berrangé]

On Thu, Oct 29, 2026 at 07:15:38AM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > With CompatPolicyInput gaining a "warn" option, the CompatPolicySecurity
> > type is redundant.
> >
> > Note the error message for insecure-input changes from
> >
> >    Type 'rtl8139' does not provide a security boundary
> >    to protect against untrusted data or actions
> >
> > To
> >
> >    Insecure QOM type rtl8139 disabled by policy
> 
> This loses a bit of information.  Do we care?  I show a way to retain it
> below.
> 
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  qapi/compat.json | 18 +-----------------
> >  qapi/qapi-util.c | 21 ++++-----------------
> >  2 files changed, 5 insertions(+), 34 deletions(-)
> >
> > diff --git a/qapi/compat.json b/qapi/compat.json
> > index f28c24149c8..7790a7896a8 100644
> > --- a/qapi/compat.json
> > +++ b/qapi/compat.json
> > @@ -37,22 +37,6 @@
> >  { 'enum': 'CompatPolicyOutput',
> >    'data': [ 'accept', 'hide' ] }
> >  
> > -##
> > -# @CompatPolicySecurity:
> > -#     Policy for handling any devices or backends which do not provide
> > -#     a security boundary to protect against untrusted environments
> > -#
> > -# @accept: Allow any usage
> > -#
> > -# @reject: Reject with an error reported
> > -#
> > -# @warn: Allow any usage, with a warning reported
> > -#
> > -# Since: 11.2
> > -##
> > -{ 'enum': 'CompatPolicySecurity',
> > -  'data': [ 'accept', 'reject', 'warn' ] }
> > -
> >  ##
> >  # @CompatPolicy:
> >  #     Policy for handling deprecated management interfaces.
> > @@ -87,4 +71,4 @@
> >              '*deprecated-output': 'CompatPolicyOutput',
> >              '*unstable-input': 'CompatPolicyInput',
> >              '*unstable-output': 'CompatPolicyOutput',
> > -            '*insecure-input': 'CompatPolicySecurity' } }
> > +            '*insecure-input': 'CompatPolicyInput' } }
> 
> This adds policy value "crash", doesn't it?  The commit message should
> mention it.

Ok

> 
> > diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
> > index e6f3006b16c..36926b5003d 100644
> > --- a/qapi/qapi-util.c
> > +++ b/qapi/qapi-util.c
> > @@ -72,23 +72,10 @@ bool compat_policy_check_security(const CompatPolicy *policy,
> >          return true;
> >      }
> >  
> > -    switch (policy->insecure_input) {
> > -    case COMPAT_POLICY_SECURITY_ACCEPT:
> > -        return true;
> > -
> > -    case COMPAT_POLICY_SECURITY_REJECT:
> > -        error_setg(errp, "Type '%s' does not provide a security boundary "
> > -                   "to protect against untrusted data or actions", typename);
> > -        return false;
> > -
> > -    case COMPAT_POLICY_SECURITY_WARN:
> > -        warn_report("Type '%s' does not provide a security boundary "
> > -                    "to protect against untrusted data or actions", typename);
> > -        return true;
> > -
> > -    default:
> > -        g_assert_not_reached();
> > -    }
> > +    return compat_policy_input_ok1(policy->insecure_input,
> > +                                   "Insecure",
> > +                                   ERROR_CLASS_GENERIC_ERROR,
> > +                                   "QOM type", typename, errp);
> >  }
> 
> ... then try something like this:
> 
>        if (!compat_policy_input_ok1(...) {
>            error_append_hint(errp, ...);
>        }
> 
> Don't forget ERRP_GUARD() then, error_append_hint() needs it.

Yep, that's a nice idea.

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH 7/8] qemu-options: document -compat insecure-input parameter

[Daniel P. Berrangé]

On Thu, Oct 29, 2026 at 07:52:36AM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > This adds documentation for the previously added 'insecure-input'
> > parameter to -compat, which controls policy for insecure
> > configuration requests.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  qemu-options.hx | 22 +++++++++++++++++++++-
> >  1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 16d9c6c9072..0487e74bb3e 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -4696,7 +4696,9 @@ DEF("compat", HAS_ARG, QEMU_OPTION_compat,
> >      "-compat [deprecated-input=accept|reject|crash|warn][,deprecated-output=accept|hide]\n"
> >      "                Policy for handling deprecated management interfaces\n"
> >      "-compat [unstable-input=accept|reject|crash|warn][,unstable-output=accept|hide]\n"
> > -    "                Policy for handling unstable management interfaces\n",
> > +    "                Policy for handling unstable management interfaces\n"
> > +    "-compat [insecure-input=accept|reject|crash|warn]\n"
> > +    "                Policy for handling requests for insecure configuration requests\n",
> >      QEMU_ARCH_ALL)
> >  SRST
> >  ``-compat [deprecated-input=<input-policy>][,deprecated-output=<output-policy>]``
> > @@ -4734,6 +4736,24 @@ SRST
> >          Suppress unstable command results and events
> >  
> >      Limitation: covers only syntactic aspects of QMP.
> > +
> > +``-compat [insecure-input=<input-policy>]
> > +    Set policy for handling requests for insecure configuration
> > +    requests:
> > +
> > +    ``insecure-input=accept`` (default)
> > +        Accept insecure configurations requests silently
> > +    ``insecure-input=warn``
> > +        Accept insecure configuration requests with warning on stderr
> > +    ``insecure-input=reject``
> > +        Reject insecure configuration requests
> > +    ``insecure-input=crash``
> > +        Crash on insecure configuration requests
> > +
> > +    Limitation: an insecure configuration is one where QEMU has
> > +    not intended to provide a security boundary to protect against
> 
> "Where QEMU has not intended to provide" sounds odd to me.  Mind, I'm
> not a native speaker.  "Where QEMU does not provide".

Yeah, that language choice was me by pessimistic expecting that
QEMU is full of undiscovered flaws. I'll change to what you
suggested.

> > +    malicious usage. Covers only QOM types for devices, machine types
> > +    and accelerators.
> 
> The policy check is in
> 
> * qemu_create_machine(), which covers machine creation.
> 
> * do_configure_accelerator(), which covers accelerator creation.
> 
> * qdev_device_add_from_qdict(), which covers *some* device creation:
> 
>   * device_add and -device always
> 
>   * Half of -audio model=hda: covers the "intel-hda" device, but not the
>     "hda-duplex" device.  See intel_hda_and_codec_init().
> 
>   * The "primary device" created in failover_hide_primary_device() for
>     the virtio-net standby feature.
> 
>   * The "usb-host" device device created in xen-usb.c for some reason.
> 
>   There's plenty of device creation it doesn't cover.  Isn't this
>   problematic?  I certainly expected it to cover *all* device creation!

Yes, eventually, but not yet....

> 
>   Say board code creates a certain onboard device with the usual low
>   level interfaces like qdev_new() and qdev_realize().  Say we mark the
>   onboard device insecure.  The policy check doesn't apply.  We need to
>   manually mark the machine type insecure as well.  Feels awfully prone
>   to human error.
> 
>   Moreover, what if the onboard device is configurable?  Then the
>   machine type may be secure unless you configure this insecure device.
>   Marking the machine type would be too coarse.
> 
>   Could we check policy in qdev_realize() instead?

Yes and no.

To make this a tractable problem I only wanted to add '.secure = true'
annotations for public facing types initially, so needed the check in
the device_add/-device code path only.

This is sufficient to let us tell bug report filers to take their demo
command line and add

  '-compat insecure-input=reject -nodfaults'

as a pre-requisite for a bug being considered a security flaw
egligible for CVE handling.


If we put a check in qdev_realize(), then we need to add annotations
for *all* types, before the -compat insecure-input=....  option is
viable to use. That requires a much bigger effort to mark all types.

In the 12.0 dev cycle, my intention is to

 a) Expand ".secure = true" markers to all parents of leaf node types
    marked secure, and enforce them.
 b) Expand ".secure = true" markers to all built-in devices that are
    indirectly created by a leaf node type marked secure, when
    -nodefaults is present

Once (b) is done, then qdev_realize() will likely be the better place
to put the policy check.

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|