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

[Matthew Garrett]

On Fri, Jul 15, 2016 at 4:29 AM, Dr. David Alan Gilbert <address@hidden
> wrote:

> * Matthew Garrett (address@hidden) 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 / address@hidden / Manchester, UK
>