[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V3] hw/misc: Add simple measurement hardware
From: |
Matthew Garrett |
Subject: |
Re: [Qemu-devel] [PATCH V3] hw/misc: Add simple measurement hardware |
Date: |
Tue, 16 Aug 2016 15:13:50 -0700 |
On Fri, Aug 12, 2016 at 10:59 AM, Dr. David Alan Gilbert
<address@hidden> wrote:
> * Matthew Garrett (address@hidden) 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.