Re: [Xen-devel] [PATCH] Initialize ioreq_t in hvmemul_do_io()

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 22 May 2014 14:47
> To: Jan Beulich
> Cc: Paul Durrant; xen-devel@xxxxxxxxxxxxx; Keir (Xen.org)
> Subject: Re: [PATCH] Initialize ioreq_t in hvmemul_do_io()
> 
> On 22/05/14 14:27, Jan Beulich wrote:
> >>>> On 22.05.14 at 15:20, <Paul.Durrant@xxxxxxxxxx> wrote:
> >>>  -----Original Message-----
> >>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >>> Sent: 22 May 2014 14:14
> >>> To: Andrew Cooper; Paul Durrant
> >>> Cc: xen-devel@xxxxxxxxxxxxx; Keir (Xen.org)
> >>> Subject: Re: [PATCH] Initialize ioreq_t in hvmemul_do_io()
> >>>
> >>>>>> On 22.05.14 at 14:48, <paul.durrant@xxxxxxxxxx> wrote:
> >>>> All relevant fields of the ioreq_t are properly initialized but coverity
> >>>> complains about vp_eport being uninitialized (ID 1215178) because it is
> >>>> the subject of a struct copy in hvm_send_assist_req().
> >>>>
> >>>> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> >>>> Cc: Keir Fraser <keir@xxxxxxx>
> >>>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> >>>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >>>> ---
> >>>>  xen/arch/x86/hvm/emulate.c |    2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/xen/arch/x86/hvm/emulate.c
> b/xen/arch/x86/hvm/emulate.c
> >>>> index 904c71a..e516194 100644
> >>>> --- a/xen/arch/x86/hvm/emulate.c
> >>>> +++ b/xen/arch/x86/hvm/emulate.c
> >>>> @@ -57,7 +57,7 @@ static int hvmemul_do_io(
> >>>>      int value_is_ptr = (p_data == NULL);
> >>>>      struct vcpu *curr = current;
> >>>>      struct hvm_vcpu_io *vio;
> >>>> -    ioreq_t p;
> >>>> +    ioreq_t p = { 0 };
> >>> To be honest, I dislike fixes like this to address tool shortcomings. It's
> >>> not like this is giaramteed to be just a single instruction that gets
> >>> added (maybe the compiler can figure this, but I would want to rely
> >>> on it.
> >>>
> >> I could always change hvm_send_assist_req() to only copy the relevant
> fields
> >> from the proto structure, as it used to in earlier versions of my patch.
> > But the structure copy is certainly more efficient than the piece-wise
> > copying.
> >
> > Jan
> >
> 
> When interpreting the coverity results, I accidentally missed the second
> occurrence of this issue.
> 
> It is possible to "goto finished_access" and have p.addr and p.dir
> uninitialised when tracing the IO.
> 

Unfortunately the ioreq passed to hvmtrace_io_assist() is just bogus in this 
case. I need to fix that too.

  Paul

> By far the safest option is just initialise the entire structure to 0.
> 
> ~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel