[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 3/6] Qemu-Xen-vTPM: Xen frontend driver infra
From: |
Xu, Quan |
Subject: |
Re: [Qemu-devel] [PATCH v5 3/6] Qemu-Xen-vTPM: Xen frontend driver infrastructure |
Date: |
Thu, 16 Apr 2015 01:03:22 +0000 |
> -----Original Message-----
> From: Daniel De Graaf [mailto:address@hidden
> Sent: Wednesday, April 15, 2015 11:07 PM
> To: Stefan Berger; Xu, Quan
> Cc: address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden;
> address@hidden
> 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
- [Qemu-devel] [PATCH v5 0/6] QEMU:Xen stubdom vTPM for HVM virtual machine(QEMU patch), Quan Xu, 2015/04/10
- [Qemu-devel] [PATCH v5 2/6] Qemu-Xen-vTPM: Xen frontend driver infrastructure, Quan Xu, 2015/04/10
- [Qemu-devel] [PATCH v5 4/6] Qemu-Xen-vTPM: Qemu vTPM xenstubdoms backen., Quan Xu, 2015/04/10
- [Qemu-devel] [PATCH v5 6/6] Qemu-Xen-vTPM: Add a parameter indicating whether the command that was a selftest, Quan Xu, 2015/04/10
- Re: [Qemu-devel] [PATCH v5 6/6] Qemu-Xen-vTPM: Add a parameter indicating whether the command that was a selftest, Stefan Berger, 2015/04/13