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
signature.asc
Description: OpenPGP digital signature
