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

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel