Re: [Qemu-devel] [PATCH V2] hw/misc: Add simple measurement hardware

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5673 bytes --]

On 08/05/2016 05:17 PM, Matthew Garrett wrote:

Generally, we recommend that v2 patches be sent as their own top-level
thread, rather than in-reply-to v1, because several tooling scripts get
confused and don't look for deep patches.

> Trusted Boot is based around having a trusted store of measurement data and
> a secure communications channel between that store and an attestation
> target. In actual hardware, that's a TPM. Since the TPM can only be accessed
> via the host system, this in turn requires that the TPM be able to perform
> reasonably complicated cryptographic functions in order to demonstrate its
> trusted state.
> 

> ---
> 
> This should cover the initial review feedback, with the exception of porting
> it to MMIO. It seems easier to keep it in port io space on x86, and I'll add
> MMIO support once it looks like we're happy with this implementation.
> 
>  default-configs/x86_64-softmmu.mak |   1 +
>  hmp-commands-info.hx               |  14 ++
>  hmp.c                              |  13 ++
>  hmp.h                              |   1 +
>  hw/core/loader.c                   |  12 ++
>  hw/i386/acpi-build.c               |  29 +++-
>  hw/misc/Makefile.objs              |   1 +
>  hw/misc/measurements.c             | 295 +++++++++++++++++++++++++++++++++++++
>  hw/misc/measurements.h             |   4 +
>  include/hw/isa/isa.h               |  13 ++
>  include/hw/loader.h                |   1 +
>  monitor.c                          |   1 +
>  qapi-schema.json                   |  26 ++++
>  qmp-commands.hx                    |  20 +++

I'm just focusing on the interface review:


> +++ b/hmp.c
> @@ -2038,6 +2038,19 @@ void hmp_info_iothreads(Monitor *mon, const QDict *qdict)
>      qapi_free_IOThreadInfoList(info_list);
>  }
>  
> +void hmp_info_measurements(Monitor *mon, const QDict *qdict)
> +{
> +    MeasurementList *info_list = qmp_query_measurements(NULL);

Is it really a good idea to ignore errors?


> +
> +MeasurementList *qmp_query_measurements(Error **errp)
> +{
> +    MeasurementList *head = NULL;
> +    MeasurementList **prev = &head;
> +    MeasurementList *elem;
> +    Measurement *info;
> +    Object *obj = object_resolve_path_type("", TYPE_MEASUREMENTS, NULL);
> +    MeasurementState *s;
> +    int pcr, i;
> +
> +    if (!obj) {
> +        return NULL;
> +    }

Returning NULL in a qmp_* function requires that errp be set first.

> +
> +    s = MEASUREMENT(obj);
> +
> +    for (pcr = 0; pcr < PCR_COUNT; pcr++) {
> +        info = g_new0(Measurement, 1);
> +        info->pcr = pcr;
> +        info->hash = g_malloc0(DIGEST_SIZE*2+1);

Spaces around binary operators, please.

> +        for (i = 0; i < DIGEST_SIZE; i++) {
> +            sprintf(info->hash + i * 2, "%02x", s->measurements[pcr][i]);
> +        }
> +        elem = g_new0(MeasurementList, 1);
> +        elem->value = info;
> +        *prev = elem;
> +        prev = &elem->next;
> +    }
> +    return head;
> +}
> +

> +++ b/qapi-schema.json
> @@ -4338,3 +4338,29 @@
>  # Since: 2.7
>  ##
>  { 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }
> +
> +##
> +# @Measurement
> +#
> +# @pcr: The PCR in the measurement

Might be worth spelling out what the acronym PCR means.

> +# @value: The hash value
> +# @vcpus-count: number of logical VCPU threads @HotpluggableCPU provides
> +# @qom-path: #optional link to existing CPU object if CPU is present or
> +#            omitted if CPU is not present.

Bad copy-and-paste. @pcr is correct, @hash is missing, and @value,
@vcpus-count, and @qom-path are wrong.

> +#
> +# Since: 2.7

You've missed 2.7 hard freeze. This is 2.8 material.

> +##
> +{ 'struct': 'Measurement',
> +  'data': { 'pcr': 'int',
> +            'hash': 'str'
> +          }
> +}
> +
> +##
> +# @query-measurements
> +#
> +# Returns: a list of Measurement objects
> +#

A little more detail on what a Measurement object represents would be
worthwhile.  Reading just the .json file gives me no idea why I'd want
to query this, or what I'm really getting as a result.

> +# Since: 2.7

2.8

> +##
> +{ 'command': 'query-measurements', 'returns': ['Measurement'] }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index c8d360a..a13f939 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -5041,3 +5041,23 @@ Example for pc machine type started with
>              "props": {"core-id": 0, "socket-id": 0, "thread-id": 0}
>           }
>         ]}
> +
> +EQMP
> +    {
> +        .name       = "query-measurements",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_query_measurements,


This part conflicts with Marc-Andre's patches to kill qmp-commands.hx as
redundant. Depending on what goes in first, there will be some rebasing
required from the other party.

> +    },
> +SQMP
> +Show system measurements
> +------------------------
> +
> +Arguments: None.
> +
> +Example:
> +
> +-> { "execute": "query-measurements" }
> +<- {"return": [
> +     { "pcr": 0, "hash": "2cfb9f764876a5c7a3a96770fb79043353a5fa38"},
> +     { "pcr": 1, "hash": "30b2c318442bd985ce9394ff649056ffde691617"}
> +     ]}'

> +++ b/stubs/measurements.c
> @@ -0,0 +1,18 @@
> +#include "qemu/osdep.h"
> +#include "hw/misc/measurements.h"
> +#include "qmp-commands.h"
> +
> +MeasurementList *qmp_query_measurements(Error **errp)
> +{
> +    return NULL;

If you return NULL, you should set errp.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]