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

[Matthew Garrett]

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.