Re: [Xen-devel] [PATCH 1/4] x86/vmx: Don't leak host syscall MSR state into HVM guests

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.coop...@citrix.com]
> Sent: Monday, February 20, 2017 7:17 PM
> To: Tian, Kevin
> Cc: Jan Beulich; Suravee Suthikulpanit; Nakajima, Jun; Xen-devel; Boris 
> Ostrovsky
> Subject: Re: [PATCH 1/4] x86/vmx: Don't leak host syscall MSR state into HVM 
> guests
> 
> On 14/02/17 10:13, Jan Beulich wrote:
>  On 14.02.17 at 09:40,  wrote:
> >>>  From: Andrew Cooper [mailto:am...@hermes.cam.ac.uk] On Behalf Of Andrew
> Cooper
> >>> Sent: Tuesday, February 14, 2017 4:19 PM
> >>>
> >>> On 14/02/2017 08:04, Tian, Kevin wrote:
> > From: Andrew Cooper [mailto:am...@hermes.cam.ac.uk] On Behalf Of Andrew
> >>> Cooper
> > Sent: Tuesday, February 14, 2017 3:59 PM
> >
> > On 14/02/2017 02:52, Tian, Kevin wrote:
> >>> From: Andrew Cooper [mailto:andrew.coop...@citrix.com]
> >>> Sent: Monday, February 13, 2017 10:32 PM
> >>>
> >>> hvm_hw_cpu->msr_flags is in fact the VMX dirty bitmap of MSRs needing 
> >>> to
> be
> >>> restored when switching into guest context.  It should never have 
> >>> been part
> of
> >>> the migration state to start with, and Xen must not make any 
> >>> decisions based
> >>> on the value seen during restore.
> >>>
> >>> Identify it as obsolete in the header files, consistently save it as 
> >>> zero and
> >>> ignore it on restore.
> >>>
> >>> The MSRs must be considered dirty during VMCS creation to cause the 
> >>> proper
> >>> defaults of 0 to be visible to the guest.
> >>>
> >>> Signed-off-by: Andrew Cooper 
> >> Reviewed-by: Kevin Tian , with one small comment.
> >>
> >> the effect of this patch should be more than not leaking syscall MSR.
> >> what about making the subject clearer when check-in?
> > What other effects do you think are going on here?  Yes, we now context
> > switch the MSRs right from the start of the domain, but that is
> > necessary to avoid leaking them.
> >
>  If just looking at this patch, it's for general MSR save/restore policy,
>  nothing specific to syscall MSR.
> >>> The only three MSRs which use this infrastructure are LSTAR, STAR and
> >>> FMASK.  What if I were to clarify that in the first paragraph?
> >> I meant the subject line (talk about syscall MSR leakage) mismatches the
> >> commit message (for general MSR load)
> > I'm with Andrew here: The title seems perfectly fine to me, considering
> > that the generic mechanism is only used for the syscall MSRs. Hence I
> > would think his offer to clarify the change in the first paragraph of the
> > commit message ought to suffice. Otherwise, may I ask that you make
> > a concrete suggestion as to what you'd like to see?
> 
> Kevin: Do you have any other concerns?
> 
> ~Andrew

fine to me.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/4] x86/vmx: Don't leak host syscall MSR state into HVM guests

[Andrew Cooper]

On 14/02/17 10:13, Jan Beulich wrote:
 On 14.02.17 at 09:40,  wrote:
>>>  From: Andrew Cooper [mailto:am...@hermes.cam.ac.uk] On Behalf Of Andrew 
>>> Cooper
>>> Sent: Tuesday, February 14, 2017 4:19 PM
>>>
>>> On 14/02/2017 08:04, Tian, Kevin wrote:
> From: Andrew Cooper [mailto:am...@hermes.cam.ac.uk] On Behalf Of Andrew
>>> Cooper
> Sent: Tuesday, February 14, 2017 3:59 PM
>
> On 14/02/2017 02:52, Tian, Kevin wrote:
>>> From: Andrew Cooper [mailto:andrew.coop...@citrix.com]
>>> Sent: Monday, February 13, 2017 10:32 PM
>>>
>>> hvm_hw_cpu->msr_flags is in fact the VMX dirty bitmap of MSRs needing 
>>> to be
>>> restored when switching into guest context.  It should never have been 
>>> part of
>>> the migration state to start with, and Xen must not make any decisions 
>>> based
>>> on the value seen during restore.
>>>
>>> Identify it as obsolete in the header files, consistently save it as 
>>> zero and
>>> ignore it on restore.
>>>
>>> The MSRs must be considered dirty during VMCS creation to cause the 
>>> proper
>>> defaults of 0 to be visible to the guest.
>>>
>>> Signed-off-by: Andrew Cooper 
>> Reviewed-by: Kevin Tian , with one small comment.
>>
>> the effect of this patch should be more than not leaking syscall MSR.
>> what about making the subject clearer when check-in?
> What other effects do you think are going on here?  Yes, we now context
> switch the MSRs right from the start of the domain, but that is
> necessary to avoid leaking them.
>
 If just looking at this patch, it's for general MSR save/restore policy,
 nothing specific to syscall MSR.
>>> The only three MSRs which use this infrastructure are LSTAR, STAR and
>>> FMASK.  What if I were to clarify that in the first paragraph?
>> I meant the subject line (talk about syscall MSR leakage) mismatches the 
>> commit message (for general MSR load)
> I'm with Andrew here: The title seems perfectly fine to me, considering
> that the generic mechanism is only used for the syscall MSRs. Hence I
> would think his offer to clarify the change in the first paragraph of the
> commit message ought to suffice. Otherwise, may I ask that you make
> a concrete suggestion as to what you'd like to see?

Kevin: Do you have any other concerns?

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/4] x86/vmx: Don't leak host syscall MSR state into HVM guests

[Jan Beulich]

>>> On 14.02.17 at 09:40,  wrote:
>>  From: Andrew Cooper [mailto:am...@hermes.cam.ac.uk] On Behalf Of Andrew 
>> Cooper
>> Sent: Tuesday, February 14, 2017 4:19 PM
>> 
>> On 14/02/2017 08:04, Tian, Kevin wrote:
>> >> From: Andrew Cooper [mailto:am...@hermes.cam.ac.uk] On Behalf Of Andrew
>> Cooper
>> >> Sent: Tuesday, February 14, 2017 3:59 PM
>> >>
>> >> On 14/02/2017 02:52, Tian, Kevin wrote:
>>  From: Andrew Cooper [mailto:andrew.coop...@citrix.com]
>>  Sent: Monday, February 13, 2017 10:32 PM
>> 
>>  hvm_hw_cpu->msr_flags is in fact the VMX dirty bitmap of MSRs needing 
>>  to be
>>  restored when switching into guest context.  It should never have been 
>>  part of
>>  the migration state to start with, and Xen must not make any decisions 
>>  based
>>  on the value seen during restore.
>> 
>>  Identify it as obsolete in the header files, consistently save it as 
>>  zero and
>>  ignore it on restore.
>> 
>>  The MSRs must be considered dirty during VMCS creation to cause the 
>>  proper
>>  defaults of 0 to be visible to the guest.
>> 
>>  Signed-off-by: Andrew Cooper 
>> >>> Reviewed-by: Kevin Tian , with one small comment.
>> >>>
>> >>> the effect of this patch should be more than not leaking syscall MSR.
>> >>> what about making the subject clearer when check-in?
>> >> What other effects do you think are going on here?  Yes, we now context
>> >> switch the MSRs right from the start of the domain, but that is
>> >> necessary to avoid leaking them.
>> >>
>> > If just looking at this patch, it's for general MSR save/restore policy,
>> > nothing specific to syscall MSR.
>> 
>> The only three MSRs which use this infrastructure are LSTAR, STAR and
>> FMASK.  What if I were to clarify that in the first paragraph?
> 
> I meant the subject line (talk about syscall MSR leakage) mismatches the 
> commit message (for general MSR load)

I'm with Andrew here: The title seems perfectly fine to me, considering
that the generic mechanism is only used for the syscall MSRs. Hence I
would think his offer to clarify the change in the first paragraph of the
commit message ought to suffice. Otherwise, may I ask that you make
a concrete suggestion as to what you'd like to see?

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/4] x86/vmx: Don't leak host syscall MSR state into HVM guests

[Tian, Kevin]

> From: Andrew Cooper [mailto:am...@hermes.cam.ac.uk] On Behalf Of Andrew Cooper
> Sent: Tuesday, February 14, 2017 4:19 PM
> 
> On 14/02/2017 08:04, Tian, Kevin wrote:
> >> From: Andrew Cooper [mailto:am...@hermes.cam.ac.uk] On Behalf Of Andrew
> Cooper
> >> Sent: Tuesday, February 14, 2017 3:59 PM
> >>
> >> On 14/02/2017 02:52, Tian, Kevin wrote:
>  From: Andrew Cooper [mailto:andrew.coop...@citrix.com]
>  Sent: Monday, February 13, 2017 10:32 PM
> 
>  hvm_hw_cpu->msr_flags is in fact the VMX dirty bitmap of MSRs needing to 
>  be
>  restored when switching into guest context.  It should never have been 
>  part of
>  the migration state to start with, and Xen must not make any decisions 
>  based
>  on the value seen during restore.
> 
>  Identify it as obsolete in the header files, consistently save it as 
>  zero and
>  ignore it on restore.
> 
>  The MSRs must be considered dirty during VMCS creation to cause the 
>  proper
>  defaults of 0 to be visible to the guest.
> 
>  Signed-off-by: Andrew Cooper 
> >>> Reviewed-by: Kevin Tian , with one small comment.
> >>>
> >>> the effect of this patch should be more than not leaking syscall MSR.
> >>> what about making the subject clearer when check-in?
> >> What other effects do you think are going on here?  Yes, we now context
> >> switch the MSRs right from the start of the domain, but that is
> >> necessary to avoid leaking them.
> >>
> > If just looking at this patch, it's for general MSR save/restore policy,
> > nothing specific to syscall MSR.
> 
> The only three MSRs which use this infrastructure are LSTAR, STAR and
> FMASK.  What if I were to clarify that in the first paragraph?
> 
> ~Andrew

I meant the subject line (talk about syscall MSR leakage) mismatches the 
commit message (for general MSR load)

Thanks
Kevin

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/4] x86/vmx: Don't leak host syscall MSR state into HVM guests

[Andrew Cooper]

On 14/02/2017 08:04, Tian, Kevin wrote:
>> From: Andrew Cooper [mailto:am...@hermes.cam.ac.uk] On Behalf Of Andrew 
>> Cooper
>> Sent: Tuesday, February 14, 2017 3:59 PM
>>
>> On 14/02/2017 02:52, Tian, Kevin wrote:
 From: Andrew Cooper [mailto:andrew.coop...@citrix.com]
 Sent: Monday, February 13, 2017 10:32 PM

 hvm_hw_cpu->msr_flags is in fact the VMX dirty bitmap of MSRs needing to be
 restored when switching into guest context.  It should never have been 
 part of
 the migration state to start with, and Xen must not make any decisions 
 based
 on the value seen during restore.

 Identify it as obsolete in the header files, consistently save it as zero 
 and
 ignore it on restore.

 The MSRs must be considered dirty during VMCS creation to cause the proper
 defaults of 0 to be visible to the guest.

 Signed-off-by: Andrew Cooper 
>>> Reviewed-by: Kevin Tian , with one small comment.
>>>
>>> the effect of this patch should be more than not leaking syscall MSR.
>>> what about making the subject clearer when check-in?
>> What other effects do you think are going on here?  Yes, we now context
>> switch the MSRs right from the start of the domain, but that is
>> necessary to avoid leaking them.
>>
> If just looking at this patch, it's for general MSR save/restore policy,
> nothing specific to syscall MSR.

The only three MSRs which use this infrastructure are LSTAR, STAR and
FMASK.  What if I were to clarify that in the first paragraph?

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/4] x86/vmx: Don't leak host syscall MSR state into HVM guests

[Tian, Kevin]

> From: Andrew Cooper [mailto:am...@hermes.cam.ac.uk] On Behalf Of Andrew Cooper
> Sent: Tuesday, February 14, 2017 3:59 PM
> 
> On 14/02/2017 02:52, Tian, Kevin wrote:
> >> From: Andrew Cooper [mailto:andrew.coop...@citrix.com]
> >> Sent: Monday, February 13, 2017 10:32 PM
> >>
> >> hvm_hw_cpu->msr_flags is in fact the VMX dirty bitmap of MSRs needing to be
> >> restored when switching into guest context.  It should never have been 
> >> part of
> >> the migration state to start with, and Xen must not make any decisions 
> >> based
> >> on the value seen during restore.
> >>
> >> Identify it as obsolete in the header files, consistently save it as zero 
> >> and
> >> ignore it on restore.
> >>
> >> The MSRs must be considered dirty during VMCS creation to cause the proper
> >> defaults of 0 to be visible to the guest.
> >>
> >> Signed-off-by: Andrew Cooper 
> > Reviewed-by: Kevin Tian , with one small comment.
> >
> > the effect of this patch should be more than not leaking syscall MSR.
> > what about making the subject clearer when check-in?
> 
> What other effects do you think are going on here?  Yes, we now context
> switch the MSRs right from the start of the domain, but that is
> necessary to avoid leaking them.
> 

If just looking at this patch, it's for general MSR save/restore policy,
nothing specific to syscall MSR. 

Thanks
Kevin

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/4] x86/vmx: Don't leak host syscall MSR state into HVM guests

[Andrew Cooper]

On 14/02/2017 02:52, Tian, Kevin wrote:
>> From: Andrew Cooper [mailto:andrew.coop...@citrix.com]
>> Sent: Monday, February 13, 2017 10:32 PM
>>
>> hvm_hw_cpu->msr_flags is in fact the VMX dirty bitmap of MSRs needing to be
>> restored when switching into guest context.  It should never have been part 
>> of
>> the migration state to start with, and Xen must not make any decisions based
>> on the value seen during restore.
>>
>> Identify it as obsolete in the header files, consistently save it as zero and
>> ignore it on restore.
>>
>> The MSRs must be considered dirty during VMCS creation to cause the proper
>> defaults of 0 to be visible to the guest.
>>
>> Signed-off-by: Andrew Cooper 
> Reviewed-by: Kevin Tian , with one small comment.
>
> the effect of this patch should be more than not leaking syscall MSR.
> what about making the subject clearer when check-in?

What other effects do you think are going on here?  Yes, we now context
switch the MSRs right from the start of the domain, but that is
necessary to avoid leaking them.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/4] x86/vmx: Don't leak host syscall MSR state into HVM guests

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.coop...@citrix.com]
> Sent: Monday, February 13, 2017 10:32 PM
> 
> hvm_hw_cpu->msr_flags is in fact the VMX dirty bitmap of MSRs needing to be
> restored when switching into guest context.  It should never have been part of
> the migration state to start with, and Xen must not make any decisions based
> on the value seen during restore.
> 
> Identify it as obsolete in the header files, consistently save it as zero and
> ignore it on restore.
> 
> The MSRs must be considered dirty during VMCS creation to cause the proper
> defaults of 0 to be visible to the guest.
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Kevin Tian , with one small comment.

the effect of this patch should be more than not leaking syscall MSR.
what about making the subject clearer when check-in?

Thanks
Kevin

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/4] x86/vmx: Don't leak host syscall MSR state into HVM guests

[Jan Beulich]

>>> On 13.02.17 at 15:32,  wrote:
> hvm_hw_cpu->msr_flags is in fact the VMX dirty bitmap of MSRs needing to be
> restored when switching into guest context.  It should never have been part of
> the migration state to start with, and Xen must not make any decisions based
> on the value seen during restore.
> 
> Identify it as obsolete in the header files, consistently save it as zero and
> ignore it on restore.
> 
> The MSRs must be considered dirty during VMCS creation to cause the proper
> defaults of 0 to be visible to the guest.
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Jan Beulich 



___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH 1/4] x86/vmx: Don't leak host syscall MSR state into HVM guests

[Andrew Cooper]

hvm_hw_cpu->msr_flags is in fact the VMX dirty bitmap of MSRs needing to be
restored when switching into guest context.  It should never have been part of
the migration state to start with, and Xen must not make any decisions based
on the value seen during restore.

Identify it as obsolete in the header files, consistently save it as zero and
ignore it on restore.

The MSRs must be considered dirty during VMCS creation to cause the proper
defaults of 0 to be visible to the guest.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Jun Nakajima 
CC: Kevin Tian 
CC: Boris Ostrovsky 
CC: Suravee Suthikulpanit 
---
 xen/arch/x86/hvm/svm/svm.c | 2 +-
 xen/arch/x86/hvm/vmx/vmcs.c| 3 +++
 xen/arch/x86/hvm/vmx/vmx.c | 5 ++---
 xen/include/public/arch-x86/hvm/save.h | 4 ++--
 4 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 01c7b58..e09c102 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -348,7 +348,7 @@ static void svm_save_cpu_state(struct vcpu *v, struct 
hvm_hw_cpu *data)
 data->msr_cstar= vmcb->cstar;
 data->msr_syscall_mask = vmcb->sfmask;
 data->msr_efer = v->arch.hvm_vcpu.guest_efer;
-data->msr_flags= -1ULL;
+data->msr_flags= 0;
 }
 
 
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 0b77dbc..62340bb 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1090,6 +1090,9 @@ static int construct_vmcs(struct vcpu *v)
 vmx_disable_intercept_for_msr(v, MSR_IA32_BNDCFGS, MSR_TYPE_R | 
MSR_TYPE_W);
 }
 
+/* All guest MSR state is dirty. */
+v->arch.hvm_vmx.msr_state.flags = ((1u << VMX_MSR_COUNT) - 1);
+
 /* I/O access bitmap. */
 __vmwrite(IO_BITMAP_A, __pa(d->arch.hvm_domain.io_bitmap));
 __vmwrite(IO_BITMAP_B, __pa(d->arch.hvm_domain.io_bitmap) + PAGE_SIZE);
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index d3d98da..e3694b3 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -779,13 +779,12 @@ static int vmx_vmcs_restore(struct vcpu *v, struct 
hvm_hw_cpu *c)
 static void vmx_save_cpu_state(struct vcpu *v, struct hvm_hw_cpu *data)
 {
 struct vmx_msr_state *guest_state = >arch.hvm_vmx.msr_state;
-unsigned long guest_flags = guest_state->flags;
 
 data->shadow_gs = v->arch.hvm_vmx.shadow_gs;
 data->msr_cstar = v->arch.hvm_vmx.cstar;
 
 /* save msrs */
-data->msr_flags= guest_flags;
+data->msr_flags= 0;
 data->msr_lstar= guest_state->msrs[VMX_INDEX_MSR_LSTAR];
 data->msr_star = guest_state->msrs[VMX_INDEX_MSR_STAR];
 data->msr_syscall_mask = guest_state->msrs[VMX_INDEX_MSR_SYSCALL_MASK];
@@ -796,7 +795,7 @@ static void vmx_load_cpu_state(struct vcpu *v, struct 
hvm_hw_cpu *data)
 struct vmx_msr_state *guest_state = >arch.hvm_vmx.msr_state;
 
 /* restore msrs */
-guest_state->flags = data->msr_flags & 7;
+guest_state->flags = ((1u << VMX_MSR_COUNT) - 1);
 guest_state->msrs[VMX_INDEX_MSR_LSTAR]= data->msr_lstar;
 guest_state->msrs[VMX_INDEX_MSR_STAR] = data->msr_star;
 guest_state->msrs[VMX_INDEX_MSR_SYSCALL_MASK] = data->msr_syscall_mask;
diff --git a/xen/include/public/arch-x86/hvm/save.h 
b/xen/include/public/arch-x86/hvm/save.h
index 8d73b51..419a3b2 100644
--- a/xen/include/public/arch-x86/hvm/save.h
+++ b/xen/include/public/arch-x86/hvm/save.h
@@ -135,7 +135,7 @@ struct hvm_hw_cpu {
 uint64_t shadow_gs;
 
 /* msr content saved/restored. */
-uint64_t msr_flags;
+uint64_t msr_flags; /* Obsolete, ignored. */
 uint64_t msr_lstar;
 uint64_t msr_star;
 uint64_t msr_cstar;
@@ -249,7 +249,7 @@ struct hvm_hw_cpu_compat {
 uint64_t shadow_gs;
 
 /* msr content saved/restored. */
-uint64_t msr_flags;
+uint64_t msr_flags; /* Obsolete, ignored. */
 uint64_t msr_lstar;
 uint64_t msr_star;
 uint64_t msr_cstar;
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel