Re: [PATCH v2 03/32] qapi: add 'insecure-types' option for -compat argument

[Markus Armbruster <armbru@redhat.com>]

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

> This introduces a new 'insecure-types' option for the 'compat'
> argument that accepts three values
>
>  * accept: Allow any usage
>  * reject: Reject with an error reported
>  * warn: Allow any usage, with a warning reported
>
> For historical compatibility it defaults to 'accept'.
>
> The 'reject' and 'warn' values will take effect for any type
> that has been explicitly marked insecure, or is lacking an
> explicit declaration of its security status.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Suggest to mention that the new option doesn't do anything, yet.

> ---
>  include/qapi/compat-policy.h |  5 +++++
>  qapi/compat.json             | 24 +++++++++++++++++++++++-
>  qapi/qapi-util.c             | 30 ++++++++++++++++++++++++++++++
>  3 files changed, 58 insertions(+), 1 deletion(-)
>
> diff --git a/include/qapi/compat-policy.h b/include/qapi/compat-policy.h
> index ea65e10744..b2d0835c36 100644
> --- a/include/qapi/compat-policy.h
> +++ b/include/qapi/compat-policy.h
> @@ -24,6 +24,11 @@ bool compat_policy_input_ok(uint64_t features,
>                              const char *kind, const char *name,
>                              Error **errp);
>  
> +bool compat_policy_check_security(CompatPolicy *policy,
> +                                  const char *typename,
> +                                  bool isSecure,

is_secure

> +                                  Error **errp);
> +
>  /*
>   * Create a QObject input visitor for @obj for use with QMP
>   *
> diff --git a/qapi/compat.json b/qapi/compat.json
> index 90b8d51cf2..dcef10a3a5 100644
> --- a/qapi/compat.json
> +++ b/qapi/compat.json
> @@ -37,6 +37,24 @@
>  { '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

Please wrap like this:

   # 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: 10.2
> +##
> +{ 'enum': 'CompatPolicySecurity',
> +  'data': [ 'accept', 'reject', 'warn' ] }
> +
>  ##
>  # @CompatPolicy:
>  #
> @@ -62,10 +80,14 @@
>  # @unstable-output: how to handle unstable output (default 'accept')
>  #     (since 6.2)
>  #
> +# @insecure-types: how to handle types that are not declared
> +#     secure (default 'accept') (since 10.2)
> +#

Please wrap like this:

   # @insecure-types: how to handle types that are not declared secure
   #     (default 'accept') (since 10.2)

>  # Since: 6.0
>  ##
>  { 'struct': 'CompatPolicy',
>    'data': { '*deprecated-input': 'CompatPolicyInput',
>              '*deprecated-output': 'CompatPolicyOutput',
>              '*unstable-input': 'CompatPolicyInput',
> -            '*unstable-output': 'CompatPolicyOutput' } }
> +            '*unstable-output': 'CompatPolicyOutput',
> +            '*insecure-types': 'CompatPolicySecurity' } }
> diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
> index 3d849fe034..ef982d903e 100644
> --- a/qapi/qapi-util.c
> +++ b/qapi/qapi-util.c
> @@ -14,6 +14,7 @@
>  #include "qapi/compat-policy.h"
>  #include "qapi/error.h"
>  #include "qemu/ctype.h"
> +#include "qemu/error-report.h"
>  #include "qapi/qmp/qerror.h"
>  
>  CompatPolicy compat_policy;
> @@ -58,6 +59,35 @@ bool compat_policy_input_ok(uint64_t features,
>      return true;
>  }
>  
> +bool compat_policy_check_security(CompatPolicy *policy,
> +                                  const char *typename,
> +                                  bool isSecure,
> +                                  Error **errp)
> +{
> +    if (isSecure) {
> +        return true;
> +    }
> +
> +    switch (policy->insecure_types) {
> +    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 workloads", typename);
> +        return false;
> +
> +    case COMPAT_POLICY_SECURITY_WARN:
> +        warn_report("Type '%s' does not provide a security boundary "
> +                    "to protect against untrusted workloads", typename);
> +        return true;

The error messages are hard to judge until we see uses.  I figure what
"untrusted workloads" actually means depends on the type.  For a device,
it's probably an untrusted guest.  For a block backend, it could be an
untrusted image.

> +
> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +
> +
>  const char *qapi_enum_lookup(const QEnumLookup *lookup, int val)
>  {
>      assert(val >= 0 && val < lookup->size);