Re: [Qemu-devel] [PATCH V14 2/7] Add TPM (frontend) hardware interface (TPM TIS) to Qemu

[Stefan Berger]

On 02/20/2012 03:51 AM, Michael S. Tsirkin wrote:
> On Wed, Dec 14, 2011 at 08:43:17AM -0500, Stefan Berger wrote:
> > This patch adds the main code of the TPM frontend driver, the TPM TIS
> > interface, to Qemu. The code is largely based on the previous implementation
> > for Xen but has been significantly extended to meet the standard's
> > requirements, such as the support for changing of localities and all the
> > functionality of the available flags.
> looks good overall
> some questions on erro handling and behaviour with
> a malicious guest, below.

[...]
> > +#ifndef RAISE_STS_IRQ
> > +
> > +# define TPM_TIS_INTERRUPTS_SUPPORTED (TPM_TIS_INT_LOCALITY_CHANGED | \
> > +                                       TPM_TIS_INT_DATA_AVAILABLE   | \
> > +                                       TPM_TIS_INT_COMMAND_READY)
> > +
> > +#else
> > +
> > +# define TPM_TIS_INTERRUPTS_SUPPORTED (TPM_TIS_INT_LOCALITY_CHANGED | \
> > +                                       TPM_TIS_INT_DATA_AVAILABLE   | \
> > +                                       TPM_TIS_INT_STS_VALID | \
> > +                                       TPM_TIS_INT_COMMAND_READY)
> > +
> let's keep these as #define

Will fix it.
> > +/* utility functions */
> > +
> > +static uint8_t tpm_tis_locality_from_addr(target_phys_addr_t addr)
> > +{
> > +    return (uint8_t)((addr>>  12)&  0x7);
> > +}
> > +
> note this can give a number 0..6

Yes, but that's ok. We are only registering the MMIO region [fed4 0000, 
fed4 5000[, so anything larger than 4 cannot actually come from this 
function.


> > +static void tpm_tis_new_active_locality(TPMState *s, uint8_t new_active_locty)
> > +{
> > +    TPMTISState *tis =&s->s.tis;
> > +    int change = (s->s.tis.active_locty != new_active_locty);
> > +
> > +    if (change&&  TPM_TIS_IS_VALID_LOCTY(s->s.tis.active_locty)) {
> > +        /* reset flags on the old active locality */
> > +        tis->loc[s->s.tis.active_locty].access&=
> > +                 ~(TPM_TIS_ACCESS_ACTIVE_LOCALITY|TPM_TIS_ACCESS_REQUEST_USE);
> > +        if (TPM_TIS_IS_VALID_LOCTY(new_active_locty)&&
> > +            tis->loc[new_active_locty].access&  TPM_TIS_ACCESS_SEIZE) {
> > +            tis->loc[tis->active_locty].access |= TPM_TIS_ACCESS_BEEN_SEIZED;
> > +        }
> > +    }
> > +
> > +    tis->active_locty = new_active_locty;
> should this happen with invalid locality?



> > + * read a byte of response data
> > + * call this with s->state_lock held
> > + */
> > +static uint32_t tpm_tis_data_read(TPMState *s, uint8_t locty)
> > +{
> > +    TPMTISState *tis =&s->s.tis;
> > +    uint32_t ret = TPM_TIS_NO_DATA_BYTE;
> > +    uint16_t len;
> > +
> > +    if ((tis->loc[locty].sts&  TPM_TIS_STS_DATA_AVAILABLE)) {
> > +        len = tpm_tis_get_size_from_buffer(&tis->loc[locty].r_buffer);
> > +
> > +        ret = tis->loc[locty].r_buffer.buffer[tis->loc[locty].r_offset++];
> what prevents this from being called when r_offset == len,
> with a malicious guest?

The device user has no control over the index of the byte stream he 
wants to read. Once he starts reading the bytes from the device starting 
at index 0, the index automatically increases, as seen above. The flag 
TPM_TIS_STS_DATA_AVAILABLE is set for as long as there are bytes 
available. The user can see this flag in the status register. Then once...

> > +        if (tis->loc[locty].r_offset>= len) {
> > +            /* got last byte */
> > +            tis->loc[locty].sts = TPM_TIS_STS_VALID;

... all bytes have been read the status flag changes and no more bytes 
can be read. The tis code here also goes by these flags as you can see.


> > +/*
> > + * Read a register of the TIS interface
> > + * See specs pages 33-63 for description of the registers
> > + */
> > +static uint64_t tpm_tis_mmio_read(void *opaque, target_phys_addr_t addr,
> > +                                  unsigned size)
> > +{
> > +    TPMState *s = opaque;
> > +    TPMTISState *tis =&s->s.tis;
> > +    uint16_t offset = addr&  0xffc;
> > +    uint8_t shift = (addr&  0x3) * 8;
> > +    uint32_t val = 0xffffffff;
> > +    uint8_t locty = tpm_tis_locality_from_addr(addr);
> Apparently locty can be 0..6 but loc array only has 5 entries.
> What if a guest accesses an invalid one?

See comment at the beginning. Any locality beyond value '4' cannot be 
reached since it's not within the MMIO region of the device.

> > +            /* allow seize if a locality is active and the requesting
> > +               locality is higher than the one that's active
> > +               OR
> > +               allow seize for requesting locality if no locality is
> > +               active */
> /*
>   * multiline comments should look like this
>   */

 I will fix 'them'.


> > +            while ((TPM_TIS_IS_VALID_LOCTY(tis->active_locty)&&
> > +                    locty>  tis->active_locty) ||
> > +                   (!TPM_TIS_IS_VALID_LOCTY(tis->active_locty))) {
> Clearer as:
> (locty>  tis->active_locty ||
> !TPM_TIS_IS_VALID_LOCTY(tis->active_locty))

Ok.

> > +static const MemoryRegionOps tpm_tis_memory_ops = {
> > +    .read = tpm_tis_mmio_read,
> > +    .write = tpm_tis_mmio_write,
> > +    .endianness = DEVICE_NATIVE_ENDIAN,
> Are you sure? Most devices are BIG or LITTLE.

Right, it should be BIG endian. I'll fix it. Luckily there are only a 
few 32bit accesses (and no 16bit access) also in SeaBIOS and those that 
did accessed it via 32 bit didn't fail due to the mix-up (excluding the 
test suite).

> > +    .valid = {
> > +        .min_access_size = 1,
> > +        .max_access_size = 4,
> > +    },
> > +};
> > +
> > +static int tpm_tis_do_startup_tpm(TPMState *s)
> > +{
> > +    return s->be_driver->ops->startup_tpm(s->be_driver);
> > +}
> > +
> > +/*
> > + * This function is called when the machine starts, resets or due to
> > + * S3 resume.
> > + */
> > +static void tpm_tis_reset(DeviceState *d)
> > +{
> > +    TPMState *s = container_of(d, TPMState, busdev.qdev);
> > +    TPMTISState *tis =&s->s.tis;
> > +    int c;
> > +
> > +    s->tpm_initialized = false;
> > +
> > +    s->be_driver->ops->reset(s->be_driver);
> > +
> > +    tis->active_locty = TPM_TIS_NO_LOCALITY;
> > +    tis->next_locty = TPM_TIS_NO_LOCALITY;
> > +    tis->aborting_locty = TPM_TIS_NO_LOCALITY;
> > +
> > +    for (c = 0; c<  TPM_TIS_NUM_LOCALITIES; c++) {
> > +        tis->loc[c].access = TPM_TIS_ACCESS_TPM_REG_VALID_STS;
> > +        tis->loc[c].sts = 0;
> > +        tis->loc[c].inte = (1<<  3);
> > +        tis->loc[c].ints = 0;
> > +        tis->loc[c].status = TPM_TIS_STATUS_IDLE;
> > +
> > +        tis->loc[c].w_offset = 0;
> > +        s->be_driver->ops->realloc_buffer(&tis->loc[c].w_buffer);
> > +        tis->loc[c].r_offset = 0;
> > +        s->be_driver->ops->realloc_buffer(&tis->loc[c].r_buffer);
> > +    }
> > +
> > +    tpm_tis_do_startup_tpm(s);
> > +}
> > +
> > +static int tpm_tis_init(ISADevice *dev)
> > +{
> > +    TPMState *s = DO_UPCAST(TPMState, busdev, dev);
> Let's stay consistent DO_UPCAST versus container_of.

Ok.

> > +    TPMTISState *tis =&s->s.tis;
> > +    int rc;
> > +
> > +    qemu_mutex_init(&s->state_lock);
> > +    qemu_cond_init(&s->from_tpm_cond);
> > +    qemu_cond_init(&s->to_tpm_cond);
> > +
> > +    s->be_driver = qemu_find_tpm(s->backend);
> > +    if (!s->be_driver) {
> > +        error_report("tpm_tis: backend driver with id %s could not be "
> > +                     "found.n\n", s->backend);
> > +        return -1;
> goto forward to have a single exit point?

Will fix it.

> > +    }
> > +
> > +    s->be_driver->fe_model = "tpm-tis";
> > +
> > +    if (s->be_driver->ops->init(s->be_driver, s, tpm_tis_receive_cb)) {
> > +        return -1;
> goto forward?
>
> > +    }
> > +
> > +    isa_init_irq(dev,&tis->irq, tis->irq_num);
> probably should validate irq_num so an illegal value
> gives us an error not an assert.

Ok.

> > +
> > +    memory_region_init_io(&s->mmio,&tpm_tis_memory_ops, s, "tpm-tis-mmio",
> > +                          0x1000 * TPM_TIS_NUM_LOCALITIES);
> > +    memory_region_add_subregion(get_system_memory(), TPM_TIS_ADDR_BASE,
> > +&s->mmio);
> > +
> > +    rc = tpm_tis_do_startup_tpm(s);
> > +    if (rc != 0) {
> > +        goto err_destroy_memory;
> > +    }
> > +
> > +    return 0;
> > +
> > + err_destroy_memory:
> You must delete mmio as subregion before destroy.

Will fix it.

> > +    memory_region_destroy(&s->mmio);
> > +
> > +    return -1;
> > +}
> > +
> > +static const VMStateDescription vmstate_tpm_tis = {
> > +    .name = "tpm",
> > +    .unmigratable = 1,
> I think the passthrough backend is what makes
> it unmigrateable, no?
> If so in the future it would be better to get that info from backend.
> For now I guess we are ok as is, but pls add a comment.

In the patches following this passthrough driver I am pushing this into 
the backend where I then register a migration blocker. While only the 
passthrough driver is here, this should be fine.

> > +};
> > +
> > +static ISADeviceInfo tpm_tis_device_info = {
> > +    .init         = tpm_tis_init,
> > +    .qdev.name    = "tpm-tis",
> > +    .qdev.size    = sizeof(TPMState),
> > +    .qdev.vmsd    =&vmstate_tpm_tis,
> > +    .qdev.reset   = tpm_tis_reset,
> > +    .qdev.props = (Property[]) {
> > +        DEFINE_PROP_UINT32("irq", TPMState,
> > +                           s.tis.irq_num, TPM_TIS_IRQ),
> > +        DEFINE_PROP_STRING("tpmdev", TPMState, backend),
> > +        DEFINE_PROP_END_OF_LIST(),
> > +    },
> > +};
> > +
> > +static void tpm_tis_register_device(void)
> > +{
> > +    isa_qdev_register(&tpm_tis_device_info);
> So this needs to be rebased to the QOM.

I followed the tree.

> Also a question: looking at the linux drivers
> there's pnp support but we don't have
> that yet, right? So you need to force
> non-pnp mode in guest?

You only need to force non-pnp mode in the guest if you don't have 
SeaBIOS with the TPM support. There I create the SSDT for the TPM device 
that then triggers the pnp driver mode inside Linux.


Thanks for the review.

   Stefan