Re: [PATCH v2 00/32] Encode object type security status in code

[Markus Armbruster <armbru@redhat.com>]

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

> Our docs/system/security.rst file loosely classifies code into that
> applicable for 'virtualization' vs 'non-virtualization' use cases.
> Only code relevant to the former group is eligible for security
> bug handling. Peter's recent proposal pointed out that we are
> increasingly hitting the limits of such a crude classification
>
> Michael suggested that with the increased complexity, docs are not
> going to be an effective way to convey the information, and we
> need to re-consider embedding this info in code.  This also allows
> users to validate a configuration's security status when starting
> a guest, or modifying a running guest.
>
> This series is an attempt to start the embedding process.
>
> Probably I should split in multiple series. One introducing the
> overall framework, and then multiple series doing type annotations,
> as the latter really need to be CC'd to maintainers, but the CC
> list would be way too huge on this combined series. At least this
> combined series shows what the real world implictions of this code
> approach will be though.

I appreciate seeing the entire work.  We can split later if it helps
with review.

> It starts with QOM, adding a "bool secure" property to the
> TypeInfo struct, which get turned into a flag on the Type
> struct. This enables querying any ObjectClass to ask whether or
> not it is declared secure.
>
> By only using a single boolean flag, at runtime we are unable
> to distinguish between "marked insecure" and "no decision,
> implicitly insecure". As such, all our existing code is
> initially considered insecure, except for that which gets
> explicit annotation.
>
> The "-compat" argument gains a new parameter
>
>   * insecure-types=accept|reject|warn
>
>     The default 'accept' preserves historical behaviour of
>     anything being permissible. The other two options both
>     identify use of types that are not explicitly marked
>     as secure.
>
> The code annotations are useful immediately, but to make the
> new -compat switch useful, we need to annotate as much as is
> possible. This series makes a strong attempt to do that across
> a large subset of the codebase. My guidance was to mark enough
> as being 'secure', that a downstream RHEL build of QEMU would
> have explicit anntation of most of its devices, with most being
> secure given they target virtualization use cases.
>
> This annotation is 90% complete for the x86 target, but more
> work is needed to finish it and then address the arch specific
> devices for arm, ppc, s390.
>
> Example: TCG is explicitly insecure, KVM is explicitly secure:

[...]

>  281 files changed, 632 insertions(+), 38 deletions(-)

PATCH 01..13, i.e. just the infrastructure:

 docs/system/security.rst     | 43 +++++++++++++++++++++++++++++++++++++++++++
 qapi/compat.json             | 24 +++++++++++++++++++++++-
 qapi/machine.json            |  8 +++++++-
 qapi/qom.json                | 10 ++++++++--
 include/hw/boards.h          | 12 +++++++++++-
 include/hw/i386/pc.h         | 13 ++++++++++++-
 include/qapi/compat-policy.h |  5 +++++
 include/qom/object.h         | 13 +++++++++++++
 hw/core/machine-qmp-cmds.c   |  1 +
 qapi/qapi-util.c             | 30 ++++++++++++++++++++++++++++++
 qom/object.c                 | 30 +++++++++++++++++++++++-------
 qom/qom-qmp-cmds.c           | 30 ++++++++++++++++++++++++------
 system/qdev-monitor.c        | 12 ++++++++++++
 system/vl.c                  | 35 ++++++++++++++++++++++++++++++-----
 14 files changed, 242 insertions(+), 24 deletions(-)

Quite tractable.

The remainder is purely declarative:

    $ git-diff -U0 3f6db27c42..review | egrep '^[-+][^-+]'| sed 's/  */ /g' | sort -u
    + .abstract = true,
    + .class_init = i2c_ddc_class_init,
    + .instance_init = aw_emac_init,
    + .instance_init = xhci_sysbus_instance_init,
    + .secure = false,
    + .secure = true,
    + .secure = true, \
    + isapc_machine_options);
    + type_info.secure = false,
    + type_info.secure = false;
    + type_info.secure = true,
    + xenfv_machine_3_1_options);
    + xenfv_machine_4_2_options);
    +DEFINE_INSECURE_MACHINE("none", machine_none_machine_init)
    +DEFINE_INSECURE_PC_MACHINE(isapc, "isapc", pc_init_isa,
    +DEFINE_SECURE_MACHINE("xenpv", xenpv_machine_init)
    +DEFINE_SECURE_PC_MACHINE(xenfv, "xenfv-3.1", pc_xen_hvm_init,
    +DEFINE_SECURE_PC_MACHINE(xenfv_4_2, "xenfv-4.2", pc_xen_hvm_init,
    - .abstract = true
    - .class_init = i2c_ddc_class_init
    - .instance_init = aw_emac_init,
    - .instance_init = xhci_sysbus_instance_init
    - isapc_machine_options);
    - xenfv_machine_3_1_options);
    - xenfv_machine_4_2_options);
    -DEFINE_MACHINE("none", machine_none_machine_init)
    -DEFINE_MACHINE("xenpv", xenpv_machine_init)
    -DEFINE_PC_MACHINE(isapc, "isapc", pc_init_isa,
    -DEFINE_PC_MACHINE(xenfv, "xenfv-3.1", pc_xen_hvm_init,
    -DEFINE_PC_MACHINE(xenfv_4_2, "xenfv-4.2", pc_xen_hvm_init,

I like it.