* [PATCH] x86/hvm/viridian: zero and check vcpu context __pad field
@ 2016-03-30 10:32 Paul Durrant
2016-03-30 11:22 ` Jan Beulich
0 siblings, 1 reply; 8+ messages in thread
From: Paul Durrant @ 2016-03-30 10:32 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Paul Durrant, Keir Fraser, Jan Beulich
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(+)
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);
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] x86/hvm/viridian: zero and check vcpu context __pad field
2016-03-30 10:32 [PATCH] x86/hvm/viridian: zero and check vcpu context __pad field Paul Durrant
@ 2016-03-30 11:22 ` Jan Beulich
2016-03-30 11:26 ` Paul Durrant
0 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2016-03-30 11:22 UTC (permalink / raw)
To: Paul Durrant; +Cc: Andrew Cooper, Keir Fraser, xen-devel
>>> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] x86/hvm/viridian: zero and check vcpu context __pad field
2016-03-30 11:22 ` Jan Beulich
@ 2016-03-30 11:26 ` Paul Durrant
2016-03-30 13:16 ` Jan Beulich
0 siblings, 1 reply; 8+ messages in thread
From: Paul Durrant @ 2016-03-30 11:26 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Keir (Xen.org), xen-devel
> -----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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] x86/hvm/viridian: zero and check vcpu context __pad field
2016-03-30 11:26 ` Paul Durrant
@ 2016-03-30 13:16 ` Jan Beulich
2016-03-30 13:19 ` Paul Durrant
0 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2016-03-30 13:16 UTC (permalink / raw)
To: Paul Durrant; +Cc: Andrew Cooper, Keir(Xen.org), xen-devel
>>> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] x86/hvm/viridian: zero and check vcpu context __pad field
2016-03-30 13:16 ` Jan Beulich
@ 2016-03-30 13:19 ` Paul Durrant
2016-03-30 14:22 ` Jan Beulich
0 siblings, 1 reply; 8+ messages in thread
From: Paul Durrant @ 2016-03-30 13:19 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Keir (Xen.org), xen-devel
> -----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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] x86/hvm/viridian: zero and check vcpu context __pad field
2016-03-30 13:19 ` Paul Durrant
@ 2016-03-30 14:22 ` Jan Beulich
2016-03-30 15:16 ` Paul Durrant
0 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2016-03-30 14:22 UTC (permalink / raw)
To: Paul Durrant; +Cc: Andrew Cooper, Keir(Xen.org), xen-devel
>>> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] x86/hvm/viridian: zero and check vcpu context __pad field
2016-03-30 14:22 ` Jan Beulich
@ 2016-03-30 15:16 ` Paul Durrant
2016-03-30 15:24 ` Jan Beulich
0 siblings, 1 reply; 8+ messages in thread
From: Paul Durrant @ 2016-03-30 15:16 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Keir (Xen.org), xen-devel
> -----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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] x86/hvm/viridian: zero and check vcpu context __pad field
2016-03-30 15:16 ` Paul Durrant
@ 2016-03-30 15:24 ` Jan Beulich
0 siblings, 0 replies; 8+ messages in thread
From: Jan Beulich @ 2016-03-30 15:24 UTC (permalink / raw)
To: Paul Durrant; +Cc: Andrew Cooper, Keir(Xen.org), xen-devel
>>> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-03-30 15:25 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-30 10:32 [PATCH] x86/hvm/viridian: zero and check vcpu context __pad field Paul Durrant
2016-03-30 11:22 ` Jan Beulich
2016-03-30 11:26 ` Paul Durrant
2016-03-30 13:16 ` Jan Beulich
2016-03-30 13:19 ` Paul Durrant
2016-03-30 14:22 ` Jan Beulich
2016-03-30 15:16 ` Paul Durrant
2016-03-30 15:24 ` Jan Beulich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).