From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51791) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bVrjo-0002X9-7A for qemu-devel@nongnu.org; Fri, 05 Aug 2016 22:53:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bVrjj-0003lx-W9 for qemu-devel@nongnu.org; Fri, 05 Aug 2016 22:53:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52870) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bVrjj-0003lm-Ni for qemu-devel@nongnu.org; Fri, 05 Aug 2016 22:53:19 -0400 References: <1466716148-10655-1-git-send-email-mjg59@coreos.com> <1470439032-20995-1-git-send-email-mjg59@coreos.com> From: Eric Blake Message-ID: <57A5511D.5070604@redhat.com> Date: Fri, 5 Aug 2016 20:53:17 -0600 MIME-Version: 1.0 In-Reply-To: <1470439032-20995-1-git-send-email-mjg59@coreos.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="9lJOTQOjheAdNt8li71Maxrip82pkdsCP" Subject: Re: [Qemu-devel] [PATCH V2] hw/misc: Add simple measurement hardware List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Matthew Garrett , qemu-devel@nongnu.org Cc: stefanb@us.ibm.com, dgilbert@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --9lJOTQOjheAdNt8li71Maxrip82pkdsCP Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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 acc= essed > via the host system, this in turn requires that the TPM be able to perf= orm > reasonably complicated cryptographic functions in order to demonstrate = its > trusted state. >=20 > --- >=20 > This should cover the initial review feedback, with the exception of po= rting > it to MMIO. It seems easier to keep it in port io space on x86, and I'l= l add > MMIO support once it looks like we're happy with this implementation. >=20 > 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 QDic= t *qdict) > qapi_free_IOThreadInfoList(info_list); > } > =20 > +void hmp_info_measurements(Monitor *mon, const QDict *qdict) > +{ > + MeasurementList *info_list =3D qmp_query_measurements(NULL); Is it really a good idea to ignore errors? > + > +MeasurementList *qmp_query_measurements(Error **errp) > +{ > + MeasurementList *head =3D NULL; > + MeasurementList **prev =3D &head; > + MeasurementList *elem; > + Measurement *info; > + Object *obj =3D object_resolve_path_type("", TYPE_MEASUREMENTS, NU= LL); > + MeasurementState *s; > + int pcr, i; > + > + if (!obj) { > + return NULL; > + } Returning NULL in a qmp_* function requires that errp be set first. > + > + s =3D MEASUREMENT(obj); > + > + for (pcr =3D 0; pcr < PCR_COUNT; pcr++) { > + info =3D g_new0(Measurement, 1); > + info->pcr =3D pcr; > + info->hash =3D g_malloc0(DIGEST_SIZE*2+1); Spaces around binary operators, please. > + for (i =3D 0; i < DIGEST_SIZE; i++) { > + sprintf(info->hash + i * 2, "%02x", s->measurements[pcr][i= ]); > + } > + elem =3D g_new0(MeasurementList, 1); > + elem->value =3D info; > + *prev =3D elem; > + prev =3D &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 provid= es > +# @qom-path: #optional link to existing CPU object if CPU is present o= r > +# 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 =3D "query-measurements", > + .args_type =3D "", > + .mhandler.cmd_new =3D 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. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --9lJOTQOjheAdNt8li71Maxrip82pkdsCP Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJXpVEdAAoJEKeha0olJ0NqlegH/Rb9N3JIHe3/7Wa/JW6S8upy VtLtiilpWI5LI06MuiIYezk3xJCfHUs7vMFGiW9pSs4fibUo5MJ8BjGtPtdogKUo FBWUlAXHkxMK/EUVLaajUyb4yL9piClR+ANFI6yX/+nbfd6vvHZv3QYAVlmUZxTN JTortrLsLEkepKGuQHrV2XGQQuDghhREIHqY4aDhI1mjjXrGsSRfM8tz1l0FxnTe 0LkGPH07/P3mmIFOYzLPjfh95tZVhxijtZRc6Hb26gdMQ6aspOb5xAXS0ck7fD+1 jUfPUvasUwevMzhMiEOZF8Jn8To1MET/4YA7UP2aYoTSHoONtO4n1abA06zE6EI= =WFAh -----END PGP SIGNATURE----- --9lJOTQOjheAdNt8li71Maxrip82pkdsCP--