[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [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.

+        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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.