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

[Dr. David Alan Gilbert]

* Matthew Garrett (address@hidden) wrote:

Hi Matthew,
  (Ccing in Stefan who has been trying to get vTPM in for years and
   Paolo for any x86ism and especially the ACPIisms, and Daniel for crypto 
stuff)

I'll repeat some of my comments from yesterday's irc chat so you can reply on 
list.

So overall the plus point is it's simple (much smaller than even the interface
to the vTPM), the minus is it's very non-standard.

> 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.

I think the big question for me is what uses this system and in particular how 
the users
can guarantee who they're speaking to; I'd like to understand the cases it works
for and those it doesn't;  for example:

   a) (one that works) 'are all the VMs on my hosts running trusted OSs'
      That works with this just as well as with a vTPM; you ask your hypervisor 
to
      give you the measurements for your guests; you trust your hypervisor.
      Although I think you've somehow got to extract the measurement log from 
the
      guest and get it to the hypervisor if it's going to make sense of the
      measurements.

   b) (one that doesn't?) I'm connecting to a VM remotely over a network, I want
      to check the VM really is who it says it is and is running a trusted OS.
      As a remote entity I don't know which hypervisor is running the VM, the VM
      itself can't sign anything to give me back because it might just sign a 
reply
      for a different VM.   On a real TPM the attestation results would be 
signed
      using one of the keys in the TPM (I can't remember which).

   c) (similar to b) 'I paid you to give me a ... VM - can I check it really is 
that'
      how do I externally to the cloud show that the measurement came from the 
same VM
      I'm asking about.

and then I'm not clear which of the existing TPM users could be munged into 
working
with it; can you make an existing trusted-grub or trousers write measurements 
and log
into it?

> 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.

So only SeaBIOS for now? Would it work for EFI in principle?

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

I guess port IO has the advantage of making it easy to glue into the early 
parts of the BIOS.

Some things I can see are missing:
   Migration support: You probably want to migrate the current measurements and
                      make sure you don't include the measurements of ROMs on 
the
                      destination until after reset.
                      (Search for places that use dc->vmsd = .... as examples)
                      You might want to add a measurement to indicate a 
migration
                      happened; but that's a separate question.

   QMP support: You should wire it up to the qmp monitor as well.
                Generally the management tools use qmp rather than hmp,

   What about hotplug? If I hotplug a NIC should it measure the new ROM?
                What happens then if I restart the VM from clean with
                the ROM added.

 Why do SHA1 based TPM 1.2 now, shouldn't you start with TPM2/newer SHAs?

 I guess another approach to the same thing would be to bundle this up into
something that responded to TPM commands like a vTPM but just had less
inside it; then it could attach to the existing TPM interfaces?

> Signed-off-by: Matthew Garrett <address@hidden>
> ---
>  default-configs/x86_64-softmmu.mak |   1 +
>  hmp-commands.hx                    |  13 +++
>  hw/core/loader.c                   |  12 +++
>  hw/i386/acpi-build.c               |  22 ++++
>  hw/misc/Makefile.objs              |   1 +
>  hw/misc/measurements.c             | 206 
> +++++++++++++++++++++++++++++++++++++
>  hw/misc/measurements.h             |   2 +
>  include/hw/isa/isa.h               |  13 +++
>  include/hw/loader.h                |   1 +
>  monitor.c                          |   1 +
>  stubs/Makefile.objs                |   1 +
>  stubs/measurements.c               |  13 +++
>  12 files changed, 286 insertions(+)
>  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.hx b/hmp-commands.hx
> index 98b4b1a..6a31392 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1748,6 +1748,19 @@ Set QOM property @var{property} of object at location 
> @var{path} to value @var{v
>  ETEXI
>  
>      {
> +        .name       = "measurements",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "Print system measurements",
> +        .mhandler.cmd = print_measurements,
> +    },
> +STEXI
> address@hidden measurements
> address@hidden measurements
> +Redirect Print system measurements
> +ETEXI
> +
> +    {
>          .name       = "info",
>          .args_type  = "item:s?",
>          .params     = "[subcommand]",
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 53e0e41..e1b7af7 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;
> +        }
> +        extend_data(0, rom->data, rom->datasize);
> +    }
> +}

Are you making unpredictable assumptions about ROM order?
You're explicitly measuring these into PCR 0 - I guess
that's probably reasonable but it should be documented.

>  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 8ca2032..92129d1 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -107,6 +107,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 {
> @@ -203,6 +204,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();
>  }
>  
>  /*
> @@ -2185,6 +2187,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_eisaid("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,
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index ffb49c1..e7a784b 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..3940d31
> --- /dev/null
> +++ b/hw/misc/measurements.c
> @@ -0,0 +1,206 @@
> +/*
> + * QEMU boot measurement
> + *
> + * Copyright (c) 2016 CoreOS, Inc <address@hidden>
> + *
> + * 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 "hw/isa/isa.h"
> +#include "crypto/hash.h"
> +#include "hw/misc/measurements.h"
> +#include "monitor/monitor.h"
> +#include "hw/loader.h"
> +
> +#define MEASUREMENT(obj) OBJECT_CHECK(MeasurementState, (obj), 
> TYPE_MEASUREMENTS)
> +
> +typedef struct MeasurementState MeasurementState;
> +
> +struct MeasurementState {
> +    ISADevice parent_obj;
> +    MemoryRegion io_select;
> +    MemoryRegion io_value;
> +    uint16_t iobase;
> +    uint8_t measurements[24][20];
> +    uint8_t tmpmeasurement[20];

Magic numbers; please use #define's somewhere.

> +    int write_count;
> +    int read_count;
> +    uint8_t pcr;
> +};
> +
> +static void measurement_reset(DeviceState *dev)
> +{
> +    MeasurementState *s = MEASUREMENT(dev);
> +
> +    memset(s->measurements, 0, sizeof(s->measurements));
> +    measure_roms();

If you're assuming that can be none-0 then don't you also
want to zero pcr, read_count, write_count?

> +}
> +
> +static void measurement_select(void *opaque, hwaddr addr, uint64_t val, 
> unsigned size)
> +{
> +    MeasurementState *s = MEASUREMENT(opaque);
> +    s->pcr = val;
> +    s->read_count = 0;
> +    s->write_count = 0;

What stops me selecting pcr 25 and overwriting stuff with junk?

> +}
> +
> +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 == 20) {
> +        s->read_count = 0;
> +    }
> +    return s->measurements[s->pcr][s->read_count++];
> +}
> +
> +static void extend(MeasurementState *s, int pcrnum, uint8_t *data)
> +{
> +    uint8_t *result;
> +    char tmpbuf[40];
> +    Error *err;
> +    size_t resultlen = 0;
> +
> +    memcpy(tmpbuf, s->measurements[pcrnum], 20);
> +    memcpy(tmpbuf + 20, data, 20);
> +    qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA1, tmpbuf, 40, &result, 
> &resultlen, &err);

I think if you're ignoring any errors then using NULL instead of &err is better.

> +    memcpy(s->measurements[pcrnum], result, 20);
> +    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 == 20) {
> +        extend(s, s->pcr, s->tmpmeasurement);
> +        s->write_count = 0;
> +    }
> +}
> +
> +void extend_data(int pcrnum, uint8_t *data, size_t len)
> +{
> +    uint8_t *result;
> +    Error *err;
> +    size_t resultlen = 0;
> +    int ret;
> +    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);

Again probably NULL here for the err unless you actually want to report it.

> +    if (ret < 0) {
> +        return;
> +    }

Hmm, what are the rules on freeing result on qcrypto_has_bytes returning
an error?

> +
> +    extend(MEASUREMENT(obj), pcrnum, result);
> +    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);
> +
> +    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 void measurement_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    fprintf(stderr, "CLASS INIT\n");

Oops, fprintf escaped into the wild.  You might like to add some trace entries.

> +    dc->realize = measurement_realize;
> +    dc->reset = measurement_reset;
> +    dc->props = measurement_props;
> +    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);
> +
> +void print_measurements(Monitor *mon, const QDict *qdict)
> +{
> +    int i, j;
> +    Object *obj = object_resolve_path_type("", TYPE_MEASUREMENTS, NULL);
> +    MeasurementState *s;
> +
> +    if (!obj) {
> +        return;
> +    }
> +
> +    s = MEASUREMENT(obj);
> +
> +    for (i = 0; i < 24; i++) {

for (pcr = ....

> +        monitor_printf(mon, "0x%02x: ", i);
> +        for (j = 0; j < 20; j++) {
> +            monitor_printf(mon, "0x%02x ", s->measurements[i][j]);
> +        }
> +        monitor_printf(mon, "\n");
> +    }
> +}
> diff --git a/hw/misc/measurements.h b/hw/misc/measurements.h
> new file mode 100644
> index 0000000..65ad246
> --- /dev/null
> +++ b/hw/misc/measurements.h
> @@ -0,0 +1,2 @@
> +void print_measurements(Monitor *mon, const QDict *qdict);
> +void extend_data(int pcrnum, uint8_t *data, size_t len);
> diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
> index c87fbad..00694fd 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 6f960f1..6aa7ebc 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/stubs/Makefile.objs b/stubs/Makefile.objs
> index 4b258a6..2ad7f81 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -41,3 +41,4 @@ stub-obj-y += target-monitor-defs.o
>  stub-obj-y += target-get-monitor-def.o
>  stub-obj-y += vhost.o
>  stub-obj-y += iohandler.o
> +stub-obj-y += measurements.o
> diff --git a/stubs/measurements.c b/stubs/measurements.c
> new file mode 100644
> index 0000000..0485d2e
> --- /dev/null
> +++ b/stubs/measurements.c
> @@ -0,0 +1,13 @@
> +#include "qemu/osdep.h"
> +#include "monitor/monitor.h"
> +#include "hw/misc/measurements.h"
> +
> +void print_measurements(Monitor *mon, const QDict *qdict)
> +{
> +    monitor_printf(mon, "No measurement support\n");
> +}
> +
> +void extend_data(int pcrnum, uint8_t *data, size_t len)
> +{
> +    return;
> +}
> -- 
> 2.7.4
> 
> 

Dave

--
Dr. David Alan Gilbert / address@hidden / Manchester, UK