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@lists.xen.org
http://lists.xen.org/xen-devel
