| Message ID | 1459333920-2182-1-git-send-email-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 30.03.16 at 12:32, <paul.durrant@citrix.com> wrote: > --- a/xen/arch/x86/hvm/viridian.c > +++ b/xen/arch/x86/hvm/viridian.c > @@ -824,6 +824,8 @@ static int viridian_save_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h) > for_each_vcpu( d, v ) { > struct hvm_viridian_vcpu_context ctxt; > > + memset(&ctxt, 0, sizeof(ctxt)); How about just adding an empty initializer to the declaration? > @@ -834,6 +836,15 @@ static int viridian_save_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h) > return 0; > } > > +static bool_t is_zero(void *p, size_t size) At the very least this wants to be a pointer to const. > +{ > + while ( size-- ) > + if ( *(uint8_t *)p++ != 0 ) > + return 0; > + > + return 1; > +} > + > static int viridian_load_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h) > { > int vcpuid; > @@ -851,6 +862,9 @@ static int viridian_load_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h) > if ( hvm_load_entry_zeroextend(VIRIDIAN_VCPU, h, &ctxt) != 0 ) > return -EINVAL; > > + if ( !is_zero(&ctxt._pad, sizeof(ctxt._pad)) ) > + return -EINVAL; "memcmp(&ctx._pad, zero_page, sizeof(ctxt._pad))" would be an alternative not requiring any new helper function. Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 30 March 2016 12:23 > To: Paul Durrant > Cc: Andrew Cooper; xen-devel@lists.xenproject.org; Keir (Xen.org) > Subject: Re: [PATCH] x86/hvm/viridian: zero and check vcpu context __pad > field > > >>> On 30.03.16 at 12:32, <paul.durrant@citrix.com> wrote: > > --- a/xen/arch/x86/hvm/viridian.c > > +++ b/xen/arch/x86/hvm/viridian.c > > @@ -824,6 +824,8 @@ static int viridian_save_vcpu_ctxt(struct domain *d, > hvm_domain_context_t *h) > > for_each_vcpu( d, v ) { > > struct hvm_viridian_vcpu_context ctxt; > > > > + memset(&ctxt, 0, sizeof(ctxt)); > > How about just adding an empty initializer to the declaration? > I think having a 'zero the entire struct' call at the start is better as it will cover any additions made to the struct in future. It's what I had mistakenly assumed was already there. In fact I think adding a similar call into the domain context save function would probably be worthwhile. > > @@ -834,6 +836,15 @@ static int viridian_save_vcpu_ctxt(struct domain > *d, hvm_domain_context_t *h) > > return 0; > > } > > > > +static bool_t is_zero(void *p, size_t size) > > At the very least this wants to be a pointer to const. > > > +{ > > + while ( size-- ) > > + if ( *(uint8_t *)p++ != 0 ) > > + return 0; > > + > > + return 1; > > +} > > + > > static int viridian_load_vcpu_ctxt(struct domain *d, > hvm_domain_context_t *h) > > { > > int vcpuid; > > @@ -851,6 +862,9 @@ static int viridian_load_vcpu_ctxt(struct domain *d, > hvm_domain_context_t *h) > > if ( hvm_load_entry_zeroextend(VIRIDIAN_VCPU, h, &ctxt) != 0 ) > > return -EINVAL; > > > > + if ( !is_zero(&ctxt._pad, sizeof(ctxt._pad)) ) > > + return -EINVAL; > > "memcmp(&ctx._pad, zero_page, sizeof(ctxt._pad))" would be an > alternative not requiring any new helper function. > Ah, I didn't know about the zero_page definition. I'll use that and drop the helper. Paul > Jan
>>> On 30.03.16 at 13:26, <Paul.Durrant@citrix.com> wrote: >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: 30 March 2016 12:23 >> >>> On 30.03.16 at 12:32, <paul.durrant@citrix.com> wrote: >> > --- a/xen/arch/x86/hvm/viridian.c >> > +++ b/xen/arch/x86/hvm/viridian.c >> > @@ -824,6 +824,8 @@ static int viridian_save_vcpu_ctxt(struct domain *d, >> hvm_domain_context_t *h) >> > for_each_vcpu( d, v ) { >> > struct hvm_viridian_vcpu_context ctxt; >> > >> > + memset(&ctxt, 0, sizeof(ctxt)); >> >> How about just adding an empty initializer to the declaration? >> > > I think having a 'zero the entire struct' call at the start is better as it > will cover any additions made to the struct in future. It's what I had > mistakenly assumed was already there. In fact I think adding a similar call > into the domain context save function would probably be worthwhile. And how does the initializer approach not fulfill that intention? Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 30 March 2016 14:17 > To: Paul Durrant > Cc: Andrew Cooper; xen-devel@lists.xenproject.org; Keir (Xen.org) > Subject: RE: [PATCH] x86/hvm/viridian: zero and check vcpu context __pad > field > > >>> On 30.03.16 at 13:26, <Paul.Durrant@citrix.com> wrote: > >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> Sent: 30 March 2016 12:23 > >> >>> On 30.03.16 at 12:32, <paul.durrant@citrix.com> wrote: > >> > --- a/xen/arch/x86/hvm/viridian.c > >> > +++ b/xen/arch/x86/hvm/viridian.c > >> > @@ -824,6 +824,8 @@ static int viridian_save_vcpu_ctxt(struct domain > *d, > >> hvm_domain_context_t *h) > >> > for_each_vcpu( d, v ) { > >> > struct hvm_viridian_vcpu_context ctxt; > >> > > >> > + memset(&ctxt, 0, sizeof(ctxt)); > >> > >> How about just adding an empty initializer to the declaration? > >> > > > > I think having a 'zero the entire struct' call at the start is better as it > > will cover any additions made to the struct in future. It's what I had > > mistakenly assumed was already there. In fact I think adding a similar call > > into the domain context save function would probably be worthwhile. > > And how does the initializer approach not fulfill that intention? > Because any time anyone adds another field they have to remember to add another initializer, which is what I forgot to do. This approach OTOH is failsafe. Paul > Jan
>>> On 30.03.16 at 15:19, <Paul.Durrant@citrix.com> wrote: >> -----Original Message----- >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: 30 March 2016 14:17 >> To: Paul Durrant >> Cc: Andrew Cooper; xen-devel@lists.xenproject.org; Keir (Xen.org) >> Subject: RE: [PATCH] x86/hvm/viridian: zero and check vcpu context __pad >> field >> >> >>> On 30.03.16 at 13:26, <Paul.Durrant@citrix.com> wrote: >> >> From: Jan Beulich [mailto:JBeulich@suse.com] >> >> Sent: 30 March 2016 12:23 >> >> >>> On 30.03.16 at 12:32, <paul.durrant@citrix.com> wrote: >> >> > --- a/xen/arch/x86/hvm/viridian.c >> >> > +++ b/xen/arch/x86/hvm/viridian.c >> >> > @@ -824,6 +824,8 @@ static int viridian_save_vcpu_ctxt(struct domain >> *d, >> >> hvm_domain_context_t *h) >> >> > for_each_vcpu( d, v ) { >> >> > struct hvm_viridian_vcpu_context ctxt; >> >> > >> >> > + memset(&ctxt, 0, sizeof(ctxt)); >> >> >> >> How about just adding an empty initializer to the declaration? >> >> >> > >> > I think having a 'zero the entire struct' call at the start is better as it >> > will cover any additions made to the struct in future. It's what I had >> > mistakenly assumed was already there. In fact I think adding a similar call >> > into the domain context save function would probably be worthwhile. >> >> And how does the initializer approach not fulfill that intention? >> > > Because any time anyone adds another field they have to remember to add > another initializer, which is what I forgot to do. This approach OTOH is > failsafe. But note how I said "an empty initializer": When there is an initializer at all, all fields not mentioned in the initializer will get default initialized (i.e. zeroed). Hence an empty initializer clears the entire structure. Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 30 March 2016 15:22 > To: Paul Durrant > Cc: Andrew Cooper; xen-devel@lists.xenproject.org; Keir (Xen.org) > Subject: RE: [PATCH] x86/hvm/viridian: zero and check vcpu context __pad > field > > >>> On 30.03.16 at 15:19, <Paul.Durrant@citrix.com> wrote: > >> -----Original Message----- > >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> Sent: 30 March 2016 14:17 > >> To: Paul Durrant > >> Cc: Andrew Cooper; xen-devel@lists.xenproject.org; Keir (Xen.org) > >> Subject: RE: [PATCH] x86/hvm/viridian: zero and check vcpu context > __pad > >> field > >> > >> >>> On 30.03.16 at 13:26, <Paul.Durrant@citrix.com> wrote: > >> >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> >> Sent: 30 March 2016 12:23 > >> >> >>> On 30.03.16 at 12:32, <paul.durrant@citrix.com> wrote: > >> >> > --- a/xen/arch/x86/hvm/viridian.c > >> >> > +++ b/xen/arch/x86/hvm/viridian.c > >> >> > @@ -824,6 +824,8 @@ static int viridian_save_vcpu_ctxt(struct > domain > >> *d, > >> >> hvm_domain_context_t *h) > >> >> > for_each_vcpu( d, v ) { > >> >> > struct hvm_viridian_vcpu_context ctxt; > >> >> > > >> >> > + memset(&ctxt, 0, sizeof(ctxt)); > >> >> > >> >> How about just adding an empty initializer to the declaration? > >> >> > >> > > >> > I think having a 'zero the entire struct' call at the start is better as it > >> > will cover any additions made to the struct in future. It's what I had > >> > mistakenly assumed was already there. In fact I think adding a similar call > >> > into the domain context save function would probably be worthwhile. > >> > >> And how does the initializer approach not fulfill that intention? > >> > > > > Because any time anyone adds another field they have to remember to > add > > another initializer, which is what I forgot to do. This approach OTOH is > > failsafe. > > But note how I said "an empty initializer": When there is an > initializer at all, all fields not mentioned in the initializer will get > default initialized (i.e. zeroed). Hence an empty initializer > clears the entire structure. > Ah, you mean C99 initializer style. That would be neater. Paul > Jan
>>> On 30.03.16 at 17:16, <Paul.Durrant@citrix.com> wrote: >> -----Original Message----- >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: 30 March 2016 15:22 >> To: Paul Durrant >> Cc: Andrew Cooper; xen-devel@lists.xenproject.org; Keir (Xen.org) >> Subject: RE: [PATCH] x86/hvm/viridian: zero and check vcpu context __pad >> field >> >> >>> On 30.03.16 at 15:19, <Paul.Durrant@citrix.com> wrote: >> >> -----Original Message----- >> >> From: Jan Beulich [mailto:JBeulich@suse.com] >> >> Sent: 30 March 2016 14:17 >> >> To: Paul Durrant >> >> Cc: Andrew Cooper; xen-devel@lists.xenproject.org; Keir (Xen.org) >> >> Subject: RE: [PATCH] x86/hvm/viridian: zero and check vcpu context >> __pad >> >> field >> >> >> >> >>> On 30.03.16 at 13:26, <Paul.Durrant@citrix.com> wrote: >> >> >> From: Jan Beulich [mailto:JBeulich@suse.com] >> >> >> Sent: 30 March 2016 12:23 >> >> >> >>> On 30.03.16 at 12:32, <paul.durrant@citrix.com> wrote: >> >> >> > --- a/xen/arch/x86/hvm/viridian.c >> >> >> > +++ b/xen/arch/x86/hvm/viridian.c >> >> >> > @@ -824,6 +824,8 @@ static int viridian_save_vcpu_ctxt(struct >> domain >> >> *d, >> >> >> hvm_domain_context_t *h) >> >> >> > for_each_vcpu( d, v ) { >> >> >> > struct hvm_viridian_vcpu_context ctxt; >> >> >> > >> >> >> > + memset(&ctxt, 0, sizeof(ctxt)); >> >> >> >> >> >> How about just adding an empty initializer to the declaration? >> >> >> >> >> > >> >> > I think having a 'zero the entire struct' call at the start is better as > it >> >> > will cover any additions made to the struct in future. It's what I had >> >> > mistakenly assumed was already there. In fact I think adding a similar > call >> >> > into the domain context save function would probably be worthwhile. >> >> >> >> And how does the initializer approach not fulfill that intention? >> >> >> > >> > Because any time anyone adds another field they have to remember to >> add >> > another initializer, which is what I forgot to do. This approach OTOH is >> > failsafe. >> >> But note how I said "an empty initializer": When there is an >> initializer at all, all fields not mentioned in the initializer will get >> default initialized (i.e. zeroed). Hence an empty initializer >> clears the entire structure. >> > > Ah, you mean C99 initializer style. That would be neater. Not really - "static struct s s = {};" was allowed in C89 too iirc. Jan
diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c index 5c76c1a..b85b55b 100644 --- a/xen/arch/x86/hvm/viridian.c +++ b/xen/arch/x86/hvm/viridian.c @@ -824,6 +824,8 @@ static int viridian_save_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h) for_each_vcpu( d, v ) { struct hvm_viridian_vcpu_context ctxt; + memset(&ctxt, 0, sizeof(ctxt)); + ctxt.apic_assist_msr = v->arch.hvm_vcpu.viridian.apic_assist.msr.raw; ctxt.apic_assist_vector = v->arch.hvm_vcpu.viridian.apic_assist.vector; @@ -834,6 +836,15 @@ static int viridian_save_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h) return 0; } +static bool_t is_zero(void *p, size_t size) +{ + while ( size-- ) + if ( *(uint8_t *)p++ != 0 ) + return 0; + + return 1; +} + static int viridian_load_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h) { int vcpuid; @@ -851,6 +862,9 @@ static int viridian_load_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h) if ( hvm_load_entry_zeroextend(VIRIDIAN_VCPU, h, &ctxt) != 0 ) return -EINVAL; + if ( !is_zero(&ctxt._pad, sizeof(ctxt._pad)) ) + return -EINVAL; + v->arch.hvm_vcpu.viridian.apic_assist.msr.raw = ctxt.apic_assist_msr; if ( v->arch.hvm_vcpu.viridian.apic_assist.msr.fields.enabled ) initialize_apic_assist(v);
Commit 57844631 "save APIC assist vector" added an extra field to the viridian vcpu context save record. This field was only a uint8_t and so an extra __pad field was also added to pad up to the next 64-bit boundary. This patch makes sure that __pad field is zeroed on save and checked for zero on restore. This prevents a potential leak of information from the stack and a compatibility check against future use of the space occupied by the __pad field. 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/viridian.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)