From mboxrd@z Thu Jan 1 00:00:00 1970 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 Message-ID: <945CA011AD5F084CBEA3E851C0AB28890E8E086B__29210.0916420624$1429146344$gmane$org@SHSMSX101.ccr.corp.intel.com> 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> <552E7E98.3020206@tycho.nsa.gov> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <552E7E98.3020206@tycho.nsa.gov> Content-Language: en-US List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Daniel De Graaf , Stefan Berger 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 > -----Original Message----- > From: Daniel De Graaf [mailto:dgdegra@tycho.nsa.gov] > Sent: Wednesday, April 15, 2015 11:07 PM > To: Stefan Berger; Xu, Quan > Cc: stefano.stabellini@eu.citrix.com; eblake@redhat.com; wei.liu2@citrix.com; > qemu-devel@nongnu.org; xen-devel@lists.xen.org; aliguori@amazon.com; > pbonzini@redhat.com > 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