Re: [Qemu-devel] [PATCH v5 3/6] Qemu-Xen-vTPM: Xen frontend driver infrastructure

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

On 04/15/2015 10:44 AM, Stefan Berger wrote:
> On 04/10/2015 02:59 AM, Quan Xu wrote:
>> This patch adds infrastructure for xen front drivers living in qemu,
>> so drivers don't need to implement common stuff on their own.  It's
>> mostly xenbus management stuff: some functions to access XenStore,
>> setting up XenStore watches, callbacks on device discovery and state
>> changes, and handle event channel between the virtual machines.
>>
[...]
>> +int vtpm_recv(struct XenDevice *xendev, uint8_t* buf, uint32_t buf_size,
>> +              size_t *count)
>> +{
>> +    struct xen_vtpm_dev *vtpmdev = container_of(xendev, struct xen_vtpm_dev,
>> +                                                xendev);
>> +    struct tpmif_shared_page *shr = vtpmdev->shr;
>> +    unsigned int offset;
>> +    size_t length = shr->length;
>> +
>> +    if (shr->state == TPMIF_STATE_IDLE) {
>> +        return -ECANCELED;
>> +    }
>> +
>> +    offset = sizeof(*shr) + sizeof(shr->extra_pages[0])*shr->nr_extra_pages;
>
> offset now points to where the TPM response starts, right?

Yes.

>> +    if (offset > buf_size) {
>
> You are comparing offset as if it was the size of the TPM response, but that's not what it is as far as I understand this.
>
> I would have thought that shr->length indicates the size of the TPM response that starts at offset.
> So then you should only have to ensure that shr->length <= buf_size and never copy more than buf_size bytes to buf.
>
> Similar comments to vtpm_send.

No, this check needs to remain (on both send and recv), but buf_size should
be replaced by PAGE_SIZE.  This prevents an incorrectly large value for
nr_extra_pages from causing the packet to be located outside of the shared
page, resulting in TPM packets being written to some random heap address.

>> +        return -EIO;
>> +    }
>> +
>> +    if (offset + length > buf_size) {
>> +        length = buf_size - offset;
>> +    }

This check also needs to be against PAGE_SIZE.

>> +
>> +    if (length > *count) {
>> +        length = *count;
>> +    }

Is *count even valid here?  I would have assumed it was a purely
out parameter, with buf_size as the maximum value it can be assigned.

>> +
>> +    memcpy(buf, offset + (uint8_t *)shr, shr->length);
>
> use length rather than shr->length otherwise length goes unused.

Agreed; the values from the shared page should not be read more than
once, because an uncooperative peer could end up changing them.

-- 
Daniel De Graaf
National Security Agency