[v4] hw/misc: Add simple measurement hardware

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.

In cloud environments, qemu is inherently trusted and the hypervisor
infrastructure provides a trusted mechanism for extracting information from
qemu and providing it to another system. This means we can skip the crypto
and stick with the basic functionality - ie, providing a trusted store of
measurement data.

This driver provides a very small subset of TPM 1.2 functionality in the
form of a bank of registers that can store SHA1 measurements of boot
components. Performing a write to one of these registers will append the new
20 byte hash to the 20 bytes currently stored within the register, take a
SHA1 of this 40 byte value and then replace the existing register contents
with the new value. This ensures that a given value can only be obtained by
performing the same sequence of writes. It also adds a monitor command to
allow an external agent to extract this information from the running system
and provide it over a secure communications channel. Finally, it measures
each of the loaded ROMs into one of the registers at reset time.

In combination with work in SeaBIOS and the kernel, this permits a fully
measured boot in a virtualised environment without the overhead of a full
TPM implementation.

This version of the implementation depends on port io, but if there's
interest I'll add mmio as well.

Signed-off-by: Matthew Garrett <mjg59@coreos.com>
---

Updated based on David's feedback.

 default-configs/x86_64-softmmu.mak |   1 +
 hmp-commands-info.hx               |  14 ++
 hmp.c                              |  16 ++
 hmp.h                              |   1 +
 hw/core/loader.c                   |  12 ++
 hw/i386/acpi-build.c               |  29 +++-
 hw/misc/Makefile.objs              |   1 +
 hw/misc/measurements.c             | 328 +++++++++++++++++++++++++++++++++++++
 hw/misc/measurements.h             |   5 +
 hw/tpm/tpm_tis.c                   |   5 +
 include/hw/isa/isa.h               |  13 ++
 include/hw/loader.h                |   1 +
 monitor.c                          |   1 +
 qapi-schema.json                   |  30 ++++
 qmp-commands.hx                    |  20 +++
 stubs/Makefile.objs                |   1 +
 stubs/measurements.c               |  20 +++
 17 files changed, 496 insertions(+), 2 deletions(-)
 create mode 100644 hw/misc/measurements.c
 create mode 100644 hw/misc/measurements.h
 create mode 100644 stubs/measurements.c

Matthew Garrett <mjg59@coreos.com> wrote on 08/17/2016 03:48:52 PM:

> From: Matthew Garrett <mjg59@coreos.com>
> To: qemu-devel@nongnu.org
> Cc: dgilbert@redhat.com, berrange@redhat.com, Stefan Berger/Watson/
> IBM@IBMUS, Matthew Garrett <mjg59@coreos.com>
> Date: 08/17/2016 03:49 PM
> Subject: [PATCH v4] hw/misc: Add simple measurement hardware
> 
> 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.
> 
> In cloud environments, qemu is inherently trusted and the hypervisor
> infrastructure provides a trusted mechanism for extracting information 
from
> qemu and providing it to another system. This means we can skip the 
crypto
> and stick with the basic functionality - ie, providing a trusted store 
of
> measurement data.
> 
> This driver provides a very small subset of TPM 1.2 functionality in the
> form of a bank of registers that can store SHA1 measurements of boot
> components. Performing a write to one of these registers will append the 
new
> 20 byte hash to the 20 bytes currently stored within the register, take 
a
> SHA1 of this 40 byte value and then replace the existing register 
contents
> with the new value. This ensures that a given value can only be obtained 
by
> performing the same sequence of writes. It also adds a monitor command 
to
> allow an external agent to extract this information from the running 
system
> and provide it over a secure communications channel. Finally, it 
measures
> each of the loaded ROMs into one of the registers at reset time.
> 
> In combination with work in SeaBIOS and the kernel, this permits a fully
> measured boot in a virtualised environment without the overhead of a 
full
> TPM implementation.
> 
> This version of the implementation depends on port io, but if there's
> interest I'll add mmio as well.
> 
> Signed-off-by: Matthew Garrett <mjg59@coreos.com>
> ---
> 
> Updated based on David's feedback.
> 
>  default-configs/x86_64-softmmu.mak |   1 +
>  hmp-commands-info.hx               |  14 ++
>  hmp.c                              |  16 ++
>  hmp.h                              |   1 +
>  hw/core/loader.c                   |  12 ++
>  hw/i386/acpi-build.c               |  29 +++-
>  hw/misc/Makefile.objs              |   1 +
>  hw/misc/measurements.c             | 328 ++++++++++++++++++++++++++
> +++++++++++
>  hw/misc/measurements.h             |   5 +
>  hw/tpm/tpm_tis.c                   |   5 +


There shouldn't be a change to tpm_tis.c since this is just one specific 
front end of possibly different one. I think the mutual exclusion test 
should go into more common code: tpm.c:tpm_init()

* Matthew Garrett (mjg59@coreos.com) wrote:
> 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.
> 
> In cloud environments, qemu is inherently trusted and the hypervisor
> infrastructure provides a trusted mechanism for extracting information from
> qemu and providing it to another system. This means we can skip the crypto
> and stick with the basic functionality - ie, providing a trusted store of
> measurement data.
> 
> This driver provides a very small subset of TPM 1.2 functionality in the
> form of a bank of registers that can store SHA1 measurements of boot
> components. Performing a write to one of these registers will append the new
> 20 byte hash to the 20 bytes currently stored within the register, take a
> SHA1 of this 40 byte value and then replace the existing register contents
> with the new value. This ensures that a given value can only be obtained by
> performing the same sequence of writes. It also adds a monitor command to
> allow an external agent to extract this information from the running system
> and provide it over a secure communications channel. Finally, it measures
> each of the loaded ROMs into one of the registers at reset time.
> 
> In combination with work in SeaBIOS and the kernel, this permits a fully
> measured boot in a virtualised environment without the overhead of a full
> TPM implementation.
> 
> This version of the implementation depends on port io, but if there's
> interest I'll add mmio as well.

Other than a couple of nits I'll mention below (and Stefan's comment)
I don't see why we shouldn't have this; although we'll need a full vTPM
for many uses, this is small and self-contained enough I can't
see why not to have it.  I'd also be tempted to prefix the commands in
both qmp and hmp by an x-  (i.e. mark experimental) so that you have the
freedom to change them after it goes in initially 

Paolo: Does the acpi stuff make sense?

Dave

> 
> Signed-off-by: Matthew Garrett <mjg59@coreos.com>
> ---
> 
> Updated based on David's feedback.
> 
>  default-configs/x86_64-softmmu.mak |   1 +
>  hmp-commands-info.hx               |  14 ++
>  hmp.c                              |  16 ++
>  hmp.h                              |   1 +
>  hw/core/loader.c                   |  12 ++
>  hw/i386/acpi-build.c               |  29 +++-
>  hw/misc/Makefile.objs              |   1 +
>  hw/misc/measurements.c             | 328 +++++++++++++++++++++++++++++++++++++
>  hw/misc/measurements.h             |   5 +
>  hw/tpm/tpm_tis.c                   |   5 +
>  include/hw/isa/isa.h               |  13 ++
>  include/hw/loader.h                |   1 +
>  monitor.c                          |   1 +
>  qapi-schema.json                   |  30 ++++
>  qmp-commands.hx                    |  20 +++
>  stubs/Makefile.objs                |   1 +
>  stubs/measurements.c               |  20 +++
>  17 files changed, 496 insertions(+), 2 deletions(-)
>  create mode 100644 hw/misc/measurements.c
>  create mode 100644 hw/misc/measurements.h
>  create mode 100644 stubs/measurements.c
> 
> diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
> index 6e3b312..6f0fcc3 100644
> --- a/default-configs/x86_64-softmmu.mak
> +++ b/default-configs/x86_64-softmmu.mak
> @@ -58,3 +58,4 @@ CONFIG_IOH3420=y
>  CONFIG_I82801B11=y
>  CONFIG_SMBIOS=y
>  CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
> +CONFIG_MEASUREMENTS=y
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index 74446c6..bf1cf67 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -816,6 +816,20 @@ STEXI
>  Show information about hotpluggable CPUs
>  ETEXI
>  
> +    {
> +        .name       = "measurements",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "show PCR measurements",
> +        .mhandler.cmd = hmp_info_measurements,
> +    },
> +
> +STEXI
> +@item info measurements
> +@findex measurements
> +Show PCR measurements
> +ETEXI
> +
>  STEXI
>  @end table
>  ETEXI
> diff --git a/hmp.c b/hmp.c
> index cc2056e..462e0c3 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2038,6 +2038,22 @@ void hmp_info_iothreads(Monitor *mon, const QDict *qdict)
>      qapi_free_IOThreadInfoList(info_list);
>  }
>  
> +void hmp_info_measurements(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +    MeasurementList *info_list = qmp_query_measurements(&err);
> +    MeasurementList *info;
> +
> +    if (err == NULL) {
> +        for (info = info_list; info; info = info->next) {
> +            monitor_printf(mon, "%02" PRId64 ":%s\n", info->value->pcr,
> +                           info->value->hash);
> +        }
> +        qapi_free_MeasurementList(info_list);
> +    }
> +    hmp_handle_error(mon, &err);
> +}
> +
>  void hmp_qom_list(Monitor *mon, const QDict *qdict)
>  {
>      const char *path = qdict_get_try_str(qdict, "path");
> diff --git a/hmp.h b/hmp.h
> index 0876ec0..6afb1d9 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -40,6 +40,7 @@ void hmp_info_pci(Monitor *mon, const QDict *qdict);
>  void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
>  void hmp_info_tpm(Monitor *mon, const QDict *qdict);
>  void hmp_info_iothreads(Monitor *mon, const QDict *qdict);
> +void hmp_info_measurements(Monitor *mon, const QDict *qdict);
>  void hmp_quit(Monitor *mon, const QDict *qdict);
>  void hmp_stop(Monitor *mon, const QDict *qdict);
>  void hmp_system_reset(Monitor *mon, const QDict *qdict);
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 53e0e41..d7ed110 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -55,6 +55,7 @@
>  #include "exec/address-spaces.h"
>  #include "hw/boards.h"
>  #include "qemu/cutils.h"
> +#include "hw/misc/measurements.h"
>  
>  #include <zlib.h>
>  
> @@ -1026,6 +1027,17 @@ static void rom_reset(void *unused)
>      }
>  }
>  
> +void measure_roms(void)
> +{
> +    Rom *rom;
> +    QTAILQ_FOREACH(rom, &roms, next) {
> +        if (rom->data == NULL) {
> +            continue;
> +        }
> +        measurements_extend_data(0, rom->data, rom->datasize, rom->name);
> +    }
> +}
> +
>  int rom_check_and_register_reset(void)
>  {
>      hwaddr addr = 0;
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index a26a4bb..c9b5f12 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -34,6 +34,7 @@
>  #include "hw/acpi/acpi-defs.h"
>  #include "hw/acpi/acpi.h"
>  #include "hw/acpi/cpu.h"
> +#include "hw/misc/measurements.h"
>  #include "hw/nvram/fw_cfg.h"
>  #include "hw/acpi/bios-linker-loader.h"
>  #include "hw/loader.h"
> @@ -115,6 +116,7 @@ typedef struct AcpiMiscInfo {
>      unsigned dsdt_size;
>      uint16_t pvpanic_port;
>      uint16_t applesmc_io_base;
> +    uint16_t measurements_io_base;
>  } AcpiMiscInfo;
>  
>  typedef struct AcpiBuildPciBusHotplugState {
> @@ -211,6 +213,7 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
>      info->tpm_version = tpm_get_version();
>      info->pvpanic_port = pvpanic_port();
>      info->applesmc_io_base = applesmc_port();
> +    info->measurements_io_base = measurements_port();
>  }
>  
>  /*
> @@ -2282,6 +2285,26 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>          aml_append(dsdt, scope);
>      }
>  
> +    if (misc->measurements_io_base) {
> +        scope = aml_scope("\\_SB.PCI0.ISA");
> +        dev = aml_device("PCRS");
> +
> +        aml_append(dev, aml_name_decl("_HID", aml_string("CORE0001")));
> +        /* device present, functioning, decoding, not shown in UI */
> +        aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
> +
> +        crs = aml_resource_template();
> +        aml_append(crs,
> +               aml_io(AML_DECODE16, misc->measurements_io_base,
> +                      misc->measurements_io_base,
> +                      0x01, 2)
> +        );
> +        aml_append(dev, aml_name_decl("_CRS", crs));
> +
> +        aml_append(scope, dev);
> +        aml_append(dsdt, scope);
> +    }
> +
>      sb_scope = aml_scope("\\_SB");
>      {
>          build_memory_devices(sb_scope, nr_mem, pm->mem_hp_io_base,
> @@ -2689,11 +2712,13 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>          acpi_add_table(table_offsets, tables_blob);
>          build_hpet(tables_blob, tables->linker);
>      }
> -    if (misc.tpm_version != TPM_VERSION_UNSPEC) {
> +    if (misc.tpm_version != TPM_VERSION_UNSPEC || misc.measurements_io_base) {
>          acpi_add_table(table_offsets, tables_blob);
>          build_tpm_tcpa(tables_blob, tables->linker, tables->tcpalog);
>  
> -        if (misc.tpm_version == TPM_VERSION_2_0) {
> +        if (misc.measurements_io_base) {
> +            measurements_set_log(tables->tcpalog->data);
> +        } else if (misc.tpm_version == TPM_VERSION_2_0) {
>              acpi_add_table(table_offsets, tables_blob);
>              build_tpm2(tables_blob, tables->linker);
>          }
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index 4cfbd10..b76e60c 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -5,6 +5,7 @@ common-obj-$(CONFIG_ISA_DEBUG) += debugexit.o
>  common-obj-$(CONFIG_SGA) += sga.o
>  common-obj-$(CONFIG_ISA_TESTDEV) += pc-testdev.o
>  common-obj-$(CONFIG_PCI_TESTDEV) += pci-testdev.o
> +common-obj-$(CONFIG_MEASUREMENTS) += measurements.o
>  
>  obj-$(CONFIG_VMPORT) += vmport.o
>  
> diff --git a/hw/misc/measurements.c b/hw/misc/measurements.c
> new file mode 100644
> index 0000000..2274342
> --- /dev/null
> +++ b/hw/misc/measurements.c
> @@ -0,0 +1,328 @@
> +/*
> + * QEMU boot measurement
> + *
> + * Copyright (c) 2016 CoreOS, Inc <mjg59@coreos.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> + * "Software"), to deal in the Software without restriction, including
> + * without limitation the rights to use, copy, modify, merge, publish,
> + * distribute, sublicense, and/or sell copies of the Software, and to permit
> + * persons to whom the Software is furnished to do so, subject to the
> + * following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included
> + * in all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> + * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
> + * NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
> + * USE OR OTHER DEALINGS IN THE SOFTWARE.
> + */
> +#include "qemu/osdep.h"
> +#include "crypto/hash.h"
> +#include "monitor/monitor.h"
> +#include "hw/loader.h"
> +#include "hw/acpi/tpm.h"
> +#include "hw/isa/isa.h"
> +#include "hw/misc/measurements.h"
> +#include "qmp-commands.h"
> +#include "sysemu/tpm.h"
> +
> +#define EV_POST_CODE 1 /* Code measured during POST */
> +
> +#define MEASUREMENT(obj) OBJECT_CHECK(MeasurementState, (obj), \
> +                                      TYPE_MEASUREMENTS)
> +
> +#define MEASUREMENT_DIGEST_SIZE 20
> +#define MEASUREMENT_PCR_COUNT 24
> +
> +typedef struct MeasurementState MeasurementState;
> +
> +struct MeasurementState {
> +    ISADevice parent_obj;
> +    MemoryRegion io_select;
> +    MemoryRegion io_value;
> +    uint16_t iobase;
> +    uint8_t measurements[MEASUREMENT_PCR_COUNT][MEASUREMENT_DIGEST_SIZE];
> +    uint8_t tmpmeasurement[MEASUREMENT_DIGEST_SIZE];
> +    uint32_t write_count;
> +    uint32_t read_count;
> +    uint8_t pcr;
> +    uint32_t logsize;
> +    struct tpm_event *log;
> +};
> +
> +struct tpm_event {
> +    uint32_t pcrindex;
> +    uint32_t eventtype;
> +    uint8_t digest[MEASUREMENT_DIGEST_SIZE];
> +    uint32_t eventdatasize;
> +    uint8_t event[0];
> +};
> +
> +static Object *measurement_dev_find(void)
> +{
> +    return object_resolve_path_type("", TYPE_MEASUREMENTS, NULL);
> +}
> +
> +static void measurement_reset(DeviceState *dev)
> +{
> +    MeasurementState *s = MEASUREMENT(dev);
> +
> +    s->read_count = 0;
> +    s->write_count = 0;
> +    s->logsize = 0;
> +    memset(s->measurements, 0, sizeof(s->measurements));
> +    measure_roms();
> +}
> +
> +static void measurement_select(void *opaque, hwaddr addr, uint64_t val,
> +                               unsigned size)
> +{
> +    MeasurementState *s = MEASUREMENT(opaque);
> +
> +    if (val >= MEASUREMENT_PCR_COUNT) {
> +        return;
> +    }
> +
> +    s->pcr = val;
> +    s->read_count = 0;
> +    s->write_count = 0;
> +}
> +
> +static uint64_t measurement_version(void *opaque, hwaddr addr, unsigned size)
> +{
> +    return 0;
> +}
> +
> +static uint64_t measurement_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    MeasurementState *s = MEASUREMENT(opaque);
> +
> +    if (s->read_count == MEASUREMENT_DIGEST_SIZE) {
> +        s->read_count = 0;
> +    }
> +    return s->measurements[s->pcr][s->read_count++];
> +}
> +
> +static void extend(MeasurementState *s, uint8_t pcrnum, uint8_t *data)
> +{
> +    Error *err;

Doesn't that need to be err = NULL    ?

> +    uint8_t tmpbuf[2 * MEASUREMENT_DIGEST_SIZE];
> +    size_t resultlen = 0;
> +    uint8_t *result = NULL;
> +
> +    memcpy(tmpbuf, s->measurements[pcrnum], MEASUREMENT_DIGEST_SIZE);
> +    memcpy(tmpbuf + MEASUREMENT_DIGEST_SIZE, data, MEASUREMENT_DIGEST_SIZE);
> +    if (qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA1, (const char *)tmpbuf,
> +                           2 * MEASUREMENT_DIGEST_SIZE, &result, &resultlen,
> +                           &err) == 0) {
> +        memcpy(s->measurements[pcrnum], result, MEASUREMENT_DIGEST_SIZE);
> +    } else {
> +        error_reportf_err(err, "Failed to measure data:");
> +    }
> +
> +    g_free(result);
> +}
> +
> +static void measurement_value(void *opaque, hwaddr addr, uint64_t val,
> +                              unsigned size)
> +{
> +    MeasurementState *s = opaque;
> +
> +    s->tmpmeasurement[s->write_count++] = val;
> +    if (s->write_count == MEASUREMENT_DIGEST_SIZE) {
> +        extend(s, s->pcr, s->tmpmeasurement);
> +        s->write_count = 0;
> +    }
> +}
> +
> +static void log_data(MeasurementState *s, uint8_t pcrnum, uint8_t *hash,
> +                     char *description)
> +{
> +    int eventlen = strlen(description);
> +    int entrylen = eventlen + sizeof(struct tpm_event);
> +    struct tpm_event *logentry;
> +
> +    if (!s->log) {
> +        return;
> +    }
> +
> +    if (s->logsize + entrylen > TPM_LOG_AREA_MINIMUM_SIZE) {
> +        fprintf(stderr, "Measurement log entry would overflow log - dropping");

error_report please.

> +        return;
> +    }
> +    logentry = (struct tpm_event *)(((void *)s->log) + s->logsize);
> +    logentry->pcrindex = pcrnum;
> +    logentry->eventtype = EV_POST_CODE;
> +    memcpy(logentry->digest, hash, MEASUREMENT_DIGEST_SIZE);
> +    logentry->eventdatasize = eventlen;
> +    memcpy(logentry->event, description, eventlen);
> +
> +    s->logsize += entrylen;
> +}
> +
> +void measurements_extend_data(uint8_t pcrnum, uint8_t *data, size_t len,
> +                              char *description)
> +{
> +    int ret;
> +    Error *err;
> +    uint8_t *result;
> +    size_t resultlen = 0;
> +    Object *obj = object_resolve_path_type("", TYPE_MEASUREMENTS, NULL);
> +
> +    if (!obj) {
> +        return;
> +    }
> +
> +    ret = qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA1, (char *)data, len, &result,
> +                             &resultlen, &err);
> +    if (ret < 0) {
> +        error_reportf_err(err, "Failed to hash extension data");
> +    }
> +
> +    extend(MEASUREMENT(obj), pcrnum, result);
> +    log_data(MEASUREMENT(obj), pcrnum, result, description);
> +    g_free(result);
> +}
> +
> +static const MemoryRegionOps measurement_select_ops = {
> +    .write = measurement_select,
> +    .read = measurement_version,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +};
> +
> +static const MemoryRegionOps measurement_value_ops = {
> +    .write = measurement_value,
> +    .read = measurement_read,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +};
> +
> +static void measurement_realize(DeviceState *dev, Error **errp)
> +{
> +    MeasurementState *s = MEASUREMENT(dev);
> +
> +    if (tpm_get_version() != TPM_VERSION_UNSPEC) {
> +        error_setg(errp, "Can't use measurements and TPM simultaneously");
> +        return;
> +    }
> +    memory_region_init_io(&s->io_select, OBJECT(s), &measurement_select_ops,
> +                          s, "measurement-select", 1);
> +    isa_register_ioport(&s->parent_obj, &s->io_select, s->iobase);
> +    memory_region_init_io(&s->io_value, OBJECT(s), &measurement_value_ops, s,
> +                          "measurement-value", 1);
> +    isa_register_ioport(&s->parent_obj, &s->io_value, s->iobase + 1);
> +    measurement_reset(dev);
> +}
> +
> +static Property measurement_props[] = {
> +    DEFINE_PROP_UINT16(MEASUREMENTS_PROP_IO_BASE, MeasurementState, iobase,
> +                       0x620),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static int measurement_state_post_load(void *opaque, int version_id)
> +{
> +    MeasurementState *s = opaque;
> +
> +    if (s->write_count > MEASUREMENT_DIGEST_SIZE ||

There's an off-by-1 on that;  in the places you increment write_count
above, you do a write, increment and then check if it hit the size,
so it should be >= for write_count. 

> +        s->read_count > MEASUREMENT_DIGEST_SIZE ||

That's OK I think; you compare, wrap then read in the code above.

> +        s->pcr > MEASUREMENT_PCR_COUNT) {

Also I think that should be >= since it's an array index.

> +        fprintf(stderr, "Invalid measurement state on reload\n");

error_report

> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
> +static const VMStateDescription measurement_state = {
> +    .name = "measurements",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .post_load = measurement_state_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8_2DARRAY(measurements, MeasurementState,
> +                              MEASUREMENT_PCR_COUNT, MEASUREMENT_DIGEST_SIZE),
> +        VMSTATE_BUFFER(tmpmeasurement, MeasurementState),
> +        VMSTATE_UINT32(write_count, MeasurementState),
> +        VMSTATE_UINT32(read_count, MeasurementState),
> +        VMSTATE_UINT8(pcr, MeasurementState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void measurement_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    dc->realize = measurement_realize;
> +    dc->reset = measurement_reset;
> +    dc->props = measurement_props;
> +    dc->vmsd = &measurement_state;
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +}
> +
> +static const TypeInfo measurement = {
> +    .name          = TYPE_MEASUREMENTS,
> +    .parent        = TYPE_ISA_DEVICE,
> +    .instance_size = sizeof(MeasurementState),
> +    .class_init    = measurement_class_init,
> +};
> +
> +static void measurement_register_types(void)
> +{
> +    type_register_static(&measurement);
> +}
> +
> +type_init(measurement_register_types);
> +
> +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) {
> +        error_setg(errp, "Unable to locate measurement object");
> +        return NULL;
> +    }
> +
> +    s = MEASUREMENT(obj);
> +
> +    for (pcr = 0; pcr < MEASUREMENT_PCR_COUNT; pcr++) {
> +        info = g_new0(Measurement, 1);
> +        info->pcr = pcr;
> +        info->hash = g_malloc0(MEASUREMENT_DIGEST_SIZE * 2 + 1);
> +        for (i = 0; i < MEASUREMENT_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;
> +}
> +
> +void measurements_set_log(gchar *log)
> +{
> +    Object *obj = measurement_dev_find();
> +    MeasurementState *s = MEASUREMENT(obj);
> +
> +    s->log = (struct tpm_event *)log;
> +}
> diff --git a/hw/misc/measurements.h b/hw/misc/measurements.h
> new file mode 100644
> index 0000000..4a26aab
> --- /dev/null
> +++ b/hw/misc/measurements.h
> @@ -0,0 +1,5 @@
> +#include "hw/sysbus.h"
> +
> +void measurements_extend_data(uint8_t pcrnum, uint8_t *data, size_t len,
> +                              char *description);
> +void measurements_set_log(gchar *log);
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index 381e726..95e69c0 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -1036,6 +1036,11 @@ static void tpm_tis_realizefn(DeviceState *dev, Error **errp)
>      TPMState *s = TPM(dev);
>      TPMTISEmuState *tis = &s->s.tis;
>  
> +    if (measurements_port()) {
> +        error_setg(errp, "Can't use measurements and TPM simultaneously");
> +        return;
> +    }
> +
>      s->be_driver = qemu_find_tpm(s->backend);
>      if (!s->be_driver) {
>          error_setg(errp, "tpm_tis: backend driver with id %s could not be "
> diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
> index 7693ac5..55e5472 100644
> --- a/include/hw/isa/isa.h
> +++ b/include/hw/isa/isa.h
> @@ -24,6 +24,9 @@
>  #define APPLESMC_MAX_DATA_LENGTH       32
>  #define APPLESMC_PROP_IO_BASE "iobase"
>  
> +#define TYPE_MEASUREMENTS "measurements"
> +#define MEASUREMENTS_PROP_IO_BASE "iobase"
> +
>  static inline uint16_t applesmc_port(void)
>  {
>      Object *obj = object_resolve_path_type("", TYPE_APPLE_SMC, NULL);
> @@ -34,6 +37,16 @@ static inline uint16_t applesmc_port(void)
>      return 0;
>  }
>  
> +static inline uint16_t measurements_port(void)
> +{
> +    Object *obj = object_resolve_path_type("", TYPE_MEASUREMENTS, NULL);
> +
> +    if (obj) {
> +        return object_property_get_int(obj, MEASUREMENTS_PROP_IO_BASE, NULL);
> +    }
> +    return 0;
> +}
> +
>  #define TYPE_ISADMA "isa-dma"
>  
>  #define ISADMA_CLASS(klass) \
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 4879b63..cc3157d 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -133,6 +133,7 @@ void rom_reset_order_override(void);
>  int rom_copy(uint8_t *dest, hwaddr addr, size_t size);
>  void *rom_ptr(hwaddr addr);
>  void hmp_info_roms(Monitor *mon, const QDict *qdict);
> +void measure_roms(void);
>  
>  #define rom_add_file_fixed(_f, _a, _i)          \
>      rom_add_file(_f, NULL, _a, _i, false, NULL)
> diff --git a/monitor.c b/monitor.c
> index 5c00373..e8858c8 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -32,6 +32,7 @@
>  #include "hw/pci/pci.h"
>  #include "sysemu/watchdog.h"
>  #include "hw/loader.h"
> +#include "hw/misc/measurements.h"
>  #include "exec/gdbstub.h"
>  #include "net/net.h"
>  #include "net/slirp.h"
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 5658723..145d723 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4338,3 +4338,33 @@
>  # Since: 2.7
>  ##
>  { 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }
> +
> +##
> +# @Measurement
> +#
> +# @pcr: The platform configuration register in the measurement
> +# @hash: The SHA1 value stored inside the given platform configuration register
> +#
> +# Since: 2.8
> +##
> +{ 'struct': 'Measurement',
> +  'data': { 'pcr': 'int',
> +            'hash': 'str'
> +          }
> +}
> +
> +##
> +# @query-measurements
> +#
> +# Various components of the boot process (including ROMs) will be
> +# cryptographically hashed and stored in platform configuration registers. Any
> +# modification of these boot components will result in changes to these values,
> +# making it possible to verify that the system was booted in the expected
> +# configuration. This command will return the values.
> +#
> +# Returns: a list of Measurement objects
> +#
> +#
> +# Since: 2.8
> +##
> +{ 'command': 'query-measurements', 'returns': ['Measurement'] }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 6866264..f236b6b 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -5039,3 +5039,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,
> +    },
> +SQMP
> +Show system measurements
> +------------------------
> +
> +Arguments: None.
> +
> +Example:
> +
> +-> { "execute": "query-measurements" }
> +<- {"return": [
> +     { "pcr": 0, "hash": "2cfb9f764876a5c7a3a96770fb79043353a5fa38"},
> +     { "pcr": 1, "hash": "30b2c318442bd985ce9394ff649056ffde691617"}
> +     ]}'
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 55edd15..0ec2f7b 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -45,3 +45,4 @@ stub-obj-y += iohandler.o
>  stub-obj-y += smbios_type_38.o
>  stub-obj-y += ipmi.o
>  stub-obj-y += pc_madt_cpu_entry.o
> +stub-obj-y += measurements.o
> diff --git a/stubs/measurements.c b/stubs/measurements.c
> new file mode 100644
> index 0000000..edd363b
> --- /dev/null
> +++ b/stubs/measurements.c
> @@ -0,0 +1,20 @@
> +#include "qemu/osdep.h"
> +#include "hw/misc/measurements.h"
> +#include "qmp-commands.h"
> +
> +MeasurementList *qmp_query_measurements(Error **errp)
> +{
> +    error_setg(errp, "No support for measurements");
> +    return NULL;
> +}
> +
> +void measurements_extend_data(uint8_t pcrnum, uint8_t *data, size_t len,
> +                              char *description)
> +{
> +    return;
> +}
> +
> +void measurements_set_log(gchar *log)
> +{
> +    return;
> +}
> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On 30/09/2016 12:45, Dr. David Alan Gilbert wrote:
>> > 
>> > This version of the implementation depends on port io, but if there's
>> > interest I'll add mmio as well.
> Other than a couple of nits I'll mention below (and Stefan's comment)
> I don't see why we shouldn't have this; although we'll need a full vTPM
> for many uses, this is small and self-contained enough I can't
> see why not to have it.  I'd also be tempted to prefix the commands in
> both qmp and hmp by an x-  (i.e. mark experimental) so that you have the
> freedom to change them after it goes in initially 
> 
> Paolo: Does the acpi stuff make sense?

I think I'd prefer to use a QEMUxxxx identifier if it's not a problem
for Matthew, but that's really all I have to complain about it.

Paolo