* [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
@ 2019-05-01 23:52 ` Tamas K Lengyel
0 siblings, 0 replies; 18+ messages in thread
From: Tamas K Lengyel @ 2019-05-01 23:52 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Tamas K Lengyel, Wei Liu, Jun Nakajima,
Razvan Cojocaru, Andrew Cooper, Jan Beulich, Roger Pau Monne
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Xen-devel] [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
@ 2019-05-01 23:52 ` Tamas K Lengyel
0 siblings, 0 replies; 18+ messages in thread
From: Tamas K Lengyel @ 2019-05-01 23:52 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Tamas K Lengyel, Wei Liu, Jun Nakajima,
Razvan Cojocaru, Andrew Cooper, Jan Beulich, Roger Pau Monne
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
@ 2019-05-02 6:20 ` Razvan Cojocaru
0 siblings, 0 replies; 18+ messages in thread
From: Razvan Cojocaru @ 2019-05-02 6:20 UTC (permalink / raw)
To: Tamas K Lengyel, xen-devel
Cc: Kevin Tian, Wei Liu, Jun Nakajima, Andrew Cooper, Jan Beulich,
Roger Pau Monne
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
@ 2019-05-02 6:20 ` Razvan Cojocaru
0 siblings, 0 replies; 18+ messages in thread
From: Razvan Cojocaru @ 2019-05-02 6:20 UTC (permalink / raw)
To: Tamas K Lengyel, xen-devel
Cc: Kevin Tian, Wei Liu, Jun Nakajima, Andrew Cooper, Jan Beulich,
Roger Pau Monne
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
@ 2019-05-02 8:36 ` Jan Beulich
0 siblings, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2019-05-02 8:36 UTC (permalink / raw)
To: Razvan Cojocaru, xen-devel, Tamas K Lengyel
Cc: Andrew Cooper, Kevin Tian, Wei Liu, Jun Nakajima, Roger Pau Monne
>>> 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
@ 2019-05-02 8:36 ` Jan Beulich
0 siblings, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2019-05-02 8:36 UTC (permalink / raw)
To: Razvan Cojocaru, xen-devel, Tamas K Lengyel
Cc: Andrew Cooper, Kevin Tian, Wei Liu, Jun Nakajima, Roger Pau Monne
>>> 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
@ 2019-05-02 8:41 ` Razvan Cojocaru
0 siblings, 0 replies; 18+ messages in thread
From: Razvan Cojocaru @ 2019-05-02 8:41 UTC (permalink / raw)
To: Jan Beulich, xen-devel, Tamas K Lengyel
Cc: Andrew Cooper, Kevin Tian, Wei Liu, Jun Nakajima, Roger Pau Monne
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
@ 2019-05-02 8:41 ` Razvan Cojocaru
0 siblings, 0 replies; 18+ messages in thread
From: Razvan Cojocaru @ 2019-05-02 8:41 UTC (permalink / raw)
To: Jan Beulich, xen-devel, Tamas K Lengyel
Cc: Andrew Cooper, Kevin Tian, Wei Liu, Jun Nakajima, Roger Pau Monne
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
@ 2019-05-02 8:45 ` Andrew Cooper
0 siblings, 0 replies; 18+ messages in thread
From: Andrew Cooper @ 2019-05-02 8:45 UTC (permalink / raw)
To: Razvan Cojocaru, Tamas K Lengyel, xen-devel
Cc: Kevin Tian, Wei Liu, Jan Beulich, Jun Nakajima, Roger Pau Monne
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
@ 2019-05-02 8:45 ` Andrew Cooper
0 siblings, 0 replies; 18+ messages in thread
From: Andrew Cooper @ 2019-05-02 8:45 UTC (permalink / raw)
To: Razvan Cojocaru, Tamas K Lengyel, xen-devel
Cc: Kevin Tian, Wei Liu, Jan Beulich, Jun Nakajima, Roger Pau Monne
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
@ 2019-05-02 8:50 ` Razvan Cojocaru
0 siblings, 0 replies; 18+ messages in thread
From: Razvan Cojocaru @ 2019-05-02 8:50 UTC (permalink / raw)
To: Andrew Cooper, Tamas K Lengyel, xen-devel
Cc: Kevin Tian, Wei Liu, Jan Beulich, Jun Nakajima, Roger Pau Monne
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
@ 2019-05-02 8:50 ` Razvan Cojocaru
0 siblings, 0 replies; 18+ messages in thread
From: Razvan Cojocaru @ 2019-05-02 8:50 UTC (permalink / raw)
To: Andrew Cooper, Tamas K Lengyel, xen-devel
Cc: Kevin Tian, Wei Liu, Jan Beulich, Jun Nakajima, Roger Pau Monne
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
@ 2019-05-02 10:45 ` Andrew Cooper
0 siblings, 0 replies; 18+ messages in thread
From: Andrew Cooper @ 2019-05-02 10:45 UTC (permalink / raw)
To: Tamas K Lengyel, xen-devel
Cc: Kevin Tian, Wei Liu, Jun Nakajima, Razvan Cojocaru, Jan Beulich,
Roger Pau Monne
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
@ 2019-05-02 10:45 ` Andrew Cooper
0 siblings, 0 replies; 18+ messages in thread
From: Andrew Cooper @ 2019-05-02 10:45 UTC (permalink / raw)
To: Tamas K Lengyel, xen-devel
Cc: Kevin Tian, Wei Liu, Jun Nakajima, Razvan Cojocaru, Jan Beulich,
Roger Pau Monne
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
@ 2019-05-02 13:14 ` Tamas K Lengyel
0 siblings, 0 replies; 18+ messages in thread
From: Tamas K Lengyel @ 2019-05-02 13:14 UTC (permalink / raw)
To: Andrew Cooper
Cc: Kevin Tian, Wei Liu, Jun Nakajima, Razvan Cojocaru, Jan Beulich,
Xen-devel, Roger Pau Monne
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
@ 2019-05-02 13:14 ` Tamas K Lengyel
0 siblings, 0 replies; 18+ messages in thread
From: Tamas K Lengyel @ 2019-05-02 13:14 UTC (permalink / raw)
To: Andrew Cooper
Cc: Kevin Tian, Wei Liu, Jun Nakajima, Razvan Cojocaru, Jan Beulich,
Xen-devel, Roger Pau Monne
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
@ 2019-05-11 0:04 ` Tian, Kevin
0 siblings, 0 replies; 18+ messages in thread
From: Tian, Kevin @ 2019-05-11 0:04 UTC (permalink / raw)
To: Tamas K Lengyel, xen-devel
Cc: Wei Liu, Nakajima, Jun, Razvan Cojocaru, Andrew Cooper,
Jan Beulich, Roger Pau Monne
> 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU
@ 2019-05-11 0:04 ` Tian, Kevin
0 siblings, 0 replies; 18+ messages in thread
From: Tian, Kevin @ 2019-05-11 0:04 UTC (permalink / raw)
To: Tamas K Lengyel, xen-devel
Cc: Wei Liu, Nakajima, Jun, Razvan Cojocaru, Andrew Cooper,
Jan Beulich, Roger Pau Monne
> 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
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2019-05-11 0:05 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-01 23:52 [PATCH v2] x86/vmx: correctly gather gs_shadow value for current vCPU Tamas K Lengyel
2019-05-01 23:52 ` [Xen-devel] " Tamas K Lengyel
2019-05-02 6:20 ` Razvan Cojocaru
2019-05-02 6:20 ` [Xen-devel] " Razvan Cojocaru
2019-05-02 8:36 ` Jan Beulich
2019-05-02 8:36 ` [Xen-devel] " Jan Beulich
2019-05-02 8:41 ` Razvan Cojocaru
2019-05-02 8:41 ` [Xen-devel] " Razvan Cojocaru
2019-05-02 8:45 ` Andrew Cooper
2019-05-02 8:45 ` [Xen-devel] " Andrew Cooper
2019-05-02 8:50 ` Razvan Cojocaru
2019-05-02 8:50 ` [Xen-devel] " Razvan Cojocaru
2019-05-02 10:45 ` Andrew Cooper
2019-05-02 10:45 ` [Xen-devel] " Andrew Cooper
2019-05-02 13:14 ` Tamas K Lengyel
2019-05-02 13:14 ` [Xen-devel] " Tamas K Lengyel
2019-05-11 0:04 ` Tian, Kevin
2019-05-11 0:04 ` [Xen-devel] " Tian, Kevin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.