On Fri, Aug 12, 2016 at 10:59 AM, Dr. David Alan Gilbert <dgilb...@redhat.com> wrote: > * Matthew Garrett (mj...@coreos.com) 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. > > Do you have SeaBIOS or kernel patches/repos so that people can get a feel > how you're using it? You could add links here.
https://github.com/mjg59/seabios has my current hacky implementation. > (I also still don't quite get how an application interrogating the guest, > over say a net connection to a guest application could pass a nonce to > the hypervisor that's going to sign the measurements). Amazon provide a local API endpoint to the guest, so the flow would be something like: * Remote site requests attestation, passes nonce * Guest makes API call over local API endpoint, passing nonce * Hypervisor environment extracts measurements from qemu, appends nonce, signs with hypervisor-held key * Hypervisor passes signed blob back to guest * Guest passes signed blob back to remote site >> + if (err == NULL) { >> + for (info = info_list; info; info = info->next) { >> + monitor_printf(mon, "%02ld: %s\n", info->value->pcr, >> + info->value->hash); > > I think that's "%02" PRId64 ":%s\n" (Which I hate but that's portable). Ok. >> +#define DIGEST_SIZE 20 >> +#define PCR_COUNT 24 > > The names feel like they might name clash at some point; just a feeling. I'll prefix them. >> +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; > > OK, just on principal; why are those not unsigned? > Also, since you're migrating them, pick a size so > they're stable on the wire. uint32_t you could > use since that's what you're migrating them as. Makes sense. >> +static void measurement_select(void *opaque, hwaddr addr, uint64_t val, >> unsigned size) >> +{ >> + MeasurementState *s = MEASUREMENT(opaque); >> + >> + if (val > PCR_COUNT) > > Shouldn't that be val >= PCR_COUNT ? Yup. One day I'll learn how arrays work. >> + Error *err; >> + char tmpbuf[40]; > > Why not 2 * DIGEST_SIZE, and may as well be uint8_t ? Ok. >> + 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) { > > magic 40 again. Fixed. >> + 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); > > Does: > > error_reportf_err(err, "Failed to measure data:"); > > do the same as those 3 lines? Looks like - I'd missed that one. >> +static void log_data(MeasurementState *s, int pcrnum, uint8_t *hash, char >> *description) >> +{ > ^ it's uint32_t and uint8_t? > Pick one and stick to it. Yeah. >> + 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); > > What stops this from over running the size of the log? Added a check for that. > (Also are there any alignment restrictions etc that you might hit > on some platforms?) The log is allocated by platform-specific code, so I think that should already be taken care of? >> + logentry->pcrindex = pcrnum; >> + logentry->eventtype = 1; > Magic 1. Fixed. >> + memcpy(logentry->digest, hash, DIGEST_SIZE); >> + logentry->eventdatasize = eventlen; >> + memcpy(logentry->event, description, eventlen); >> + >> + s->logsize += entrylen; >> +} > > (Just a sanity check for me - this is data written into an acpi table that > will get > into guest RAM eventually, but isn't yet?) That's my understanding. >> +static const VMStateDescription measurement_state = { >> + .name = "measurements", >> + .version_id = 1, >> + .minimum_version_id = 1, >> + .fields = (VMStateField[]) { >> + VMSTATE_UINT16(iobase, MeasurementState), > > I don't think you need to preserve the iobase, because it's just > fixed state that comes from the command line; it'll have been set > on the destination by the command line as well. True. >> + VMSTATE_BUFFER_UNSAFE(measurements, MeasurementState, 0, PCR_COUNT >> * DIGEST_SIZE), > > How about a VMSTAET_UINT8_2DARRAY - then it's all nice and safe and type > checked? That ought to work, I was just unaware of it. >> + VMSTATE_BUFFER(tmpmeasurement, MeasurementState), >> + VMSTATE_INT32(write_count, MeasurementState), >> + VMSTATE_INT32(read_count, MeasurementState), >> + VMSTATE_UINT8(pcr, MeasurementState), > > You might want to add a .post_load that sanity checks > write_count, read_count and pcr; an evil person could modify > a migration/snapshot stream and then put in index values that > let you write off the end of your arrays. > (We've probably got places that aren't careful about it, but > we're slowly trying to make sure they're checked). Good plan.