| Message ID | 20190501235203.1179-1-tamas@tklengyel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] x86/vmx: correctly gather gs_shadow value for current vCPU | expand |
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
>>> 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
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
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
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
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
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
> 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>
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) )
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(+)