* [Qemu-devel] [PATCH] hw/misc: Add simple measurement hardware
@ 2016-06-23 21:09 Matthew Garrett
2016-07-15 11:29 ` Dr. David Alan Gilbert
2016-08-05 23:17 ` [Qemu-devel] [PATCH V2] " Matthew Garrett
0 siblings, 2 replies; 21+ messages in thread
From: Matthew Garrett @ 2016-06-23 21:09 UTC (permalink / raw)
To: qemu-devel; +Cc: Matthew Garrett
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>
---
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
+@item measurements
+@findex 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);
+ }
+}
+
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 <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 "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];
+ 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();
+}
+
+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;
+}
+
+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);
+ 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);
+ if (ret < 0) {
+ return;
+ }
+
+ 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");
+ 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++) {
+ 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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/misc: Add simple measurement hardware
2016-06-23 21:09 [Qemu-devel] [PATCH] hw/misc: Add simple measurement hardware Matthew Garrett
@ 2016-07-15 11:29 ` Dr. David Alan Gilbert
2016-07-15 18:11 ` Stefan Berger
2016-07-18 21:19 ` Matthew Garrett
2016-08-05 23:17 ` [Qemu-devel] [PATCH V2] " Matthew Garrett
1 sibling, 2 replies; 21+ messages in thread
From: Dr. David Alan Gilbert @ 2016-07-15 11:29 UTC (permalink / raw)
To: Matthew Garrett; +Cc: qemu-devel, stefanb, pbonzini, berrange
* Matthew Garrett (mjg59@coreos.com) 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 <mjg59@coreos.com>
> ---
> 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
> +@item measurements
> +@findex 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 <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 "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 / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/misc: Add simple measurement hardware
2016-07-15 11:29 ` Dr. David Alan Gilbert
@ 2016-07-15 18:11 ` Stefan Berger
2016-07-18 21:26 ` Matthew Garrett
2016-07-18 21:19 ` Matthew Garrett
1 sibling, 1 reply; 21+ messages in thread
From: Stefan Berger @ 2016-07-15 18:11 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: berrange, Matthew Garrett, pbonzini, qemu-devel, stefanb
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote on 07/15/2016
07:29:24 AM:
>
> * Matthew Garrett (mjg59@coreos.com) 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
theinterface
> to the vTPM), the minus is it's very non-standard.
>
> > Trusted Boot is based around having a trusted store of measurementdata
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).
Attestation Identity Key (AIK)
>
> 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.
Are you also providing a measurement log that goes along with these PCR
extensions? Like a measurement log we have in the TCPA ACPI table? Just
measurements without knowing what was measured wouldn't be all that
helpful. Typically recipients of the measurement list would inspect the
individual measurements and replay the extensions to come up with the same
state of the PCRs that was quoted (signed) by the TPM. Also, are you going
to instrument Linux IMA to use this device? And the list goes on into
higher level tools that may work with a measurement list from
/sys/kernel/security/{tpm0,ima}/*_measurement_list and assume there's a
/dev/tpm0 there that can issue a quote with the AIK. Well, one problem is
there's little traction for the vTPM but this device here will require new
support in existing tools.
Typically the TPM is there for the reason: it is a hardware root of trust
that signs the current state of the PCRs that were accumulated by
measurements starting early on during BIOS init. Now with this device,
apart from exposing this via HMP, how would one be sure that, if the
current list of the PCRs is presented to an attesting client, that the
kernel or attestation server not just completely fake the state of the
PCRs? My assumption here is that the state of this device's PCRs will be
exposed to user level application that can then use this in some form of
attestation, right?
> >
> > 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?
With a measurement list I would hope -- in some form of custom ACPI table
or some other buffer?
Stefan
>
> > 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 = ....
asexamples)
> 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 <mjg59@coreos.com>
> > ---
> > 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
> > +@item measurements
> > +@findex 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 <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 "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 / dgilbert@redhat.com / Manchester, UK
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/misc: Add simple measurement hardware
2016-07-15 11:29 ` Dr. David Alan Gilbert
2016-07-15 18:11 ` Stefan Berger
@ 2016-07-18 21:19 ` Matthew Garrett
2016-07-19 9:38 ` Dr. David Alan Gilbert
1 sibling, 1 reply; 21+ messages in thread
From: Matthew Garrett @ 2016-07-18 21:19 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: qemu-devel, stefanb, pbonzini, Daniel P. Berrange
On Fri, Jul 15, 2016 at 4:29 AM, Dr. David Alan Gilbert <dgilbert@redhat.com
> wrote:
> * Matthew Garrett (mjg59@coreos.com) wrote:
> 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.
>
The guest can provide the log, but you'd want to obtain the measurements
from the hypervisor. The precise implementation of this would probably be
somewhat provider-specific - AWS has support for providing signed copies of
image metadata, so this could be implemented in a similar way (eg, guest
hits the local API endpoint, hypervisor extracts measurement data and signs
it, provides that back to guest, guest hands over log and signed
measurements to whatever's handling the attestation)
> 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).
>
You have the same problem with a TPM - the quote you get back has a chain
of trust back to the TPM vendor, but unless you've previously established
some specific trust with that system you have no way of knowing whether
it's the specific TPM you want to talk to or not. In some ways it's easier
with a hypervisor, because it has more information about the guest than a
TPM does about the OS. For example, the hypervisor can provide a signed
measurement alongside signed metadata that includes the guest's IP address.
If they're signed with the same key and the IP address matches what you're
talking to, you've established that trust in a much more straightforward
manner.
> 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.
>
I think that's covered as above.
> 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?
>
Yes - my modified SeaBIOS provides the TCG measurement functions and simply
stubs them into this rather than the real TPM. Running
https://github.com/coreos/grub against that gives me a full set of boot
measurements, including log.
> > 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?
>
For now, because building EFI is tedious. There's nothing BIOS specific,
and I'll add support to OVMF once we've nailed down what the interface
looks like.
> > 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.
>
Yeah. Not sure whether the right approach is to use port IO where available
and MMIO elsewhere, or just suck up the additional implementation work and
do MMIO everywhere.
> 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.
>
Hmm, yes. I think we'd need to carefully consider what the semantics of
measurement over migration are - do you think this is necessary for an
initial implementation, or could it come later?
> QMP support: You should wire it up to the qmp monitor as well.
> Generally the management tools use qmp rather than hmp,
>
Good call.
> 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.
>
Mm good question. I *think* my argument would be that, unless we're
executing code from that ROM, it shouldn't be measured. That's how it would
behave with a real TPM on real hardware. So no to measurement on hotplug,
yes to measurement if it's present after reboot.
> Why do SHA1 based TPM 1.2 now, shouldn't you start with TPM2/newer SHAs?
>
There's no TPM2 spec for BIOS, so it'd involve adding an incompatible
interface to BIOS to make use of it. However, adding support for additional
hashes and then just having the BIOS code always use SHA1 ought to be fine.
> 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?
>
My plan was to abstract this at the firmware interface level, rather than
at the hardware level - for the functionality I'm looking at, emulating the
TPM command set would add a *lot* of additional complexity. The benefit
would be being able to use existing drivers, but I don't even know how much
functionality I'd need to implement in order to get, say, Trousers running.
> +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.
>
Hm. The ordering issue can be basically avoided by having this be logged,
which means passing the data over from qemu to the firmware and letting the
firmware use that to populate the log. What's the best way to do that
likely to be?
> +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.
>
Ok.
> > + 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?
>
Hm, yes.
> > +}
> > +
> > +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?
>
Oops! Yeah uh that's a really rather good point and I am a bad programmer.
> + 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.
>
Ok.
> > + if (ret < 0) {
> > + return;
> > + }
>
> Hmm, what are the rules on freeing result on qcrypto_has_bytes returning
> an error?
>
I'll dig into the code.
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + fprintf(stderr, "CLASS INIT\n");
Oops, fprintf escaped into the wild. You might like to add some trace
> entries.
>
Heh. Yup.
Thanks for the review!
--
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/misc: Add simple measurement hardware
2016-07-15 18:11 ` Stefan Berger
@ 2016-07-18 21:26 ` Matthew Garrett
2016-07-18 23:40 ` Stefan Berger
0 siblings, 1 reply; 21+ messages in thread
From: Matthew Garrett @ 2016-07-18 21:26 UTC (permalink / raw)
To: Stefan Berger
Cc: Dr. David Alan Gilbert, Daniel P. Berrange, pbonzini, qemu-devel,
stefanb
On Fri, Jul 15, 2016 at 11:11 AM, Stefan Berger <stefanb@us.ibm.com> wrote:
>
> Are you also providing a measurement log that goes along with these PCR extensions? Like a measurement log we have in the TCPA ACPI table? Just measurements without knowing what was measured wouldn't be all that helpful. Typically recipients of the measurement list would inspect the individual measurements and replay the extensions to come up with the same state of the PCRs that was quoted (signed) by the TPM. Also, are you going to instrument Linux IMA to use this device? And the list goes on into higher level tools that may work with a measurement list from /sys/kernel/security/{tpm0,ima}/*_measurement_list and assume there's a /dev/tpm0 there that can issue a quote with the AIK. Well, one problem is there's little traction for the vTPM but this device here will require new support in existing tools.
Yes, the firmware will build a log - right now I'm using the TCPA
format. However, the initial measurements from qemu aren't being
handed over to the firmware, so I need to fix that. There's no
fundamental difficulty in adding IMA support, I have a small driver
that can register with enough of the TPM layer to allow its extend
calls to work. I'm not too worried about the existing tooling not
working, to be honest. Making that work would require the hypervisor
to be able to provide a signed quote in TPM format, and that seems
like a much harder sell.
>
> Typically the TPM is there for the reason: it is a hardware root of trust that signs the current state of the PCRs that were accumulated by measurements starting early on during BIOS init. Now with this device, apart from exposing this via HMP, how would one be sure that, if the current list of the PCRs is presented to an attesting client, that the kernel or attestation server not just completely fake the state of the PCRs? My assumption here is that the state of this device's PCRs will be exposed to user level application that can then use this in some form of attestation, right?
Userspace will be able to grab it, but the idea is that the hypervisor
API will allow a copy to be obtained - either a signed copy from the
local API endpoint, or directly via a remote API endpoint. The guest
won't be able to fake the former case, and isn't involved at all in
the latter case.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/misc: Add simple measurement hardware
2016-07-18 21:26 ` Matthew Garrett
@ 2016-07-18 23:40 ` Stefan Berger
2016-07-18 23:52 ` Matthew Garrett
0 siblings, 1 reply; 21+ messages in thread
From: Stefan Berger @ 2016-07-18 23:40 UTC (permalink / raw)
To: Matthew Garrett
Cc: Daniel P. Berrange, Dr. David Alan Gilbert, pbonzini, qemu-devel,
stefanb
Matthew Garrett <mjg59@coreos.com> wrote on 07/18/2016 05:26:03 PM:
>
> On Fri, Jul 15, 2016 at 11:11 AM, Stefan Berger <stefanb@us.ibm.com>
wrote:
> >
> >
> > Typically the TPM is there for the reason: it is a hardware root
> of trust that signs the current state of the PCRs that were
> accumulated by measurements starting early on during BIOS init. Now
> with this device, apart from exposing this via HMP, how would one be
> sure that, if the current list of the PCRs is presented to an
> attesting client, that the kernel or attestation server not just
> completely fake the state of the PCRs? My assumption here is that
> the state of this device's PCRs will be exposed to user level
> application that can then use this in some form of attestation, right?
>
>
> Userspace will be able to grab it, but the idea is that the hypervisor
> API will allow a copy to be obtained - either a signed copy from the
> local API endpoint, or directly via a remote API endpoint. The guest
> won't be able to fake the former case, and isn't involved at all in
> the latter case.
>
The TPM security's model related to logs, the state of the PCRs, and
attestation involves the following pieces:
- PCRs
- measurement log
- EK + certificate
- platform certificate
- AIK + certificate
- quotes (signatures) on PCR state with keys that cannot leave the TPM
(AIKs)
- infrastructure to issue the AIK certificates based on EK + certificate +
platform certificate
How does the security model of this device and its presumed infrastructure
look like? Does the hypervisor then also support IMA measurement lists or
is this restricted to firmware?
Stefan
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/misc: Add simple measurement hardware
2016-07-18 23:40 ` Stefan Berger
@ 2016-07-18 23:52 ` Matthew Garrett
2016-07-19 0:08 ` Stefan Berger
0 siblings, 1 reply; 21+ messages in thread
From: Matthew Garrett @ 2016-07-18 23:52 UTC (permalink / raw)
To: Stefan Berger
Cc: Daniel P. Berrange, Dr. David Alan Gilbert, pbonzini, qemu-devel,
stefanb
On Mon, Jul 18, 2016 at 4:40 PM, Stefan Berger <stefanb@us.ibm.com> wrote:
> The TPM security's model related to logs, the state of the PCRs, and
> attestation involves the following pieces:
>
> - PCRs
> - measurement log
> - EK + certificate
> - platform certificate
> - AIK + certificate
> - quotes (signatures) on PCR state with keys that cannot leave the TPM
> (AIKs)
> - infrastructure to issue the AIK certificates based on EK + certificate +
> platform certificate
>
> How does the security model of this device and its presumed infrastructure
> look like? Does the hypervisor then also support IMA measurement lists or is
> this restricted to firmware?
The model here is:
- PCRs
- measurement log
- quote on PCR state with key held by hypervisor
There's no fundamental reason why additional layers of key can't be
introduced, but since all that complexity is on the hypervisor side
it's out of scope for the qemu implementation. A mechanism for
establishing trust between the hypervisor and the customer is
obviously necessary, but there are already examples such as Amazon's
Instance Identity Documents (
http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/instance-identity-documents.html
). The OS is free to continue to extend the PCRs after boot, so IMA
could certainly be integrated with this.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/misc: Add simple measurement hardware
2016-07-18 23:52 ` Matthew Garrett
@ 2016-07-19 0:08 ` Stefan Berger
2016-07-19 0:39 ` Matthew Garrett
0 siblings, 1 reply; 21+ messages in thread
From: Stefan Berger @ 2016-07-19 0:08 UTC (permalink / raw)
To: Matthew Garrett
Cc: Daniel P. Berrange, Dr. David Alan Gilbert, pbonzini, qemu-devel,
stefanb
Matthew Garrett <mjg59@coreos.com> wrote on 07/18/2016 07:52:22 PM:
> Subject: Re: [Qemu-devel] [PATCH] hw/misc: Add simple measurement
hardware
>
> On Mon, Jul 18, 2016 at 4:40 PM, Stefan Berger <stefanb@us.ibm.com>
wrote:
> > The TPM security's model related to logs, the state of the PCRs, and
> > attestation involves the following pieces:
> >
> > - PCRs
> > - measurement log
> > - EK + certificate
> > - platform certificate
> > - AIK + certificate
> > - quotes (signatures) on PCR state with keys that cannot leave the TPM
> > (AIKs)
> > - infrastructure to issue the AIK certificates based on EK +
certificate +
> > platform certificate
> >
> > How does the security model of this device and its presumed
infrastructure
> > look like? Does the hypervisor then also support IMA measurement lists
or is
> > this restricted to firmware?
>
> The model here is:
> - PCRs
> - measurement log
> - quote on PCR state with key held by hypervisor
>
> There's no fundamental reason why additional layers of key can't be
> introduced, but since all that complexity is on the hypervisor side
> it's out of scope for the qemu implementation. A mechanism for
> establishing trust between the hypervisor and the customer is
> obviously necessary, but there are already examples such as Amazon's
> Instance Identity Documents (
> http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/instance-
> identity-documents.html
> ). The OS is free to continue to extend the PCRs after boot, so IMA
> could certainly be integrated with this.
>
The point of the TPM is that the device that holds the state of the PCRs
provides the signatures over their state rather than some other 'entity'
whose trustworthiness wouldn't be clear. Admittedly the device comes with
its own set of challenges.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/misc: Add simple measurement hardware
2016-07-19 0:08 ` Stefan Berger
@ 2016-07-19 0:39 ` Matthew Garrett
2016-07-19 0:46 ` Stefan Berger
0 siblings, 1 reply; 21+ messages in thread
From: Matthew Garrett @ 2016-07-19 0:39 UTC (permalink / raw)
To: Stefan Berger
Cc: stefanb, qemu-devel, Daniel P. Berrange, pbonzini,
Dr. David Alan Gilbert
On Jul 18, 2016 17:08, "Stefan Berger" <stefanb@us.ibm.com> wrote:
> The point of the TPM is that the device that holds the state of the PCRs
provides the signatures over their state rather than some other 'entity'
whose trustworthiness wouldn't be clear. Admittedly the device comes with
its own set of challenges.
The hypervisor holds the PCR state and also provides the signature. If the
hypervisor is untrustworthy than the state of the virtualised system can
never be verified, since it could simply have faked the measurements passed
to whatever the root of trust is.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/misc: Add simple measurement hardware
2016-07-19 0:39 ` Matthew Garrett
@ 2016-07-19 0:46 ` Stefan Berger
2016-07-19 0:49 ` Matthew Garrett
0 siblings, 1 reply; 21+ messages in thread
From: Stefan Berger @ 2016-07-19 0:46 UTC (permalink / raw)
To: Matthew Garrett
Cc: Daniel P. Berrange, Dr. David Alan Gilbert, pbonzini, qemu-devel,
stefanb
Matthew Garrett <mjg59@coreos.com> wrote on 07/18/2016 08:39:07 PM:
>
> On Jul 18, 2016 17:08, "Stefan Berger" <stefanb@us.ibm.com> wrote:
> > The point of the TPM is that the device that holds the state of
> the PCRs provides the signatures over their state rather than some
> other 'entity' whose trustworthiness wouldn't be clear. Admittedly
> the device comes with its own set of challenges.
> The hypervisor holds the PCR state and also provides the signature.
> If the hypervisor is untrustworthy than the state of the virtualised
> system can never be verified, since it could simply have faked the
> measurements passed to whatever the root of trust is.
So the hypervisor will have the key for signing and provide the quote ?
Stefan
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/misc: Add simple measurement hardware
2016-07-19 0:46 ` Stefan Berger
@ 2016-07-19 0:49 ` Matthew Garrett
0 siblings, 0 replies; 21+ messages in thread
From: Matthew Garrett @ 2016-07-19 0:49 UTC (permalink / raw)
To: Stefan Berger
Cc: stefanb, qemu-devel, Daniel P. Berrange, pbonzini,
Dr. David Alan Gilbert
On Jul 18, 2016 17:46, "Stefan Berger" <stefanb@us.ibm.com> wrote:
>
>
> Matthew Garrett <mjg59@coreos.com> wrote on 07/18/2016 08:39:07 PM:
>
>
> >
> > On Jul 18, 2016 17:08, "Stefan Berger" <stefanb@us.ibm.com> wrote:
> > > The point of the TPM is that the device that holds the state of
> > the PCRs provides the signatures over their state rather than some
> > other 'entity' whose trustworthiness wouldn't be clear. Admittedly
> > the device comes with its own set of challenges.
>
> > The hypervisor holds the PCR state and also provides the signature.
> > If the hypervisor is untrustworthy than the state of the virtualised
> > system can never be verified, since it could simply have faked the
> > measurements passed to whatever the root of trust is.
>
> So the hypervisor will have the key for signing and provide the quote ?
Either the hypervisor itself or part of the associated platform. This
framework is typically inside the same trust boundary.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/misc: Add simple measurement hardware
2016-07-18 21:19 ` Matthew Garrett
@ 2016-07-19 9:38 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 21+ messages in thread
From: Dr. David Alan Gilbert @ 2016-07-19 9:38 UTC (permalink / raw)
To: Matthew Garrett; +Cc: qemu-devel, stefanb, pbonzini, Daniel P. Berrange
* Matthew Garrett (mjg59@coreos.com) wrote:
> On Fri, Jul 15, 2016 at 4:29 AM, Dr. David Alan Gilbert <dgilbert@redhat.com
> > wrote:
>
> > * Matthew Garrett (mjg59@coreos.com) wrote:
> > 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.
> >
>
> The guest can provide the log, but you'd want to obtain the measurements
> from the hypervisor. The precise implementation of this would probably be
> somewhat provider-specific - AWS has support for providing signed copies of
> image metadata, so this could be implemented in a similar way (eg, guest
> hits the local API endpoint, hypervisor extracts measurement data and signs
> it, provides that back to guest, guest hands over log and signed
> measurements to whatever's handling the attestation)
Yes, this doesn't seem too bad.
> > 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).
> >
>
> You have the same problem with a TPM - the quote you get back has a chain
> of trust back to the TPM vendor, but unless you've previously established
> some specific trust with that system you have no way of knowing whether
> it's the specific TPM you want to talk to or not. In some ways it's easier
> with a hypervisor, because it has more information about the guest than a
> TPM does about the OS. For example, the hypervisor can provide a signed
> measurement alongside signed metadata that includes the guest's IP address.
> If they're signed with the same key and the IP address matches what you're
> talking to, you've established that trust in a much more straightforward
> manner.
Hmm true; with TPM once you have done it once and enrolled it you do know that
you're making the measurement against the same device; but in your world
that's probably true if the host is involved somewhere with getting the measurement.
But I don't see in your description how the guest would get a copy of it's
PCRs signed by the host.
> > 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.
> >
>
> I think that's covered as above.
Yep, same.
> > 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?
> >
>
> Yes - my modified SeaBIOS provides the TCG measurement functions and simply
> stubs them into this rather than the real TPM. Running
> https://github.com/coreos/grub against that gives me a full set of boot
> measurements, including log.
Is that reimplementing TPM support in grub? Isn't there a version
that already has it?
Dealing with those measurements is really difficult in practice - in particular
things like knowing whether a kernel command line is sane/safe is pretty
difficult, as is knowing the sanity of an initrd; so please make sure
those measurements as close to normal TPM behaviour as possible.
> > > 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?
> >
>
> For now, because building EFI is tedious. There's nothing BIOS specific,
> and I'll add support to OVMF once we've nailed down what the interface
> looks like.
>
>
> > > 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.
> >
>
> Yeah. Not sure whether the right approach is to use port IO where available
> and MMIO elsewhere, or just suck up the additional implementation work and
> do MMIO everywhere.
>
>
> > 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.
> >
>
> Hmm, yes. I think we'd need to carefully consider what the semantics of
> measurement over migration are - do you think this is necessary for an
> initial implementation, or could it come later?
My main work is migration so of course I'd say you should think about it;
but you're right you can probably leave it at first.
My assumption would be that you'd just migrate the values as part of the
stream (pretty easy see VMState stuff in other drivers). I can't remember
the behaviour you'll see after a reset on the destination and whether
that means you get the destinations ROMs or the ROMs that were migrated.
> > QMP support: You should wire it up to the qmp monitor as well.
> > Generally the management tools use qmp rather than hmp,
> >
>
> Good call.
>
>
> > 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.
> >
>
> Mm good question. I *think* my argument would be that, unless we're
> executing code from that ROM, it shouldn't be measured. That's how it would
> behave with a real TPM on real hardware. So no to measurement on hotplug,
> yes to measurement if it's present after reboot.
OK.
> > Why do SHA1 based TPM 1.2 now, shouldn't you start with TPM2/newer SHAs?
> >
>
> There's no TPM2 spec for BIOS, so it'd involve adding an incompatible
> interface to BIOS to make use of it. However, adding support for additional
> hashes and then just having the BIOS code always use SHA1 ought to be fine.
Yes, although it's probably messier than that; if I remember correctly
the new hashes are longer.
> > 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?
> >
>
> My plan was to abstract this at the firmware interface level, rather than
> at the hardware level - for the functionality I'm looking at, emulating the
> TPM command set would add a *lot* of additional complexity. The benefit
> would be being able to use existing drivers, but I don't even know how much
> functionality I'd need to implement in order to get, say, Trousers running.
>
> > +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.
> >
>
> Hm. The ordering issue can be basically avoided by having this be logged,
> which means passing the data over from qemu to the firmware and letting the
> firmware use that to populate the log. What's the best way to do that
> likely to be?
Hmm, not sure. We know this is happening before the guest starts running,
so if it somehow made it's way to the code that sets up ACPI perhaps that
could do it; but I'm not sure.
> > +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.
> >
>
> Ok.
>
>
> > > + 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?
> >
>
> Hm, yes.
>
>
> > > +}
> > > +
> > > +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?
> >
>
> Oops! Yeah uh that's a really rather good point and I am a bad programmer.
>
> > + 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.
> >
>
> Ok.
>
>
> > > + if (ret < 0) {
> > > + return;
> > > + }
> >
> > Hmm, what are the rules on freeing result on qcrypto_has_bytes returning
> > an error?
> >
>
> I'll dig into the code.
>
> > + DeviceClass *dc = DEVICE_CLASS(klass);
> > + fprintf(stderr, "CLASS INIT\n");
>
> Oops, fprintf escaped into the wild. You might like to add some trace
> > entries.
> >
>
> Heh. Yup.
>
> Thanks for the review!
No problem.
Dave
> --
>
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH V2] hw/misc: Add simple measurement hardware
2016-06-23 21:09 [Qemu-devel] [PATCH] hw/misc: Add simple measurement hardware Matthew Garrett
2016-07-15 11:29 ` Dr. David Alan Gilbert
@ 2016-08-05 23:17 ` Matthew Garrett
2016-08-06 2:53 ` Eric Blake
2016-08-06 3:56 ` Stefan Berger
1 sibling, 2 replies; 21+ messages in thread
From: Matthew Garrett @ 2016-08-05 23:17 UTC (permalink / raw)
To: qemu-devel; +Cc: berrange, dgilbert, stefanb, Matthew Garrett
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>
---
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 +++
stubs/Makefile.objs | 1 +
stubs/measurements.c | 18 +++
16 files changed, 448 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..c2ef869 100644
--- a/hmp.c
+++ 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);
+ MeasurementList *info;
+
+ for (info = info_list; info; info = info->next) {
+ monitor_printf(mon, "%02ld: %s\n", info->value->pcr,
+ info->value->hash);
+ }
+
+ qapi_free_MeasurementList(info_list);
+}
+
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..4d55f81
--- /dev/null
+++ b/hw/misc/measurements.c
@@ -0,0 +1,295 @@
+/*
+ * 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/isa/isa.h"
+#include "hw/misc/measurements.h"
+#include "qmp-commands.h"
+
+#define MEASUREMENT(obj) OBJECT_CHECK(MeasurementState, (obj), TYPE_MEASUREMENTS)
+
+#define DIGEST_SIZE 20
+#define PCR_COUNT 24
+
+typedef struct MeasurementState MeasurementState;
+
+struct MeasurementState {
+ ISADevice parent_obj;
+ MemoryRegion io_select;
+ MemoryRegion io_value;
+ uint16_t iobase;
+ uint8_t measurements[PCR_COUNT][DIGEST_SIZE];
+ uint8_t tmpmeasurement[DIGEST_SIZE];
+ int write_count;
+ int read_count;
+ uint8_t pcr;
+ int logsize;
+ struct tpm_event *log;
+};
+
+struct tpm_event {
+ uint32_t pcrindex;
+ uint32_t eventtype;
+ uint8_t digest[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 > 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 == DIGEST_SIZE) {
+ s->read_count = 0;
+ }
+ return s->measurements[s->pcr][s->read_count++];
+}
+
+static void extend(MeasurementState *s, int pcrnum, uint8_t *data)
+{
+ Error *err;
+ char tmpbuf[40];
+ size_t resultlen = 0;
+ uint8_t *result = NULL;
+
+ memcpy(tmpbuf, s->measurements[pcrnum], DIGEST_SIZE);
+ memcpy(tmpbuf + DIGEST_SIZE, data, DIGEST_SIZE);
+ if (qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA1, tmpbuf, 40, &result, &resultlen, &err) == 0) {
+ memcpy(s->measurements[pcrnum], result, DIGEST_SIZE);
+ } else {
+ const char *msg = error_get_pretty(err);
+ fprintf(stderr, "Failed to measure data: %s\n", msg);
+ error_free(err);
+ }
+
+ 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 == DIGEST_SIZE) {
+ extend(s, s->pcr, s->tmpmeasurement);
+ s->write_count = 0;
+ }
+}
+
+static void log_data(MeasurementState *s, int 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;
+
+ logentry = (struct tpm_event *)(((void *)s->log) + s->logsize);
+ logentry->pcrindex = pcrnum;
+ logentry->eventtype = 1;
+ memcpy(logentry->digest, hash, DIGEST_SIZE);
+ logentry->eventdatasize = eventlen;
+ memcpy(logentry->event, description, eventlen);
+
+ s->logsize += entrylen;
+}
+
+void measurements_extend_data(int 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) {
+ const char *msg = error_get_pretty(err);
+ fprintf(stderr, "Failed to hash extension data: %s\n", msg);
+ return;
+ }
+
+ 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);
+
+ 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 const VMStateDescription measurement_state = {
+ .name = "measurements",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT16(iobase, MeasurementState),
+ VMSTATE_BUFFER_UNSAFE(measurements, MeasurementState, 0, PCR_COUNT * DIGEST_SIZE),
+ VMSTATE_BUFFER(tmpmeasurement, MeasurementState),
+ VMSTATE_INT32(write_count, MeasurementState),
+ VMSTATE_INT32(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) {
+ return NULL;
+ }
+
+ 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);
+ 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;
+}
+
+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..7cb8db2
--- /dev/null
+++ b/hw/misc/measurements.h
@@ -0,0 +1,4 @@
+#include "hw/sysbus.h"
+
+void measurements_extend_data(int pcrnum, uint8_t *data, size_t len, char *description);
+void measurements_set_log(gchar *log);
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 5d68a5d..3affc90 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..6151fcd 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4338,3 +4338,29 @@
# Since: 2.7
##
{ 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }
+
+##
+# @Measurement
+#
+# @pcr: The PCR in the measurement
+# @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.
+#
+# Since: 2.7
+##
+{ 'struct': 'Measurement',
+ 'data': { 'pcr': 'int',
+ 'hash': 'str'
+ }
+}
+
+##
+# @query-measurements
+#
+# Returns: a list of Measurement objects
+#
+# Since: 2.7
+##
+{ '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,
+ },
+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..49fc4bd
--- /dev/null
+++ 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;
+}
+
+void measurements_extend_data(int pcrnum, uint8_t *data, size_t len, char *description)
+{
+ return;
+}
+
+void measurements_set_log(gchar *log)
+{
+ return;
+}
--
2.7.4
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH V2] hw/misc: Add simple measurement hardware
2016-08-05 23:17 ` [Qemu-devel] [PATCH V2] " Matthew Garrett
@ 2016-08-06 2:53 ` Eric Blake
2016-08-06 3:56 ` Stefan Berger
1 sibling, 0 replies; 21+ messages in thread
From: Eric Blake @ 2016-08-06 2:53 UTC (permalink / raw)
To: Matthew Garrett, qemu-devel; +Cc: stefanb, dgilbert
[-- 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 --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH V2] hw/misc: Add simple measurement hardware
2016-08-05 23:17 ` [Qemu-devel] [PATCH V2] " Matthew Garrett
2016-08-06 2:53 ` Eric Blake
@ 2016-08-06 3:56 ` Stefan Berger
2016-08-08 19:43 ` Matthew Garrett
1 sibling, 1 reply; 21+ messages in thread
From: Stefan Berger @ 2016-08-06 3:56 UTC (permalink / raw)
To: Matthew Garrett; +Cc: berrange, dgilbert, qemu-devel
Matthew Garrett <mjg59@coreos.com> wrote on 08/05/2016 07:17:12 PM:
>
> 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.
Port io is x86 specific, right? I don't think it should stay an x86
specific device.
>
> Signed-off-by: Matthew Garrett <mjg59@coreos.com>
> ---
>
> 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 +++
> stubs/Makefile.objs | 1 +
> stubs/measurements.c | 18 +++
> 16 files changed, 448 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..c2ef869 100644
> --- a/hmp.c
> +++ 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);
> + MeasurementList *info;
> +
> + for (info = info_list; info; info = info->next) {
> + monitor_printf(mon, "%02ld: %s\n", info->value->pcr,
> + info->value->hash);
> + }
> +
> + qapi_free_MeasurementList(info_list);
> +}
> +
> 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 this device is hitchhiking on the TPM's ACPI tables, then are you also
making this device mutually exclusive with the TPM ? Of not please do so.
> - 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..4d55f81
> --- /dev/null
> +++ b/hw/misc/measurements.c
> @@ -0,0 +1,295 @@
> +/*
> + * 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/isa/isa.h"
> +#include "hw/misc/measurements.h"
> +#include "qmp-commands.h"
> +
> +#define MEASUREMENT(obj) OBJECT_CHECK(MeasurementState, (obj),
> TYPE_MEASUREMENTS)
> +
> +#define DIGEST_SIZE 20
> +#define PCR_COUNT 24
> +
> +typedef struct MeasurementState MeasurementState;
> +
> +struct MeasurementState {
> + ISADevice parent_obj;
> + MemoryRegion io_select;
> + MemoryRegion io_value;
> + uint16_t iobase;
> + uint8_t measurements[PCR_COUNT][DIGEST_SIZE];
> + uint8_t tmpmeasurement[DIGEST_SIZE];
> + int write_count;
> + int read_count;
> + uint8_t pcr;
> + int logsize;
> + struct tpm_event *log;
> +};
> +
> +struct tpm_event {
> + uint32_t pcrindex;
> + uint32_t eventtype;
> + uint8_t digest[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 > 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 == DIGEST_SIZE) {
> + s->read_count = 0;
> + }
> + return s->measurements[s->pcr][s->read_count++];
> +}
> +
> +static void extend(MeasurementState *s, int pcrnum, uint8_t *data)
> +{
> + Error *err;
> + char tmpbuf[40];
> + size_t resultlen = 0;
> + uint8_t *result = NULL;
> +
> + memcpy(tmpbuf, s->measurements[pcrnum], DIGEST_SIZE);
> + memcpy(tmpbuf + DIGEST_SIZE, data, DIGEST_SIZE);
> + if (qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA1, tmpbuf, 40,
> &result, &resultlen, &err) == 0) {
> + memcpy(s->measurements[pcrnum], result, DIGEST_SIZE);
> + } else {
> + const char *msg = error_get_pretty(err);
> + fprintf(stderr, "Failed to measure data: %s\n", msg);
> + error_free(err);
> + }
> +
> + 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 == DIGEST_SIZE) {
> + extend(s, s->pcr, s->tmpmeasurement);
> + s->write_count = 0;
> + }
> +}
> +
> +static void log_data(MeasurementState *s, int 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;
> +
> + logentry = (struct tpm_event *)(((void *)s->log) + s->logsize);
> + logentry->pcrindex = pcrnum;
> + logentry->eventtype = 1;
> + memcpy(logentry->digest, hash, DIGEST_SIZE);
> + logentry->eventdatasize = eventlen;
> + memcpy(logentry->event, description, eventlen);
> +
> + s->logsize += entrylen;
> +}
> +
> +void measurements_extend_data(int 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) {
> + const char *msg = error_get_pretty(err);
> + fprintf(stderr, "Failed to hash extension data: %s\n", msg);
> + return;
> + }
> +
> + 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);
> +
> + 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 const VMStateDescription measurement_state = {
> + .name = "measurements",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT16(iobase, MeasurementState),
> + VMSTATE_BUFFER_UNSAFE(measurements, MeasurementState, 0,
> PCR_COUNT * DIGEST_SIZE),
> + VMSTATE_BUFFER(tmpmeasurement, MeasurementState),
> + VMSTATE_INT32(write_count, MeasurementState),
> + VMSTATE_INT32(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) {
> + return NULL;
> + }
> +
> + 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);
> + 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;
> +}
> +
> +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..7cb8db2
> --- /dev/null
> +++ b/hw/misc/measurements.h
> @@ -0,0 +1,4 @@
> +#include "hw/sysbus.h"
> +
> +void measurements_extend_data(int pcrnum, uint8_t *data, size_t
> len, char *description);
> +void measurements_set_log(gchar *log);
> 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 5d68a5d..3affc90 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..6151fcd 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4338,3 +4338,29 @@
> # Since: 2.7
> ##
> { 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU']
}
> +
> +##
> +# @Measurement
> +#
> +# @pcr: The PCR in the measurement
> +# @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.
> +#
> +# Since: 2.7
> +##
> +{ 'struct': 'Measurement',
> + 'data': { 'pcr': 'int',
> + 'hash': 'str'
> + }
> +}
> +
> +##
> +# @query-measurements
> +#
> +# Returns: a list of Measurement objects
> +#
> +# Since: 2.7
> +##
> +{ '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,
> + },
> +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..49fc4bd
> --- /dev/null
> +++ 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;
> +}
> +
> +void measurements_extend_data(int pcrnum, uint8_t *data, size_t
> len, char *description)
> +{
> + return;
> +}
> +
> +void measurements_set_log(gchar *log)
> +{
> + return;
> +}
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH V2] hw/misc: Add simple measurement hardware
2016-08-06 3:56 ` Stefan Berger
@ 2016-08-08 19:43 ` Matthew Garrett
2016-08-09 14:58 ` Stefan Berger
0 siblings, 1 reply; 21+ messages in thread
From: Matthew Garrett @ 2016-08-08 19:43 UTC (permalink / raw)
To: Stefan Berger; +Cc: Daniel P. Berrange, Dr. David Alan Gilbert, qemu-devel
On Fri, Aug 5, 2016 at 8:56 PM, Stefan Berger <stefanb@us.ibm.com> wrote:
> Matthew Garrett <mjg59@coreos.com> wrote on 08/05/2016 07:17:12 PM:
>> This version of the implementation depends on port io, but if there's
>> interest I'll add mmio as well.
>
> Port io is x86 specific, right? I don't think it should stay an x86 specific
> device.
Not strictly, there are other architectures that implement it. But
yes, most other platforms will want MMIO support. The intent is
certainly to add that.
>> - 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 this device is hitchhiking on the TPM's ACPI tables, then are you also
> making this device mutually exclusive with the TPM ? Of not please do so.
I'll look into the best way to do that.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH V2] hw/misc: Add simple measurement hardware
2016-08-08 19:43 ` Matthew Garrett
@ 2016-08-09 14:58 ` Stefan Berger
0 siblings, 0 replies; 21+ messages in thread
From: Stefan Berger @ 2016-08-09 14:58 UTC (permalink / raw)
To: Matthew Garrett; +Cc: Daniel P. Berrange, Dr. David Alan Gilbert, qemu-devel
Matthew Garrett <mjg59@coreos.com> wrote on 08/08/2016 03:43:57 PM:
>
> On Fri, Aug 5, 2016 at 8:56 PM, Stefan Berger <stefanb@us.ibm.com>
wrote:
> > Matthew Garrett <mjg59@coreos.com> wrote on 08/05/2016 07:17:12 PM:
> >> This version of the implementation depends on port io, but if there's
> >> interest I'll add mmio as well.
> >
> > Port io is x86 specific, right? I don't think it should stay an x86
specific
> > device.
>
> Not strictly, there are other architectures that implement it. But
> yes, most other platforms will want MMIO support. The intent is
> certainly to add that.
>
> >> - 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 this device is hitchhiking on the TPM's ACPI tables, then are you
also
> > making this device mutually exclusive with the TPM ? Of not please do
so.
>
> I'll look into the best way to do that.
>
What's the plan for the driver level in Linux? Are you going to have this
device's measurement log appear under another path than the TPM path
/sys/kernel/security/tpm0/{ascii|binary}_bios_measurements? Also, I hope
that not /dev/tpm0 will be made available. Basically I think it is a mess
to use another device's ACPI table while not being compatible at all.
Though a measurement list is necessary for the state of the PCRs to make
sense.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/misc: Add simple measurement hardware
2016-07-14 14:54 ` Daniel P. Berrange
@ 2016-07-15 0:10 ` Matthew Garrett
0 siblings, 0 replies; 21+ messages in thread
From: Matthew Garrett @ 2016-07-15 0:10 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: qemu-devel
On Thu, Jul 14, 2016 at 11:54 PM, Daniel P. Berrange <berrange@redhat.com>
wrote:
> On Thu, Jun 23, 2016 at 04:36:59PM -0700, Matthew Garrett wrote:
> > 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.
>
> Will it be capable of workubg with edk2/OVMF/AVMF as well as SeaBIOS ?
>
> Yes, that will work fine.
> > This version of the implementation depends on port io, but if there's
> interest I'll
> > add mmio as well.
>
> So I guess this is the reason you've only enabled it for x86_64. Since
> we're
> inventing an entirely new type of device here, not copying existing
> hardware,
> I think it'd desirable if we created one that was supported across arches,
> particularly aarch64, since that's the new hotness. With the convergance
> of both x86_64 and aarch64 to EFI, it'd be nice to aim for parity for this
> trusted boot support too if practical.
>
Fair. I can redo this so it's mmio everywhere.
>
> > 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
> > +@item measurements
> > +@findex measurements
> > +Redirect Print system measurements
> > +ETEXI
> > +
> > + {
>
> Thus since is just reporting info about a device, from a HMP POV,
> it would fit better as an 'info' sub-command, eg 'info measurements'.
> The QMP equivalent would be a 'query-measurements' command
>
Ok.
>
> > +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++) {
> > + 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");
> > + }
> > +}
>
> The preferred approach to supporting monitor commands these
> days is to first define a schema for the information to be
> output in qapi-schema.json. Then implement a QMP command
> that returns an instance of the data structure you defined.
> Finally the HMP command, would invoke the QMP command to
> get the data to be printed.
>
Ok, thanks for the pointers!
>
> > 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);
>
> 'extend_data' is rather too generic a name, for expose across
> QEMU. Just add a "measurements_" prefix for any exported methods
> from the measurements device.
>
Will do. Thanks for the feedback!
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/misc: Add simple measurement hardware
2016-06-23 23:36 [Qemu-devel] [PATCH] " Matthew Garrett
2016-07-14 7:32 ` Matthew Garrett
@ 2016-07-14 14:54 ` Daniel P. Berrange
2016-07-15 0:10 ` Matthew Garrett
1 sibling, 1 reply; 21+ messages in thread
From: Daniel P. Berrange @ 2016-07-14 14:54 UTC (permalink / raw)
To: Matthew Garrett; +Cc: qemu-devel
On Thu, Jun 23, 2016 at 04:36:59PM -0700, Matthew Garrett 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.
Will it be capable of workubg with edk2/OVMF/AVMF as well as SeaBIOS ?
> This version of the implementation depends on port io, but if there's interest I'll
> add mmio as well.
So I guess this is the reason you've only enabled it for x86_64. Since we're
inventing an entirely new type of device here, not copying existing hardware,
I think it'd desirable if we created one that was supported across arches,
particularly aarch64, since that's the new hotness. With the convergance
of both x86_64 and aarch64 to EFI, it'd be nice to aim for parity for this
trusted boot support too if practical.
> 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
> +@item measurements
> +@findex measurements
> +Redirect Print system measurements
> +ETEXI
> +
> + {
Thus since is just reporting info about a device, from a HMP POV,
it would fit better as an 'info' sub-command, eg 'info measurements'.
The QMP equivalent would be a 'query-measurements' command
> +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++) {
> + 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");
> + }
> +}
The preferred approach to supporting monitor commands these
days is to first define a schema for the information to be
output in qapi-schema.json. Then implement a QMP command
that returns an instance of the data structure you defined.
Finally the HMP command, would invoke the QMP command to
get the data to be printed.
For a fairly clear example of best practice, take a look at
these 2 commits:
commit dc3dd0d2bed6edf3b60041f31200c674348168e9
Author: Stefan Hajnoczi <stefanha@redhat.com>
Date: Thu Feb 27 11:48:42 2014 +0100
qmp: add query-iothreads command
commit 62313160cb5b6bdfbd77a063e94a5a7d25e59f2b
Author: Ting Wang <kathy.wangting@huawei.com>
Date: Fri Jun 26 16:07:13 2015 +0800
hmp: add info iothreads command
between them, they illustrate the various files to modify
and the current preferred implementation style for monitor
commands.
> 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);
'extend_data' is rather too generic a name, for expose across
QEMU. Just add a "measurements_" prefix for any exported methods
from the measurements device.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/misc: Add simple measurement hardware
2016-06-23 23:36 [Qemu-devel] [PATCH] " Matthew Garrett
@ 2016-07-14 7:32 ` Matthew Garrett
2016-07-14 14:54 ` Daniel P. Berrange
1 sibling, 0 replies; 21+ messages in thread
From: Matthew Garrett @ 2016-07-14 7:32 UTC (permalink / raw)
To: qemu-devel; +Cc: Matthew Garrett
Any feedback on this?
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH] hw/misc: Add simple measurement hardware
@ 2016-06-23 23:36 Matthew Garrett
2016-07-14 7:32 ` Matthew Garrett
2016-07-14 14:54 ` Daniel P. Berrange
0 siblings, 2 replies; 21+ messages in thread
From: Matthew Garrett @ 2016-06-23 23:36 UTC (permalink / raw)
To: qemu-devel; +Cc: Matthew Garrett
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>
---
default-configs/x86_64-softmmu.mak | 1 +
hmp-commands.hx | 13 +++
hw/core/loader.c | 12 +++
hw/i386/acpi-build.c | 24 ++++-
hw/misc/Makefile.objs | 1 +
hw/misc/measurements.c | 205 +++++++++++++++++++++++++++++++++++++
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(+), 1 deletion(-)
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
+@item measurements
+@findex 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);
+ }
+}
+
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..b148e27 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_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,
@@ -2569,7 +2591,7 @@ 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);
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..ec142b6
--- /dev/null
+++ b/hw/misc/measurements.c
@@ -0,0 +1,205 @@
+/*
+ * 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 "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];
+ 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();
+}
+
+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;
+}
+
+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);
+ 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);
+ if (ret < 0) {
+ return;
+ }
+
+ 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);
+ 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++) {
+ 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
^ permalink raw reply related [flat|nested] 21+ messages in thread
end of thread, other threads:[~2016-08-09 14:58 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-23 21:09 [Qemu-devel] [PATCH] hw/misc: Add simple measurement hardware Matthew Garrett
2016-07-15 11:29 ` Dr. David Alan Gilbert
2016-07-15 18:11 ` Stefan Berger
2016-07-18 21:26 ` Matthew Garrett
2016-07-18 23:40 ` Stefan Berger
2016-07-18 23:52 ` Matthew Garrett
2016-07-19 0:08 ` Stefan Berger
2016-07-19 0:39 ` Matthew Garrett
2016-07-19 0:46 ` Stefan Berger
2016-07-19 0:49 ` Matthew Garrett
2016-07-18 21:19 ` Matthew Garrett
2016-07-19 9:38 ` Dr. David Alan Gilbert
2016-08-05 23:17 ` [Qemu-devel] [PATCH V2] " Matthew Garrett
2016-08-06 2:53 ` Eric Blake
2016-08-06 3:56 ` Stefan Berger
2016-08-08 19:43 ` Matthew Garrett
2016-08-09 14:58 ` Stefan Berger
2016-06-23 23:36 [Qemu-devel] [PATCH] " Matthew Garrett
2016-07-14 7:32 ` Matthew Garrett
2016-07-14 14:54 ` Daniel P. Berrange
2016-07-15 0:10 ` Matthew Garrett
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.