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

["Xu, Quan" <quan.xu@xxxxxxxxx>]

> -----Original Message-----
> From: Daniel De Graaf [mailto:dgdegra@xxxxxxxxxxxxx]
> Sent: Wednesday, April 15, 2015 11:07 PM
> To: Stefan Berger; Xu, Quan
> Cc: stefano.stabellini@xxxxxxxxxxxxx; eblake@xxxxxxxxxx; wei.liu2@xxxxxxxxxx;
> qemu-devel@xxxxxxxxxx; xen-devel@xxxxxxxxxxxxx; aliguori@xxxxxxxxxx;
> pbonzini@xxxxxxxxxx
> Subject: Re: [Qemu-devel] [PATCH v5 3/6] Qemu-Xen-vTPM: Xen frontend driver
> infrastructure
> 
> 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.
> 


Thanks Deniel and Stefan.
I will replace buf_size with PAGE_SIZE in next version.


> >> +        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.

replaced by PAGE_SIZE?

> 
> >> +
> >> +    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.
> 

Agreed.

Quan Xu
Intel

> --
> Daniel De Graaf
> National Security Agency

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