1. Patchew
  2. Xen
  3. View series

[Xen-devel] [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU

Tamas K Lengyel posted 1 patch 4 years, 11 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20190501235203.1179-1-tamas@tklengyel.com
xen/arch/x86/hvm/vmx/vmx.c | 5 +++++
1 file changed, 5 insertions(+)
[Xen-devel] [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
Posted by Tamas K Lengyel 4 years, 11 months ago 
Currently the gs_shadow value is only cached when the vCPU is being scheduled
out by Xen. Reporting this (usually) stale value through vm_event is incorrect,
since it doesn't represent the actual state of the vCPU at the time the event
was recorded. This prevents vm_event subscribers from correctly finding kernel
structures in the guest when it is trapped while in ring3.

Refresh shadow_gs value when the context being saved is for the current vCPU.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Roger Pau Monne <roger.pau@citrix.com>
---
v2: move fix to hvm so vm_event doesn't have to know specifics
---
 xen/arch/x86/hvm/vmx/vmx.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 283eb7b34d..5154ecc2a8 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -779,12 +779,17 @@ static void vmx_load_cpu_state(struct vcpu *v, struct hvm_hw_cpu *data)
 
 static void vmx_save_vmcs_ctxt(struct vcpu *v, struct hvm_hw_cpu *ctxt)
 {
+    if ( v == current )
+        vmx_save_guest_msrs(v);
+
     vmx_save_cpu_state(v, ctxt);
     vmx_vmcs_save(v, ctxt);
 }
 
 static int vmx_load_vmcs_ctxt(struct vcpu *v, struct hvm_hw_cpu *ctxt)
 {
+    ASSERT(v != current);
+
     vmx_load_cpu_state(v, ctxt);
 
     if ( vmx_vmcs_restore(v, ctxt) )
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
Posted by Razvan Cojocaru 4 years, 11 months ago 
On 5/2/19 2:52 AM, Tamas K Lengyel wrote:
> Currently the gs_shadow value is only cached when the vCPU is being scheduled
> out by Xen. Reporting this (usually) stale value through vm_event is incorrect,
> since it doesn't represent the actual state of the vCPU at the time the event
> was recorded. This prevents vm_event subscribers from correctly finding kernel
> structures in the guest when it is trapped while in ring3.
> 
> Refresh shadow_gs value when the context being saved is for the current vCPU.
> 
> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Cc: Jun Nakajima <jun.nakajima@intel.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Roger Pau Monne <roger.pau@citrix.com>
> ---
> v2: move fix to hvm so vm_event doesn't have to know specifics
> ---
>  xen/arch/x86/hvm/vmx/vmx.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 283eb7b34d..5154ecc2a8 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -779,12 +779,17 @@ static void vmx_load_cpu_state(struct vcpu *v, struct hvm_hw_cpu *data)
>  
>  static void vmx_save_vmcs_ctxt(struct vcpu *v, struct hvm_hw_cpu *ctxt)
>  {
> +    if ( v == current )
> +        vmx_save_guest_msrs(v);

vmx_save_guest_msrs() is simple enough that the if is probably not
necessary here (we can just call the function directly, regardless of
what v holds).

But that's not technically an issue, so if nobody else minds:

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


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
Posted by Andrew Cooper 4 years, 11 months ago 
On 02/05/2019 07:20, Razvan Cojocaru wrote:
> On 5/2/19 2:52 AM, Tamas K Lengyel wrote:
>> Currently the gs_shadow value is only cached when the vCPU is being scheduled
>> out by Xen. Reporting this (usually) stale value through vm_event is incorrect,
>> since it doesn't represent the actual state of the vCPU at the time the event
>> was recorded. This prevents vm_event subscribers from correctly finding kernel
>> structures in the guest when it is trapped while in ring3.
>>
>> Refresh shadow_gs value when the context being saved is for the current vCPU.
>>
>> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
>> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> Cc: Jun Nakajima <jun.nakajima@intel.com>
>> Cc: Kevin Tian <kevin.tian@intel.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> Cc: Roger Pau Monne <roger.pau@citrix.com>
>> ---
>> v2: move fix to hvm so vm_event doesn't have to know specifics
>> ---
>>  xen/arch/x86/hvm/vmx/vmx.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>> index 283eb7b34d..5154ecc2a8 100644
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -779,12 +779,17 @@ static void vmx_load_cpu_state(struct vcpu *v, struct hvm_hw_cpu *data)
>>  
>>  static void vmx_save_vmcs_ctxt(struct vcpu *v, struct hvm_hw_cpu *ctxt)
>>  {
>> +    if ( v == current )
>> +        vmx_save_guest_msrs(v);
> vmx_save_guest_msrs() is simple enough that the if is probably not
> necessary here (we can just call the function directly, regardless of
> what v holds).

Why?  Doing that would fully corrupt v's state when called in remote
context, as you'd clobber v's gs_shadow which whatever the value was
from the current vcpu.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
Posted by Razvan Cojocaru 4 years, 11 months ago 
On 5/2/19 11:45 AM, Andrew Cooper wrote:
> On 02/05/2019 07:20, Razvan Cojocaru wrote:
>> On 5/2/19 2:52 AM, Tamas K Lengyel wrote:
>>> Currently the gs_shadow value is only cached when the vCPU is being scheduled
>>> out by Xen. Reporting this (usually) stale value through vm_event is incorrect,
>>> since it doesn't represent the actual state of the vCPU at the time the event
>>> was recorded. This prevents vm_event subscribers from correctly finding kernel
>>> structures in the guest when it is trapped while in ring3.
>>>
>>> Refresh shadow_gs value when the context being saved is for the current vCPU.
>>>
>>> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
>>> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>> Cc: Jun Nakajima <jun.nakajima@intel.com>
>>> Cc: Kevin Tian <kevin.tian@intel.com>
>>> Cc: Jan Beulich <jbeulich@suse.com>
>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Cc: Wei Liu <wei.liu2@citrix.com>
>>> Cc: Roger Pau Monne <roger.pau@citrix.com>
>>> ---
>>> v2: move fix to hvm so vm_event doesn't have to know specifics
>>> ---
>>>   xen/arch/x86/hvm/vmx/vmx.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>>> index 283eb7b34d..5154ecc2a8 100644
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -779,12 +779,17 @@ static void vmx_load_cpu_state(struct vcpu *v, struct hvm_hw_cpu *data)
>>>   
>>>   static void vmx_save_vmcs_ctxt(struct vcpu *v, struct hvm_hw_cpu *ctxt)
>>>   {
>>> +    if ( v == current )
>>> +        vmx_save_guest_msrs(v);
>> vmx_save_guest_msrs() is simple enough that the if is probably not
>> necessary here (we can just call the function directly, regardless of
>> what v holds).
> 
> Why?  Doing that would fully corrupt v's state when called in remote
> context, as you'd clobber v's gs_shadow which whatever the value was
> from the current vcpu.

Good point, I've missed that.


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
Posted by Jan Beulich 4 years, 11 months ago 
>>> On 02.05.19 at 08:20, <rcojocaru@bitdefender.com> wrote:
> On 5/2/19 2:52 AM, Tamas K Lengyel wrote:
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -779,12 +779,17 @@ static void vmx_load_cpu_state(struct vcpu *v, struct hvm_hw_cpu *data)
>>  
>>  static void vmx_save_vmcs_ctxt(struct vcpu *v, struct hvm_hw_cpu *ctxt)
>>  {
>> +    if ( v == current )
>> +        vmx_save_guest_msrs(v);
> 
> vmx_save_guest_msrs() is simple enough that the if is probably not
> necessary here (we can just call the function directly, regardless of
> what v holds).

Avoiding an MSR access is perhaps worth the conditional.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
Posted by Razvan Cojocaru 4 years, 11 months ago 
On 5/2/19 11:36 AM, Jan Beulich wrote:
>>>> On 02.05.19 at 08:20, <rcojocaru@bitdefender.com> wrote:
>> On 5/2/19 2:52 AM, Tamas K Lengyel wrote:
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -779,12 +779,17 @@ static void vmx_load_cpu_state(struct vcpu *v, struct hvm_hw_cpu *data)
>>>   
>>>   static void vmx_save_vmcs_ctxt(struct vcpu *v, struct hvm_hw_cpu *ctxt)
>>>   {
>>> +    if ( v == current )
>>> +        vmx_save_guest_msrs(v);
>>
>> vmx_save_guest_msrs() is simple enough that the if is probably not
>> necessary here (we can just call the function directly, regardless of
>> what v holds).
> 
> Avoiding an MSR access is perhaps worth the conditional.

Fair enough.


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
Posted by Andrew Cooper 4 years, 11 months ago 
On 02/05/2019 00:52, Tamas K Lengyel wrote:
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 283eb7b34d..5154ecc2a8 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -779,12 +779,17 @@ static void vmx_load_cpu_state(struct vcpu *v, struct hvm_hw_cpu *data)
>  
>  static void vmx_save_vmcs_ctxt(struct vcpu *v, struct hvm_hw_cpu *ctxt)
>  {
> +    if ( v == current )
> +        vmx_save_guest_msrs(v);
> +
>      vmx_save_cpu_state(v, ctxt);
>      vmx_vmcs_save(v, ctxt);
>  }
>  
>  static int vmx_load_vmcs_ctxt(struct vcpu *v, struct hvm_hw_cpu *ctxt)
>  {
> +    ASSERT(v != current);

I'd leave a comment along the lines of /* Not currently safe to use in
current context. */

Can be fixed up on commit.

This version is much cleaner, architecturally speaking, so Reviewed-by:
Andrew Cooper <andrew.cooper3@citrix.com>

I'll drop the previous version out of x86-next.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
Posted by Tamas K Lengyel 4 years, 11 months ago 
On Thu, May 2, 2019 at 4:46 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 02/05/2019 00:52, Tamas K Lengyel wrote:
> > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> > index 283eb7b34d..5154ecc2a8 100644
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -779,12 +779,17 @@ static void vmx_load_cpu_state(struct vcpu *v, struct hvm_hw_cpu *data)
> >
> >  static void vmx_save_vmcs_ctxt(struct vcpu *v, struct hvm_hw_cpu *ctxt)
> >  {
> > +    if ( v == current )
> > +        vmx_save_guest_msrs(v);
> > +
> >      vmx_save_cpu_state(v, ctxt);
> >      vmx_vmcs_save(v, ctxt);
> >  }
> >
> >  static int vmx_load_vmcs_ctxt(struct vcpu *v, struct hvm_hw_cpu *ctxt)
> >  {
> > +    ASSERT(v != current);
>
> I'd leave a comment along the lines of /* Not currently safe to use in
> current context. */
>
> Can be fixed up on commit.
>
> This version is much cleaner, architecturally speaking, so Reviewed-by:
> Andrew Cooper <andrew.cooper3@citrix.com>
>
> I'll drop the previous version out of x86-next.

Thanks,
Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
Posted by Tian, Kevin 4 years, 11 months ago 
> From: Tamas K Lengyel [mailto:tamas@tklengyel.com]
> Sent: Thursday, May 2, 2019 7:52 AM
> 
> Currently the gs_shadow value is only cached when the vCPU is being
> scheduled
> out by Xen. Reporting this (usually) stale value through vm_event is incorrect,
> since it doesn't represent the actual state of the vCPU at the time the event
> was recorded. This prevents vm_event subscribers from correctly finding
> kernel
> structures in the guest when it is trapped while in ring3.
> 
> Refresh shadow_gs value when the context being saved is for the current
> vCPU.
> 
> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.