Re: [PATCH] Initialize ioreq_t in hvmemul_do_io()

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 22/05/14 14:27, Jan Beulich wrote:
>>>> On 22.05.14 at 15:20, <Paul.Durrant@citrix.com> wrote:
>>>  -----Original Message-----
>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>> Sent: 22 May 2014 14:14
>>> To: Andrew Cooper; Paul Durrant
>>> Cc: xen-devel@lists.xen.org; Keir (Xen.org)
>>> Subject: Re: [PATCH] Initialize ioreq_t in hvmemul_do_io()
>>>
>>>>>> On 22.05.14 at 14:48, <paul.durrant@citrix.com> 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@citrix.com>
>>>> Cc: Keir Fraser <keir@xen.org>
>>>> Cc: Jan Beulich <jbeulich@suse.com>
>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> ---
>>>>  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.

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

~Andrew