qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Dr. David Alan Gilbert
Subject: Re: [Qemu-devel] [PATCH] hw/misc: Add simple measurement hardware
Date: Tue, 19 Jul 2016 10:38:44 +0100
User-agent: Mutt/1.6.1 (2016-04-27)

* Matthew Garrett (address@hidden) wrote:
> 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)

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 / address@hidden / Manchester, UK
> >
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK



reply via email to

[Prev in Thread] Current Thread [Next in Thread]