Re: [PULL 02/14] qapi: add 'insecure-types' option for -compat argument

[Markus Armbruster <armbru@redhat.com>]

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

> On Tue, Oct 27, 2026 at 10:46:00AM +0100, Markus Armbruster wrote:
>> This has long been merged.  I couldn't review it at the time, and
>> merging it without my review was clearly appropriate.
>> 
>> I have questions / doubts about the external interface I'd like to
>> discuss now.
>
> ... just in time for freeze if we need any urgent changes
> before this gets into a release.

Basically last minute :)

>> 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.
>> >
>> > This new command line option is currently a no-op, but will
>> > become functional as following patches enable the checks.
>> >
>> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>
> snip
>
>> > diff --git a/qapi/compat.json b/qapi/compat.json
>> > index 90b8d51cf27..b54f8eb3713 100644
>> > --- a/qapi/compat.json
>> > +++ b/qapi/compat.json
>> > @@ -37,6 +37,23 @@
>> >  { '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:
>> >  #
>> > @@ -62,10 +79,14 @@
>>    # Policy for handling deprecated management interfaces.
>>    #
>>    # This is intended for testing users of the management interfaces.
>>    #
>>    # Limitation: covers only syntactic aspects of QMP, i.e. stuff tagged
>>    # with feature 'deprecated' or 'unstable'.  We may want to extend it
>>    # to cover semantic aspects and CLI.
>>    #
>>    # Limitation: deprecated-output policy @hide is not implemented for
>>    # enumeration values.  They behave the same as with policy @accept.
>>    #
>>    # @deprecated-input: how to handle deprecated input (default 'accept')
>>    #
>>    # @deprecated-output: how to handle deprecated output (default
>>    #     'accept')
>>    #
>>    # @unstable-input: how to handle unstable input (default 'accept')
>>    #     (since 6.2)
>>    #
>> >  # @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 11.2)
>> 
>> Minor issue: "types" is ambiguous without context.  We call lots of
>> things "types" in our external interfaces.  It's actually QOM types
>> here.  Let's say so.  I can take care of it.
>> 
>> Similar confusion exists elsewhere[*], but let's not digress.
>
> It is currently  QOM types. It might even stay exclusively QOM types,
> but not guaranteed.
>
> The initial focus is mostly on guest visible pieces (-device, -machine)
> with '-object' as an easy extra win that's more backend oriented.
>
> In theory it might be extended to other backend types, which are probably
> modelled in QAPI, but are not neccessarily implemented with QOM. So I
> somewhat intentionally left it fuzzy what 'types' we refer to.

If we named it @insecure-fuzz, users would get about the same
information ("it's about *something*") with less risk of confusion ("it
must be about device types, right?").

I'm not actually proposing to name it this way, I'm merely arguing that
the current name is problematic.

>> >  # Since: 6.0
>> >  ##
>> >  { 'struct': 'CompatPolicy',
>> >    'data': { '*deprecated-input': 'CompatPolicyInput',
>> >              '*deprecated-output': 'CompatPolicyOutput',
>> >              '*unstable-input': 'CompatPolicyInput',
>> > -            '*unstable-output': 'CompatPolicyOutput' } }
>> > +            '*unstable-output': 'CompatPolicyOutput',
>> > +            '*insecure-types': 'CompatPolicySecurity' } }
>> 
>> The existing members are all named "wide", and the documentation narrows
>> them.  For instance, the name @deprecated-input could cover *any* input,
>> but its documentation narrows it to "only syntactic aspects of QMP".  I
>> did it this way to let us extend coverage without having to change the
>> external interface.
>> 
>> New member @insecure-types is named "narrow".  Why different?  Why not
>> @insecure-input?
>
> In those existing examples, we're directly refering to data streams,
> whether they're command line / QMP parameters or QMP response, so
> input/output are reasonable terms.

To avoid misunderstandings: @deprecated-input is strictly QMP (all four
policies predating yours are strictly QMP in fact).  For instance,
chardev kind "memory" is deprecated.  Creating one with QMP is caught by
policy:

    $ qemu-system-x86_64 -S -nodefaults -display none -compat deprecated-input=reject -qmp stdio
    {"QMP": {"version": {"qemu": {"micro": 50, "minor": 1, "major": 11}, "package": "v11.1.0-2787-g288458be66-dirty"}, "capabilities": ["oob"]}}
    {"execute": "qmp_capabilities", "arguments": {"enable": ["oob"]}}
    {"return": {}}
    {"execute":"chardev-add", "arguments": {"id":"chr0", "backend": {"type": "memory", "data": {}}}}
    {"error": {"class": "GenericError", "desc": "Deprecated value memory disabled by policy"}}
    {"execute": "quit"}
    {"timestamp": {"seconds": 1793173931, "microseconds": 999776}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
    {"return": {}}

Creating one with CLI is not:

    $ qemu-system-x86_64 -S -nodefaults -display none -compat deprecated-input=reject -chardev memory,id=chr0 -qmp stdio
    {"QMP": {"version": {"qemu": {"micro": 50, "minor": 1, "major": 11}, "package": "v11.1.0-2787-g288458be66-dirty"}, "capabilities": ["oob"]}}
    {"execute": "qmp_capabilities", "arguments": {"enable": ["oob"]}}
    {"return": {}}
    {"execute": "query-chardev"}
    {"return": [{"frontend-open": false, "filename": "memory", "label": "chr0"}, {"frontend-open": true, "filename": "stdio", "label": "compat_monitor0"}]}
    {"execute": "quit"}
    {"timestamp": {"seconds": 1793174007, "microseconds": 270419}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
    {"return": {}}

Works as intended, and matches documentation.  Yes, applying the policy
more widely would be more useful, and applying it everywhere would
likely be most useful, but this better than nothing, and it's what I
could actually implement.  New CompatPolicy members, such as yours, need
not have this limitation.  The may have others.

> In the security status, we specifically focused on types, regardless
> of whether they're requested explicitly via -device (& friends) or
> arrive implicitly via built-in command line defaults and/or hardcoded
> type relationships (ie built devices in a machine type).  I don't
> find 'input' to be a desired term in that respect.
>
> What is being determined is whether the VM as a whole can be classed
> as secure or not.

1. You picked a different limitation for @insecure-types: it covers only
QOM types.  Again, a different limitation is *fine*.  "Only QOM types"
is obviously more useful than "only QOM types and only QMP" would be.

2. I do think the policy is about input.  What else could be accepted
(@accept), accepted with a warning (@warn), or rejected (@reject)?  The
connection from QOM object creation to input can be less obvious than it
is for @deprecated-input and @unstable-input.  But it surely exists,
don't you think?  Consider how a QOM object of type T could be created:

* The user asked for it with -device T,..., -object T,..., or similar.
  Obvious.

* The user asked for it with -machine M, -cpu C, or similar.  Same,
  except we use machine names / CPU names, ... instead of QOM type
  names, mostly for historical reasons, which makes it slightly less
  obvious.

* The user asked for another type that contains T as a component.  This
  can be unobvious, e.g. when a machine contains some obscure onboard
  device the user doesn't know or care about.  It's still the user's
  input that triggers its creation.

* We create T by default, i.e. we can't pin it on any particular piece
  of user input.  It's still triggered by user input, though.

  Example: the user elects not to pass -machine, and the default machine
  contains an insecure onboard device.  The user still asked for that
  machine, if only by not asking for another one.

  Hypothetical example: qemu-system-insecure provides just one machine
  type, which contains an insecure onboard device.  The user still asked
  for that machine, if only by running qemu-system-insecure.

* Did I miss ways to create T?

>> The existing @FOO-input support CompatPolicyInput: "accept", "reject",
>> "crash".  New insecure-types supports CompatPolicySecurity: "accept",
>> "reject", "warn".  Why different?
>
> With deprecations, there is often (but not always) a warning message
> printed unconditioally.

Yes, we do this manually when we remember to.

We do *not* do this automatically for QAPI schema feature @deprecated.
There is no warning then for QMP input.  CLI and HMP wrapping around QMP
warn only if someone added code for it manually.  Rare to never, I'd
guess.  So, @deprecated-input policy @accept does what it says on the
tin: accept silently.  Possibly with rare exceptions.

>                         eg even if you have 'accept' as policy, you
> are liable to see a deprecation warning message on stderr.

Uh, what's the difference between policy @accept and policy @warn then?

>                                                            This is
> good because even if you want to carry on using it for now, and
> never give a -compat flag, actioning the deprecation warning is
> still essential.
>
> With security checks, it is valid to launch QEMU in an insecure
> configuration and use it that way forever, with no action required
> to change anything. As such we must never report any warnings by
> default for insecure types, any warning messages must be strictly
> opt-in.  So "accept" is the default behaviour which is totally
> silent, "warn" lets you know something is bad but does not
> enforce it, and "reject' lets you know something is bad and
> refuses to let it be used. So overall, I think "warn" is justified
> for security checks, while not required for deprecations.

If I understand you correctly, you're arguing for "accept insecure
silently" by default, and for "accept deprecated with a warning" by
default.  Correct?

I feel the sane way to do this would be a choice of accept/warn/reject
in both cases, but with different default, @warn for deprecated, @accept
for insecure.

>> My rationale for "crash" is in commit dbb675c19aa (qapi: New -compat
>> deprecated-input=crash):
>> 
>>     Bugs in integration tests may mask the error from policy "reject".
>>     Provide a larger hammer: crash outright.  Masking that seems unlikely.
>> 
>> If "crash" can be useful for testing a management application's non-use
>> of deprecated and unstable interfaces, surely it's just as useful for
>> insecure ones.
>
> I'm happy to add 'crash' as an extra option.

One-liner, almost :)

>> Likewise, "warn".  I didn't provide "warn" at the time, because QMP
>> lacks support for warnings.

I just remembered another reason.  We generally want to emit a
deprecation warning at most once, to not annoy users.
warn_report_once() makes that easy.  Doesn't work when all the QAPI
deprecation warnings are detected by a single piece of code.  Using
warn_report_once() there would go silent after the first QAPI
deprecation warning, which is not what we'd want.

>>                              Do management applications find warnings on
>> stderr in response to QMP commands useful?
>
> Many deprecations none the less report warnings on stderr - we just
> haven't automated that process from QAPI schema.
>
> I've seen plenty of scenarios where people have paid attention to
> the deprecation warnings in libvirt logs capturing QEMU's stderr.
> It is likely the main way that non-developers find out about the
> deprecations, as few people proactively read the QEMU docs to
> see deprecations, and similarly few use a -compat arg.

I am open to adding policy @warn to deprecated-input and unstable-input,
with deprecated-input's default changing to @warn.

However, I'd much prefer to warn at most once per deprecated thing.  I
can't say offhand how that could be done, though.

> For the security compat arg, my expectation was that distros which
> wish to restrict what their ship & support (ie RHEL) could use
> the -compat insecure-types arg to validate their configs, and
> could even set libvirt in their distro to pass 'warn' (or even
> 'reject') as a safety net against mistakes.
>
> I think it would also be interesting for virt-install (and similar
> tools) to request secure types only by default, at least if KVM
> is used.
>
>> > diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
>> > index 3d849fe0347..38b1cec7a41 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(const CompatPolicy *policy,
>> > +                                  const char *typename,
>> > +                                  bool is_secure,
>> > +                                  Error **errp)
>> 
>> More minor issues.
>> 
>> The policy functions are named compat_policy_FOO_ok().  Why different?
>
> I wasn't paying close enough attention to the naming convention.

Happens :)  Easy enough to tidy up.

>> Their arguments are "relevant facts" (@features), @policy, "stuff for
>> error message" (@error_class, @kind, @name), @errp.  This one has the
>> same things in different order.  Why?
>
> Same lack of close attention as above.
>
> Though I would say I think the 'const CompatPolicy *policy' arg should
> be the first one in all the existing methods, as it is typical convention
> for the first arg type to be associated with the method name.

Fair point.

>> > +{
>> > +    if (is_secure) {
>> > +        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 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();
>> > +    }
>> > +}
>> > +
>> > +
>> >  const char *qapi_enum_lookup(const QEnumLookup *lookup, int val)
>> >  {
>> >      assert(val >= 0 && val < lookup->size);
>> 
>> 
>> [*] So I digress in a footnote.  Sue me ;)
>> 
>> While examining the QAPI schema, I found near object-add:
>> 
>>     ##
>>     # @qom-list-properties:
>>     #     List properties associated with a QOM object.
>>     #
>>     # @typename: the type name of an object
>>     [...]
>>     ##
>>     # @object-add:
>>     #     Create a QOM object.
>>     #
>>     # Errors:
>>     #     - If @qom-type is not a valid class name
>>     [...]
>>     ##
>>     # @ObjectOptions:
>>     #     Describes the options of a user creatable QOM object.
>>     #
>>     # @qom-type: the class name for the object to be created
>> 
>> We use "QOM type" and "QOM class".  This is pretty bad; we should pick
>> one and stick with it.
>
> Probably "type" since there is no concept of a "class" in QAPI,
> it is just a schema for describing types and their relationships.
>
> The 'class' is an internal QOM implementation detail of the
> public QAPI 'type'.

Yes.  And I don't like it much there.

>> And near device_add:
>> 
>>     ##
>>     # @device-list-properties:
>>     #     List properties associated with a device.
>>     #
>>     # @typename: the type name of a device
>>     [...]
>>     ##
>>     # @device_add:
>>     #     Add a device.
>>     #
>>     # @driver: the name of the new device's driver
>> 
>> We use "driver" and "device type".  Similarly bad.
>
> Kind of wish we just used a plain 'type' in both -object and -device,
> but changing that is not worth the disruption. At least the docs
> could refer to 'QOM type' generally.

Makes sense.

Thank you!