From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v2] Coverity ID: 1215178 Date: Thu, 22 May 2014 16:10:57 +0100 Message-ID: <537E1381.9020700@citrix.com> References: <1400771199-4850-1-git-send-email-paul.durrant@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1400771199-4850-1-git-send-email-paul.durrant@citrix.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: Paul Durrant Cc: Keir Fraser , Jan Beulich , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 22/05/14 16:06, Paul Durrant wrote: > There are two problems with initializetion of the ioreq_t in hvmemul_do_io(): > > - vp_eport is uninitialized (because it doesn't need to be) but because the > struct is the subject of a copy in hvm_send_assist_req(), this is flagged > as a problem. > - dir, addr, data_is_ptr, and data may be uninitialized when the struct is > passed to hvmtrace_io_assist(). This is clearly a bug, so the initializ- > ation of at least those fields needs to be moved earlier. > > This patch fixes both these problems. > > Signed-off-by: Paul Durrant > Cc: Keir Fraser > Cc: Jan Beulich Reviewed-by: Andrew Cooper > --- > xen/arch/x86/hvm/emulate.c | 29 ++++++++++++++--------------- > 1 file changed, 14 insertions(+), 15 deletions(-) > > diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c > index 904c71a..eac159f 100644 > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -53,11 +53,17 @@ static int hvmemul_do_io( > int is_mmio, paddr_t addr, unsigned long *reps, int size, > paddr_t ram_gpa, int dir, int df, void *p_data) > { > - paddr_t value = ram_gpa; > - int value_is_ptr = (p_data == NULL); > struct vcpu *curr = current; > struct hvm_vcpu_io *vio; > - ioreq_t p; > + ioreq_t p = { > + .type = is_mmio ? IOREQ_TYPE_COPY : IOREQ_TYPE_PIO, > + .addr = addr, > + .size = size, > + .dir = dir, > + .df = df, > + .data = ram_gpa, > + .data_is_ptr = (p_data == NULL), > + }; > unsigned long ram_gfn = paddr_to_pfn(ram_gpa); > p2m_type_t p2mt; > struct page_info *ram_page; > @@ -94,15 +100,15 @@ static int hvmemul_do_io( > return X86EMUL_UNHANDLEABLE; > } > > - if ( (p_data != NULL) && (dir == IOREQ_WRITE) ) > + if ( !p.data_is_ptr && (dir == IOREQ_WRITE) ) > { > - memcpy(&value, p_data, size); > + memcpy(&p.data, p_data, size); > p_data = NULL; > } > > vio = &curr->arch.hvm_vcpu.hvm_io; > > - if ( is_mmio && !value_is_ptr ) > + if ( is_mmio && !p.data_is_ptr ) > { > /* Part of a multi-cycle read or write? */ > if ( dir == IOREQ_WRITE ) > @@ -146,7 +152,7 @@ static int hvmemul_do_io( > goto finish_access; > case HVMIO_dispatched: > /* May have to wait for previous cycle of a multi-write to complete. */ > - if ( is_mmio && !value_is_ptr && (dir == IOREQ_WRITE) && > + if ( is_mmio && !p.data_is_ptr && (dir == IOREQ_WRITE) && > (addr == (vio->mmio_large_write_pa + > vio->mmio_large_write_bytes)) ) > { > @@ -179,14 +185,7 @@ static int hvmemul_do_io( > if ( vio->mmio_retrying ) > *reps = 1; > > - p.dir = dir; > - p.data_is_ptr = value_is_ptr; > - p.type = is_mmio ? IOREQ_TYPE_COPY : IOREQ_TYPE_PIO; > - p.size = size; > - p.addr = addr; > p.count = *reps; > - p.df = df; > - p.data = value; > > if ( dir == IOREQ_WRITE ) > hvmtrace_io_assist(is_mmio, &p); > @@ -251,7 +250,7 @@ static int hvmemul_do_io( > if ( p_data != NULL ) > memcpy(p_data, &vio->io_data, size); > > - if ( is_mmio && !value_is_ptr ) > + if ( is_mmio && !p.data_is_ptr ) > { > /* Part of a multi-cycle read or write? */ > if ( dir == IOREQ_WRITE )