[PATCH] x86/hvm_event: fix uninitialized struct field usage introduced by c/s f5365e6

[PATCH] x86/hvm_event: fix uninitialized struct field usage introduced by c/s f5365e6

[Corneliu ZUZU]

c/s f5365e6: "xen/vm-events: Move parts of monitor_domctl code to common-side",
introduced a use without initialization issue.
hvm_event_breakpoint calls hvm_event_traps(&req) and if sync is true that
ors some bits into req->flags which was never initialised.
Reported by Coverity Scan.

Initializes req @ hvm_event_breakpoint entry.

Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
 xen/arch/x86/hvm/event.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index 874a36c..cb9c152 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -173,7 +173,7 @@ int hvm_event_breakpoint(unsigned long rip,
 {
     struct vcpu *curr = current;
     struct arch_domain *ad = &curr->domain->arch;
-    vm_event_request_t req;
+    vm_event_request_t req = {};
 
     switch ( type )
     {
-- 
2.5.0

Re: [PATCH] x86/hvm_event: fix uninitialized struct field usage introduced by c/s f5365e6

[Razvan Cojocaru]

On 02/18/2016 12:45 PM, Corneliu ZUZU wrote:
> c/s f5365e6: "xen/vm-events: Move parts of monitor_domctl code to common-side",
> introduced a use without initialization issue.
> hvm_event_breakpoint calls hvm_event_traps(&req) and if sync is true that
> ors some bits into req->flags which was never initialised.
> Reported by Coverity Scan.
> 
> Initializes req @ hvm_event_breakpoint entry.
> 
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
> ---
>  xen/arch/x86/hvm/event.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
> index 874a36c..cb9c152 100644
> --- a/xen/arch/x86/hvm/event.c
> +++ b/xen/arch/x86/hvm/event.c
> @@ -173,7 +173,7 @@ int hvm_event_breakpoint(unsigned long rip,
>  {
>      struct vcpu *curr = current;
>      struct arch_domain *ad = &curr->domain->arch;
> -    vm_event_request_t req;
> +    vm_event_request_t req = {};
>  
>      switch ( type )
>      {

Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

Re: [PATCH] x86/hvm_event: fix uninitialized struct field usage introduced by c/s f5365e6

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 1333 bytes --]

On Feb 18, 2016 03:46, "Razvan Cojocaru" <rcojocaru@bitdefender.com> wrote:
>
> On 02/18/2016 12:45 PM, Corneliu ZUZU wrote:
> > c/s f5365e6: "xen/vm-events: Move parts of monitor_domctl code to
common-side",
> > introduced a use without initialization issue.
> > hvm_event_breakpoint calls hvm_event_traps(&req) and if sync is true
that
> > ors some bits into req->flags which was never initialised.
> > Reported by Coverity Scan.
> >
> > Initializes req @ hvm_event_breakpoint entry.
> >
> > Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
> > ---
> >  xen/arch/x86/hvm/event.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
> > index 874a36c..cb9c152 100644
> > --- a/xen/arch/x86/hvm/event.c
> > +++ b/xen/arch/x86/hvm/event.c
> > @@ -173,7 +173,7 @@ int hvm_event_breakpoint(unsigned long rip,
> >  {
> >      struct vcpu *curr = current;
> >      struct arch_domain *ad = &curr->domain->arch;
> > -    vm_event_request_t req;
> > +    vm_event_request_t req = {};

Should this be = { 0 } instead? Also, as I recall the request is not
initialized on any of the paths, so we might as well do it for all of them,
not just here. It would help avoid the listener erronously using some
fields that were not actually initialized as well.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 1818 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] x86/hvm_event: fix uninitialized struct field usage introduced by c/s f5365e6

[Razvan Cojocaru]

On 02/18/2016 04:10 PM, Tamas K Lengyel wrote:
> 
> On Feb 18, 2016 03:46, "Razvan Cojocaru" <rcojocaru@bitdefender.com
> <mailto:rcojocaru@bitdefender.com>> wrote:
>>
>> On 02/18/2016 12:45 PM, Corneliu ZUZU wrote:
>> > c/s f5365e6: "xen/vm-events: Move parts of monitor_domctl code to
> common-side",
>> > introduced a use without initialization issue.
>> > hvm_event_breakpoint calls hvm_event_traps(&req) and if sync is true
> that
>> > ors some bits into req->flags which was never initialised.
>> > Reported by Coverity Scan.
>> >
>> > Initializes req @ hvm_event_breakpoint entry.
>> >
>> > Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com
> <mailto:czuzu@bitdefender.com>>
>> > ---
>> >  xen/arch/x86/hvm/event.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
>> > index 874a36c..cb9c152 100644
>> > --- a/xen/arch/x86/hvm/event.c
>> > +++ b/xen/arch/x86/hvm/event.c
>> > @@ -173,7 +173,7 @@ int hvm_event_breakpoint(unsigned long rip,
>> >  {
>> >      struct vcpu *curr = current;
>> >      struct arch_domain *ad = &curr->domain->arch;
>> > -    vm_event_request_t req;
>> > +    vm_event_request_t req = {};
> 
> Should this be = { 0 } instead? Also, as I recall the request is not
> initialized on any of the paths, so we might as well do it for all of
> them, not just here. It would help avoid the listener erronously using
> some fields that were not actually initialized as well.

That should achieve the same thing. I first looked up "= {}" in the Xen
source code and found:

$ grep -R "= {}"
arch/arm/domain_build.c: struct kernel_info kinfo = {};
arch/x86/time.c: struct vcpu_time_info *u, _u = {};
arch/x86/tboot.c: uint8_t nonce[16] = {};
arch/x86/tboot.c: uint8_t nonce[16] = {};
arch/x86/tboot.c: uint8_t nonce[16] = {};
arch/x86/traps.c: unsigned char insns_before[8] = {}, insns_after[16] = {};
arch/x86/x86_emulate/x86_emulate.c: union vex vex = {};
arch/x86/x86_emulate/x86_emulate.c: struct x86_emulate_stub stub = {};
arch/x86/cpu/mtrr/generic.c:struct mtrr_state mtrr_state = {};
arch/x86/cpu/common.c:const struct cpu_dev *__read_mostly
cpu_devs[X86_VENDOR_NUM] = {};
arch/x86/domain.c: unsigned long total[MASK_EXTR(PGT_type_mask,
PGT_type_mask) + 1] = {};
tools/kconfig/expr.c: union string_value lval = {}, rval = {};
common/grant_table.c: struct gnttab_copy_buf src = {};
common/grant_table.c: struct gnttab_copy_buf dest = {};

So I thought that that's the prevailing convention. I've now searched
for "= {0}", and I see that that has also been done, so I suppose either
way is fine as far as the coding style goes?

$ grep -R "= {0}"
arch/arm/mm.c:    lpae_t pte = {0};
arch/arm/mm.c:    lpae_t pte = {0};
drivers/char/exynos4210-uart.c:} exynos4210_com = {0};
drivers/char/pl011.c:} pl011_com = {0};
drivers/char/omap-uart.c:} omap_com = {0};
drivers/char/scif-uart.c:} scif_com = {0};
drivers/char/cadence-uart.c:} cuart_com = {0};
tools/kconfig/nconf.gui.c:attributes_t attributes[ATTR_MAX+1] = {0};
crypto/vmac.c:    uint64_t in[2] = {0}, out[2];


Thanks,
Razvan

Re: [PATCH] x86/hvm_event: fix uninitialized struct field usage introduced by c/s f5365e6

[Andrew Cooper]

On 18/02/16 14:16, Razvan Cojocaru wrote:
> On 02/18/2016 04:10 PM, Tamas K Lengyel wrote:
>> On Feb 18, 2016 03:46, "Razvan Cojocaru" <rcojocaru@bitdefender.com
>> <mailto:rcojocaru@bitdefender.com>> wrote:
>>> On 02/18/2016 12:45 PM, Corneliu ZUZU wrote:
>>>> c/s f5365e6: "xen/vm-events: Move parts of monitor_domctl code to
>> common-side",
>>>> introduced a use without initialization issue.
>>>> hvm_event_breakpoint calls hvm_event_traps(&req) and if sync is true
>> that
>>>> ors some bits into req->flags which was never initialised.
>>>> Reported by Coverity Scan.
>>>>
>>>> Initializes req @ hvm_event_breakpoint entry.
>>>>
>>>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com
>> <mailto:czuzu@bitdefender.com>>
>>>> ---
>>>>  xen/arch/x86/hvm/event.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
>>>> index 874a36c..cb9c152 100644
>>>> --- a/xen/arch/x86/hvm/event.c
>>>> +++ b/xen/arch/x86/hvm/event.c
>>>> @@ -173,7 +173,7 @@ int hvm_event_breakpoint(unsigned long rip,
>>>>  {
>>>>      struct vcpu *curr = current;
>>>>      struct arch_domain *ad = &curr->domain->arch;
>>>> -    vm_event_request_t req;
>>>> +    vm_event_request_t req = {};
>> Should this be = { 0 } instead? Also, as I recall the request is not
>> initialized on any of the paths, so we might as well do it for all of
>> them, not just here. It would help avoid the listener erronously using
>> some fields that were not actually initialized as well.
> That should achieve the same thing. I first looked up "= {}" in the Xen
> source code and found:
>
> $ grep -R "= {}"
> arch/arm/domain_build.c: struct kernel_info kinfo = {};
> arch/x86/time.c: struct vcpu_time_info *u, _u = {};
> arch/x86/tboot.c: uint8_t nonce[16] = {};
> arch/x86/tboot.c: uint8_t nonce[16] = {};
> arch/x86/tboot.c: uint8_t nonce[16] = {};
> arch/x86/traps.c: unsigned char insns_before[8] = {}, insns_after[16] = {};
> arch/x86/x86_emulate/x86_emulate.c: union vex vex = {};
> arch/x86/x86_emulate/x86_emulate.c: struct x86_emulate_stub stub = {};
> arch/x86/cpu/mtrr/generic.c:struct mtrr_state mtrr_state = {};
> arch/x86/cpu/common.c:const struct cpu_dev *__read_mostly
> cpu_devs[X86_VENDOR_NUM] = {};
> arch/x86/domain.c: unsigned long total[MASK_EXTR(PGT_type_mask,
> PGT_type_mask) + 1] = {};
> tools/kconfig/expr.c: union string_value lval = {}, rval = {};
> common/grant_table.c: struct gnttab_copy_buf src = {};
> common/grant_table.c: struct gnttab_copy_buf dest = {};
>
> So I thought that that's the prevailing convention. I've now searched
> for "= {0}", and I see that that has also been done, so I suppose either
> way is fine as far as the coding style goes?
>
> $ grep -R "= {0}"
> arch/arm/mm.c:    lpae_t pte = {0};
> arch/arm/mm.c:    lpae_t pte = {0};
> drivers/char/exynos4210-uart.c:} exynos4210_com = {0};
> drivers/char/pl011.c:} pl011_com = {0};
> drivers/char/omap-uart.c:} omap_com = {0};
> drivers/char/scif-uart.c:} scif_com = {0};
> drivers/char/cadence-uart.c:} cuart_com = {0};
> tools/kconfig/nconf.gui.c:attributes_t attributes[ATTR_MAX+1] = {0};
> crypto/vmac.c:    uint64_t in[2] = {0}, out[2];

The two are equivalent as far as C goes.  I prefer the former as it is
slightly shorter.

~Andrew

Re: [PATCH] x86/hvm_event: fix uninitialized struct field usage introduced by c/s f5365e6

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 3704 bytes --]

On Thu, Feb 18, 2016 at 7:18 AM, Andrew Cooper <andrew.cooper3@citrix.com>
wrote:

> On 18/02/16 14:16, Razvan Cojocaru wrote:
> > On 02/18/2016 04:10 PM, Tamas K Lengyel wrote:
> >> On Feb 18, 2016 03:46, "Razvan Cojocaru" <rcojocaru@bitdefender.com
> >> <mailto:rcojocaru@bitdefender.com>> wrote:
> >>> On 02/18/2016 12:45 PM, Corneliu ZUZU wrote:
> >>>> c/s f5365e6: "xen/vm-events: Move parts of monitor_domctl code to
> >> common-side",
> >>>> introduced a use without initialization issue.
> >>>> hvm_event_breakpoint calls hvm_event_traps(&req) and if sync is true
> >> that
> >>>> ors some bits into req->flags which was never initialised.
> >>>> Reported by Coverity Scan.
> >>>>
> >>>> Initializes req @ hvm_event_breakpoint entry.
> >>>>
> >>>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com
> >> <mailto:czuzu@bitdefender.com>>
> >>>> ---
> >>>>  xen/arch/x86/hvm/event.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
> >>>> index 874a36c..cb9c152 100644
> >>>> --- a/xen/arch/x86/hvm/event.c
> >>>> +++ b/xen/arch/x86/hvm/event.c
> >>>> @@ -173,7 +173,7 @@ int hvm_event_breakpoint(unsigned long rip,
> >>>>  {
> >>>>      struct vcpu *curr = current;
> >>>>      struct arch_domain *ad = &curr->domain->arch;
> >>>> -    vm_event_request_t req;
> >>>> +    vm_event_request_t req = {};
> >> Should this be = { 0 } instead? Also, as I recall the request is not
> >> initialized on any of the paths, so we might as well do it for all of
> >> them, not just here. It would help avoid the listener erronously using
> >> some fields that were not actually initialized as well.
> > That should achieve the same thing. I first looked up "= {}" in the Xen
> > source code and found:
> >
> > $ grep -R "= {}"
> > arch/arm/domain_build.c: struct kernel_info kinfo = {};
> > arch/x86/time.c: struct vcpu_time_info *u, _u = {};
> > arch/x86/tboot.c: uint8_t nonce[16] = {};
> > arch/x86/tboot.c: uint8_t nonce[16] = {};
> > arch/x86/tboot.c: uint8_t nonce[16] = {};
> > arch/x86/traps.c: unsigned char insns_before[8] = {}, insns_after[16] =
> {};
> > arch/x86/x86_emulate/x86_emulate.c: union vex vex = {};
> > arch/x86/x86_emulate/x86_emulate.c: struct x86_emulate_stub stub = {};
> > arch/x86/cpu/mtrr/generic.c:struct mtrr_state mtrr_state = {};
> > arch/x86/cpu/common.c:const struct cpu_dev *__read_mostly
> > cpu_devs[X86_VENDOR_NUM] = {};
> > arch/x86/domain.c: unsigned long total[MASK_EXTR(PGT_type_mask,
> > PGT_type_mask) + 1] = {};
> > tools/kconfig/expr.c: union string_value lval = {}, rval = {};
> > common/grant_table.c: struct gnttab_copy_buf src = {};
> > common/grant_table.c: struct gnttab_copy_buf dest = {};
> >
> > So I thought that that's the prevailing convention. I've now searched
> > for "= {0}", and I see that that has also been done, so I suppose either
> > way is fine as far as the coding style goes?
> >
> > $ grep -R "= {0}"
> > arch/arm/mm.c:    lpae_t pte = {0};
> > arch/arm/mm.c:    lpae_t pte = {0};
> > drivers/char/exynos4210-uart.c:} exynos4210_com = {0};
> > drivers/char/pl011.c:} pl011_com = {0};
> > drivers/char/omap-uart.c:} omap_com = {0};
> > drivers/char/scif-uart.c:} scif_com = {0};
> > drivers/char/cadence-uart.c:} cuart_com = {0};
> > tools/kconfig/nconf.gui.c:attributes_t attributes[ATTR_MAX+1] = {0};
> > crypto/vmac.c:    uint64_t in[2] = {0}, out[2];
>
> The two are equivalent as far as C goes.  I prefer the former as it is
> slightly shorter.


OK, as long as they are equivalent I'm fine with either. I always use the
{0} syntax, it looks more explicit to me, so just wanted to double-check
that it means the same thing.

Thanks,
Tamas

[-- Attachment #1.2: Type: text/html, Size: 5086 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] x86/hvm_event: fix uninitialized struct field usage introduced by c/s f5365e6

[Andrew Cooper]

On 18/02/16 10:45, Corneliu ZUZU wrote:
> c/s f5365e6: "xen/vm-events: Move parts of monitor_domctl code to common-side",
> introduced a use without initialization issue.
> hvm_event_breakpoint calls hvm_event_traps(&req) and if sync is true that
> ors some bits into req->flags which was never initialised.
> Reported by Coverity Scan.
>
> Initializes req @ hvm_event_breakpoint entry.

Coverity-ID: 1353192

> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>  xen/arch/x86/hvm/event.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
> index 874a36c..cb9c152 100644
> --- a/xen/arch/x86/hvm/event.c
> +++ b/xen/arch/x86/hvm/event.c
> @@ -173,7 +173,7 @@ int hvm_event_breakpoint(unsigned long rip,
>  {
>      struct vcpu *curr = current;
>      struct arch_domain *ad = &curr->domain->arch;
> -    vm_event_request_t req;
> +    vm_event_request_t req = {};
>  
>      switch ( type )
>      {