From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48817) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YiPcf-0006JF-DV for qemu-devel@nongnu.org; Wed, 15 Apr 2015 11:53:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YiPcb-0005UB-CU for qemu-devel@nongnu.org; Wed, 15 Apr 2015 11:53:05 -0400 Received: from emvm-gh1-uea08.nsa.gov ([63.239.67.9]:60981) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YiPcb-0005Tv-7H for qemu-devel@nongnu.org; Wed, 15 Apr 2015 11:53:01 -0400 Message-ID: <552E7E98.3020206@tycho.nsa.gov> Date: Wed, 15 Apr 2015 11:07:04 -0400 From: Daniel De Graaf MIME-Version: 1.0 References: <1428649159-30879-1-git-send-email-quan.xu@intel.com> <1428649159-30879-4-git-send-email-quan.xu@intel.com> <552E7950.7030806@linux.vnet.ibm.com> In-Reply-To: <552E7950.7030806@linux.vnet.ibm.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 3/6] Qemu-Xen-vTPM: Xen frontend driver infrastructure List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Berger , Quan Xu Cc: wei.liu2@citrix.com, stefano.stabellini@eu.citrix.com, qemu-devel@nongnu.org, xen-devel@lists.xen.org, aliguori@amazon.com, pbonzini@redhat.com 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel De Graaf Subject: Re: [Qemu-devel] [PATCH v5 3/6] Qemu-Xen-vTPM: Xen frontend driver infrastructure Date: Wed, 15 Apr 2015 11:07:04 -0400 Message-ID: <552E7E98.3020206@tycho.nsa.gov> References: <1428649159-30879-1-git-send-email-quan.xu@intel.com> <1428649159-30879-4-git-send-email-quan.xu@intel.com> <552E7950.7030806@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <552E7950.7030806@linux.vnet.ibm.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefan Berger , Quan Xu Cc: wei.liu2@citrix.com, stefano.stabellini@eu.citrix.com, qemu-devel@nongnu.org, xen-devel@lists.xen.org, aliguori@amazon.com, pbonzini@redhat.com, eblake@redhat.com List-Id: xen-devel@lists.xenproject.org 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