[PATCH] VMX: don't crash processing 'd' debug key

[PATCH] VMX: don't crash processing 'd' debug key

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 2686 bytes --]

There's a window during scheduling where "current" and the active VMCS
may disagree: The former gets set much earlier than the latter. Since
both vmx_vmcs_enter() and vmx_vmcs_exit() immediately return when the
subject vCPU is "current", accessing VMCS fields would, depending on
whether there is any currently active VMCS, either read wrong data, or
cause a crash.

Going forward we might want to consider reducing the window during
which vmx_vmcs_enter() might fail (e.g. doing a plain __vmptrld() when
v->arch.hvm_vmx.vmcs != this_cpu(current_vmcs) but arch_vmx->active_cpu
== -1), but that would add complexities (acquiring and - more
importantly - properly dropping v->arch.hvm_vmx.vmcs_lock) that don't
look worthwhile adding right now.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -601,16 +601,16 @@ struct foreign_vmcs {
 };
 static DEFINE_PER_CPU(struct foreign_vmcs, foreign_vmcs);
 
-void vmx_vmcs_enter(struct vcpu *v)
+bool_t vmx_vmcs_enter(struct vcpu *v)
 {
     struct foreign_vmcs *fv;
 
     /*
      * NB. We must *always* run an HVM VCPU on its own VMCS, except for
-     * vmx_vmcs_enter/exit critical regions.
+     * vmx_vmcs_enter/exit and scheduling tail critical regions.
      */
     if ( likely(v == current) )
-        return;
+        return v->arch.hvm_vmx.vmcs == this_cpu(current_vmcs);
 
     fv = &this_cpu(foreign_vmcs);
 
@@ -633,6 +633,8 @@ void vmx_vmcs_enter(struct vcpu *v)
     }
 
     fv->count++;
+
+    return 1;
 }
 
 void vmx_vmcs_exit(struct vcpu *v)
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -675,7 +675,17 @@ void vmx_get_segment_register(struct vcp
 {
     unsigned long attr = 0, sel = 0, limit;
 
-    vmx_vmcs_enter(v);
+    /*
+     * We may get here in the context of dump_execstate(), which may have
+     * interrupted context switching between setting "current" and
+     * vmx_do_resume() reaching the end of vmx_load_vmcs(). That would make
+     * all the VMREADs below fail if we don't bail right away.
+     */
+    if ( unlikely(!vmx_vmcs_enter(v)) )
+    {
+        memset(reg, 0, sizeof(*reg));
+        return;
+    }
 
     switch ( seg )
     {
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -143,7 +143,7 @@ struct arch_vmx_struct {
 
 int vmx_create_vmcs(struct vcpu *v);
 void vmx_destroy_vmcs(struct vcpu *v);
-void vmx_vmcs_enter(struct vcpu *v);
+bool_t vmx_vmcs_enter(struct vcpu *v);
 void vmx_vmcs_exit(struct vcpu *v);
 
 #define CPU_BASED_VIRTUAL_INTR_PENDING        0x00000004




[-- Attachment #2: VMX-vmcs_enter-may-fail.patch --]
[-- Type: text/plain, Size: 2725 bytes --]

VMX: don't crash processing 'd' debug key

There's a window during scheduling where "current" and the active VMCS
may disagree: The former gets set much earlier than the latter. Since
both vmx_vmcs_enter() and vmx_vmcs_exit() immediately return when the
subject vCPU is "current", accessing VMCS fields would, depending on
whether there is any currently active VMCS, either read wrong data, or
cause a crash.

Going forward we might want to consider reducing the window during
which vmx_vmcs_enter() might fail (e.g. doing a plain __vmptrld() when
v->arch.hvm_vmx.vmcs != this_cpu(current_vmcs) but arch_vmx->active_cpu
== -1), but that would add complexities (acquiring and - more
importantly - properly dropping v->arch.hvm_vmx.vmcs_lock) that don't
look worthwhile adding right now.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -601,16 +601,16 @@ struct foreign_vmcs {
 };
 static DEFINE_PER_CPU(struct foreign_vmcs, foreign_vmcs);
 
-void vmx_vmcs_enter(struct vcpu *v)
+bool_t vmx_vmcs_enter(struct vcpu *v)
 {
     struct foreign_vmcs *fv;
 
     /*
      * NB. We must *always* run an HVM VCPU on its own VMCS, except for
-     * vmx_vmcs_enter/exit critical regions.
+     * vmx_vmcs_enter/exit and scheduling tail critical regions.
      */
     if ( likely(v == current) )
-        return;
+        return v->arch.hvm_vmx.vmcs == this_cpu(current_vmcs);
 
     fv = &this_cpu(foreign_vmcs);
 
@@ -633,6 +633,8 @@ void vmx_vmcs_enter(struct vcpu *v)
     }
 
     fv->count++;
+
+    return 1;
 }
 
 void vmx_vmcs_exit(struct vcpu *v)
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -675,7 +675,17 @@ void vmx_get_segment_register(struct vcp
 {
     unsigned long attr = 0, sel = 0, limit;
 
-    vmx_vmcs_enter(v);
+    /*
+     * We may get here in the context of dump_execstate(), which may have
+     * interrupted context switching between setting "current" and
+     * vmx_do_resume() reaching the end of vmx_load_vmcs(). That would make
+     * all the VMREADs below fail if we don't bail right away.
+     */
+    if ( unlikely(!vmx_vmcs_enter(v)) )
+    {
+        memset(reg, 0, sizeof(*reg));
+        return;
+    }
 
     switch ( seg )
     {
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -143,7 +143,7 @@ struct arch_vmx_struct {
 
 int vmx_create_vmcs(struct vcpu *v);
 void vmx_destroy_vmcs(struct vcpu *v);
-void vmx_vmcs_enter(struct vcpu *v);
+bool_t vmx_vmcs_enter(struct vcpu *v);
 void vmx_vmcs_exit(struct vcpu *v);
 
 #define CPU_BASED_VIRTUAL_INTR_PENDING        0x00000004

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] VMX: don't crash processing 'd' debug key

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 3158 bytes --]

On 07/11/13 10:44, Jan Beulich wrote:
> There's a window during scheduling where "current" and the active VMCS
> may disagree: The former gets set much earlier than the latter. Since
> both vmx_vmcs_enter() and vmx_vmcs_exit() immediately return when the
> subject vCPU is "current", accessing VMCS fields would, depending on
> whether there is any currently active VMCS, either read wrong data, or
> cause a crash.
>
> Going forward we might want to consider reducing the window during
> which vmx_vmcs_enter() might fail (e.g. doing a plain __vmptrld() when
> v->arch.hvm_vmx.vmcs != this_cpu(current_vmcs) but arch_vmx->active_cpu
> == -1), but that would add complexities (acquiring and - more
> importantly - properly dropping v->arch.hvm_vmx.vmcs_lock) that don't
> look worthwhile adding right now.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -601,16 +601,16 @@ struct foreign_vmcs {
>  };
>  static DEFINE_PER_CPU(struct foreign_vmcs, foreign_vmcs);
>  
> -void vmx_vmcs_enter(struct vcpu *v)
> +bool_t vmx_vmcs_enter(struct vcpu *v)
>  {
>      struct foreign_vmcs *fv;
>  
>      /*
>       * NB. We must *always* run an HVM VCPU on its own VMCS, except for
> -     * vmx_vmcs_enter/exit critical regions.
> +     * vmx_vmcs_enter/exit and scheduling tail critical regions.
>       */
>      if ( likely(v == current) )
> -        return;
> +        return v->arch.hvm_vmx.vmcs == this_cpu(current_vmcs);
>  
>      fv = &this_cpu(foreign_vmcs);
>  
> @@ -633,6 +633,8 @@ void vmx_vmcs_enter(struct vcpu *v)
>      }
>  
>      fv->count++;
> +
> +    return 1;
>  }
>  
>  void vmx_vmcs_exit(struct vcpu *v)
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -675,7 +675,17 @@ void vmx_get_segment_register(struct vcp
>  {
>      unsigned long attr = 0, sel = 0, limit;
>  
> -    vmx_vmcs_enter(v);
> +    /*
> +     * We may get here in the context of dump_execstate(), which may have
> +     * interrupted context switching between setting "current" and
> +     * vmx_do_resume() reaching the end of vmx_load_vmcs(). That would make
> +     * all the VMREADs below fail if we don't bail right away.
> +     */
> +    if ( unlikely(!vmx_vmcs_enter(v)) )
> +    {
> +        memset(reg, 0, sizeof(*reg));
> +        return;
> +    }

What are the implications of this?  All callers unconditionally expect
this to succeed, and use the results straight as-are.

On the other hand, I am not certain how we could go about dealing with
the error.

~Andrew

>  
>      switch ( seg )
>      {
> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -143,7 +143,7 @@ struct arch_vmx_struct {
>  
>  int vmx_create_vmcs(struct vcpu *v);
>  void vmx_destroy_vmcs(struct vcpu *v);
> -void vmx_vmcs_enter(struct vcpu *v);
> +bool_t vmx_vmcs_enter(struct vcpu *v);
>  void vmx_vmcs_exit(struct vcpu *v);
>  
>  #define CPU_BASED_VIRTUAL_INTR_PENDING        0x00000004
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 3906 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] VMX: don't crash processing 'd' debug key

[Jan Beulich]

>>> On 07.11.13 at 15:34, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 07/11/13 10:44, Jan Beulich wrote:
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -675,7 +675,17 @@ void vmx_get_segment_register(struct vcp
>>  {
>>      unsigned long attr = 0, sel = 0, limit;
>>  
>> -    vmx_vmcs_enter(v);
>> +    /*
>> +     * We may get here in the context of dump_execstate(), which may have
>> +     * interrupted context switching between setting "current" and
>> +     * vmx_do_resume() reaching the end of vmx_load_vmcs(). That would make
>> +     * all the VMREADs below fail if we don't bail right away.
>> +     */
>> +    if ( unlikely(!vmx_vmcs_enter(v)) )
>> +    {
>> +        memset(reg, 0, sizeof(*reg));
>> +        return;
>> +    }
> 
> What are the implications of this?  All callers unconditionally expect
> this to succeed, and use the results straight as-are.
> 
> On the other hand, I am not certain how we could go about dealing with
> the error.

The thing is that for all "normal" code paths we won't get there.
Hence it'll only be that - rather than crashing - no useful
information will be displayed for the debug key. (And obviously,
should vmx_vmcs_enter() nevertheless end up failing on a
"normal" code path, all callers properly looking at the results
should produce #GP faults to the guest [present bit clear,
minimal permissions, and segment limit zero].)

Jan

Re: [PATCH] VMX: don't crash processing 'd' debug key

[Andrew Cooper]

On 07/11/13 14:49, Jan Beulich wrote:
>>>> On 07.11.13 at 15:34, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 07/11/13 10:44, Jan Beulich wrote:
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -675,7 +675,17 @@ void vmx_get_segment_register(struct vcp
>>>  {
>>>      unsigned long attr = 0, sel = 0, limit;
>>>  
>>> -    vmx_vmcs_enter(v);
>>> +    /*
>>> +     * We may get here in the context of dump_execstate(), which may have
>>> +     * interrupted context switching between setting "current" and
>>> +     * vmx_do_resume() reaching the end of vmx_load_vmcs(). That would make
>>> +     * all the VMREADs below fail if we don't bail right away.
>>> +     */
>>> +    if ( unlikely(!vmx_vmcs_enter(v)) )
>>> +    {
>>> +        memset(reg, 0, sizeof(*reg));
>>> +        return;
>>> +    }
>> What are the implications of this?  All callers unconditionally expect
>> this to succeed, and use the results straight as-are.
>>
>> On the other hand, I am not certain how we could go about dealing with
>> the error.
> The thing is that for all "normal" code paths we won't get there.
> Hence it'll only be that - rather than crashing - no useful
> information will be displayed for the debug key. (And obviously,
> should vmx_vmcs_enter() nevertheless end up failing on a
> "normal" code path, all callers properly looking at the results
> should produce #GP faults to the guest [present bit clear,
> minimal permissions, and segment limit zero].)
>
> Jan
>

Hmm yes.  I certainly cant see any normal codepaths which would get
here, and the crash path certainly needs fixing.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH] VMX: don't crash processing 'd' debug key

[Tim Deegan]

At 10:44 +0000 on 07 Nov (1383817496), Jan Beulich wrote:
> There's a window during scheduling where "current" and the active VMCS
> may disagree: The former gets set much earlier than the latter. Since
> both vmx_vmcs_enter() and vmx_vmcs_exit() immediately return when the
> subject vCPU is "current", accessing VMCS fields would, depending on
> whether there is any currently active VMCS, either read wrong data, or
> cause a crash.
> 
> Going forward we might want to consider reducing the window during
> which vmx_vmcs_enter() might fail (e.g. doing a plain __vmptrld() when
> v->arch.hvm_vmx.vmcs != this_cpu(current_vmcs) but arch_vmx->active_cpu
> == -1), but that would add complexities (acquiring and - more
> importantly - properly dropping v->arch.hvm_vmx.vmcs_lock) that don't
> look worthwhile adding right now.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -601,16 +601,16 @@ struct foreign_vmcs {
>  };
>  static DEFINE_PER_CPU(struct foreign_vmcs, foreign_vmcs);
>  
> -void vmx_vmcs_enter(struct vcpu *v)
> +bool_t vmx_vmcs_enter(struct vcpu *v)
>  {
>      struct foreign_vmcs *fv;
>  
>      /*
>       * NB. We must *always* run an HVM VCPU on its own VMCS, except for
> -     * vmx_vmcs_enter/exit critical regions.
> +     * vmx_vmcs_enter/exit and scheduling tail critical regions.
>       */
>      if ( likely(v == current) )
> -        return;
> +        return v->arch.hvm_vmx.vmcs == this_cpu(current_vmcs);
>  
>      fv = &this_cpu(foreign_vmcs);
>  
> @@ -633,6 +633,8 @@ void vmx_vmcs_enter(struct vcpu *v)
>      }
>  
>      fv->count++;
> +
> +    return 1;
>  }
>  
>  void vmx_vmcs_exit(struct vcpu *v)
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -675,7 +675,17 @@ void vmx_get_segment_register(struct vcp
>  {
>      unsigned long attr = 0, sel = 0, limit;
>  
> -    vmx_vmcs_enter(v);
> +    /*
> +     * We may get here in the context of dump_execstate(), which may have
> +     * interrupted context switching between setting "current" and
> +     * vmx_do_resume() reaching the end of vmx_load_vmcs(). That would make
> +     * all the VMREADs below fail if we don't bail right away.
> +     */
> +    if ( unlikely(!vmx_vmcs_enter(v)) )
> +    {
> +        memset(reg, 0, sizeof(*reg));
> +        return;

It would be nice to print something here, at least on the first
instance.  Otherwise someone looking at bizarre debugkey output would
have to know (and remember) about this path.

I'd also be inclined to ASSERT that, e.g. interrupts are disabled here
-- if for any reason this function ever starts corrupting register
state on other paths, we'll want to know about it quickly!

Cheers,

Tim.

Re: [PATCH] VMX: don't crash processing 'd' debug key

[Jan Beulich]

>>> On 07.11.13 at 20:08, Tim Deegan <tim@xen.org> wrote:
> At 10:44 +0000 on 07 Nov (1383817496), Jan Beulich wrote:
>> @@ -675,7 +675,17 @@ void vmx_get_segment_register(struct vcp
>>  {
>>      unsigned long attr = 0, sel = 0, limit;
>>  
>> -    vmx_vmcs_enter(v);
>> +    /*
>> +     * We may get here in the context of dump_execstate(), which may have
>> +     * interrupted context switching between setting "current" and
>> +     * vmx_do_resume() reaching the end of vmx_load_vmcs(). That would make
>> +     * all the VMREADs below fail if we don't bail right away.
>> +     */
>> +    if ( unlikely(!vmx_vmcs_enter(v)) )
>> +    {
>> +        memset(reg, 0, sizeof(*reg));
>> +        return;
> 
> It would be nice to print something here, at least on the first
> instance.  Otherwise someone looking at bizarre debugkey output would
> have to know (and remember) about this path.

Did this.

> I'd also be inclined to ASSERT that, e.g. interrupts are disabled here
> -- if for any reason this function ever starts corrupting register
> state on other paths, we'll want to know about it quickly!

But I'm rather hesitant to do this. If anything, we'd need per-CPU
state tracking whether we're in do_invalid_op()'s main switch.

Jan

Re: [PATCH] VMX: don't crash processing 'd' debug key

[Andrew Cooper]

On 08/11/13 16:04, Jan Beulich wrote:
>>>> On 07.11.13 at 20:08, Tim Deegan <tim@xen.org> wrote:
>> At 10:44 +0000 on 07 Nov (1383817496), Jan Beulich wrote:
>>> @@ -675,7 +675,17 @@ void vmx_get_segment_register(struct vcp
>>>  {
>>>      unsigned long attr = 0, sel = 0, limit;
>>>  
>>> -    vmx_vmcs_enter(v);
>>> +    /*
>>> +     * We may get here in the context of dump_execstate(), which may have
>>> +     * interrupted context switching between setting "current" and
>>> +     * vmx_do_resume() reaching the end of vmx_load_vmcs(). That would make
>>> +     * all the VMREADs below fail if we don't bail right away.
>>> +     */
>>> +    if ( unlikely(!vmx_vmcs_enter(v)) )
>>> +    {
>>> +        memset(reg, 0, sizeof(*reg));
>>> +        return;
>> It would be nice to print something here, at least on the first
>> instance.  Otherwise someone looking at bizarre debugkey output would
>> have to know (and remember) about this path.
> Did this.
>
>> I'd also be inclined to ASSERT that, e.g. interrupts are disabled here
>> -- if for any reason this function ever starts corrupting register
>> state on other paths, we'll want to know about it quickly!
> But I'm rather hesitant to do this. If anything, we'd need per-CPU
> state tracking whether we're in do_invalid_op()'s main switch.
>
> Jan
>

I agree - the debug keys are hardly normal operation, and we don't want
to ASSERT() in a debugkey.

Perhaps an alternative would be a short printk indicating that if this
is debugkey then the caller was unlucky and should try again, as we know
there is a short vulnerable window?

> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] VMX: don't crash processing 'd' debug key

[Andrew Cooper]

On 08/11/13 16:09, Andrew Cooper wrote:
> On 08/11/13 16:04, Jan Beulich wrote:
>>>>> On 07.11.13 at 20:08, Tim Deegan <tim@xen.org> wrote:
>>> At 10:44 +0000 on 07 Nov (1383817496), Jan Beulich wrote:
>>>> @@ -675,7 +675,17 @@ void vmx_get_segment_register(struct vcp
>>>>  {
>>>>      unsigned long attr = 0, sel = 0, limit;
>>>>  
>>>> -    vmx_vmcs_enter(v);
>>>> +    /*
>>>> +     * We may get here in the context of dump_execstate(), which may have
>>>> +     * interrupted context switching between setting "current" and
>>>> +     * vmx_do_resume() reaching the end of vmx_load_vmcs(). That would make
>>>> +     * all the VMREADs below fail if we don't bail right away.
>>>> +     */
>>>> +    if ( unlikely(!vmx_vmcs_enter(v)) )
>>>> +    {
>>>> +        memset(reg, 0, sizeof(*reg));
>>>> +        return;
>>> It would be nice to print something here, at least on the first
>>> instance.  Otherwise someone looking at bizarre debugkey output would
>>> have to know (and remember) about this path.
>> Did this.
>>
>>> I'd also be inclined to ASSERT that, e.g. interrupts are disabled here
>>> -- if for any reason this function ever starts corrupting register
>>> state on other paths, we'll want to know about it quickly!
>> But I'm rather hesitant to do this. If anything, we'd need per-CPU
>> state tracking whether we're in do_invalid_op()'s main switch.
>>
>> Jan
>>
> I agree - the debug keys are hardly normal operation, and we don't want
> to ASSERT() in a debugkey.
>
> Perhaps an alternative would be a short printk indicating that if this
> is debugkey then the caller was unlucky and should try again, as we know
> there is a short vulnerable window?

I should have waited and read v2 - that looks fine to me.  Sorry for the
noise.

~Andrew

Re: [PATCH] VMX: don't crash processing 'd' debug key

[Jan Beulich]

>>> On 08.11.13 at 17:09, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 08/11/13 16:04, Jan Beulich wrote:
>>>>> On 07.11.13 at 20:08, Tim Deegan <tim@xen.org> wrote:
>>> At 10:44 +0000 on 07 Nov (1383817496), Jan Beulich wrote:
>>>> @@ -675,7 +675,17 @@ void vmx_get_segment_register(struct vcp
>>>>  {
>>>>      unsigned long attr = 0, sel = 0, limit;
>>>>  
>>>> -    vmx_vmcs_enter(v);
>>>> +    /*
>>>> +     * We may get here in the context of dump_execstate(), which may have
>>>> +     * interrupted context switching between setting "current" and
>>>> +     * vmx_do_resume() reaching the end of vmx_load_vmcs(). That would make
>>>> +     * all the VMREADs below fail if we don't bail right away.
>>>> +     */
>>>> +    if ( unlikely(!vmx_vmcs_enter(v)) )
>>>> +    {
>>>> +        memset(reg, 0, sizeof(*reg));
>>>> +        return;
>>> It would be nice to print something here, at least on the first
>>> instance.  Otherwise someone looking at bizarre debugkey output would
>>> have to know (and remember) about this path.
>> Did this.
>>
>>> I'd also be inclined to ASSERT that, e.g. interrupts are disabled here
>>> -- if for any reason this function ever starts corrupting register
>>> state on other paths, we'll want to know about it quickly!
>> But I'm rather hesitant to do this. If anything, we'd need per-CPU
>> state tracking whether we're in do_invalid_op()'s main switch.
> 
> I agree - the debug keys are hardly normal operation, and we don't want
> to ASSERT() in a debugkey.
> 
> Perhaps an alternative would be a short printk indicating that if this
> is debugkey then the caller was unlucky and should try again, as we know
> there is a short vulnerable window?

Hence the addition of a one time printk() as Tim suggested.

Jan

Re: [PATCH] VMX: don't crash processing 'd' debug key

[Tim Deegan]

At 16:09 +0000 on 08 Nov (1383923399), Andrew Cooper wrote:
> On 08/11/13 16:04, Jan Beulich wrote:
> >>>> On 07.11.13 at 20:08, Tim Deegan <tim@xen.org> wrote:
> >> At 10:44 +0000 on 07 Nov (1383817496), Jan Beulich wrote:
> >>> @@ -675,7 +675,17 @@ void vmx_get_segment_register(struct vcp
> >>>  {
> >>>      unsigned long attr = 0, sel = 0, limit;
> >>>  
> >>> -    vmx_vmcs_enter(v);
> >>> +    /*
> >>> +     * We may get here in the context of dump_execstate(), which may have
> >>> +     * interrupted context switching between setting "current" and
> >>> +     * vmx_do_resume() reaching the end of vmx_load_vmcs(). That would make
> >>> +     * all the VMREADs below fail if we don't bail right away.
> >>> +     */
> >>> +    if ( unlikely(!vmx_vmcs_enter(v)) )
> >>> +    {
> >>> +        memset(reg, 0, sizeof(*reg));
> >>> +        return;
> >> It would be nice to print something here, at least on the first
> >> instance.  Otherwise someone looking at bizarre debugkey output would
> >> have to know (and remember) about this path.
> > Did this.
> >
> >> I'd also be inclined to ASSERT that, e.g. interrupts are disabled here
> >> -- if for any reason this function ever starts corrupting register
> >> state on other paths, we'll want to know about it quickly!
> > But I'm rather hesitant to do this. If anything, we'd need per-CPU
> > state tracking whether we're in do_invalid_op()'s main switch.
> >
> > Jan
> >
> 
> I agree - the debug keys are hardly normal operation, and we don't want
> to ASSERT() in a debugkey.

But this code is also (and mode commonly) called from non-debugkey
paths, which are what the ASSERT would be guarding.

An alternative would be to plumb a 'tolerate failure' flag down the
stack and use that only from the debugkey handler.  

That kind of protection might be nice for vmx_vmcs_enter() too: make
it into vmx_vmcs_enter_try() and have vmx_vmcs_enter() be a wrapper
taht ASSERT()s success.

Tim.

[PATCH v3] VMX: don't crash processing 'd' debug key

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3338 bytes --]

There's a window during scheduling where "current" and the active VMCS
may disagree: The former gets set much earlier than the latter. Since
both vmx_vmcs_enter() and vmx_vmcs_exit() immediately return when the
subject vCPU is "current", accessing VMCS fields would, depending on
whether there is any currently active VMCS, either read wrong data, or
cause a crash.

Going forward we might want to consider reducing the window during
which vmx_vmcs_enter() might fail (e.g. doing a plain __vmptrld() when
v->arch.hvm_vmx.vmcs != this_cpu(current_vmcs) but arch_vmx->active_cpu
== -1), but that would add complexities (acquiring and - more
importantly - properly dropping v->arch.hvm_vmx.vmcs_lock) that don't
look worthwhile adding right now.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v3: Introduce vmx_vmcs_try_enter() (as suggested by Tim).

--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -601,16 +601,16 @@ struct foreign_vmcs {
 };
 static DEFINE_PER_CPU(struct foreign_vmcs, foreign_vmcs);
 
-void vmx_vmcs_enter(struct vcpu *v)
+bool_t vmx_vmcs_try_enter(struct vcpu *v)
 {
     struct foreign_vmcs *fv;
 
     /*
      * NB. We must *always* run an HVM VCPU on its own VMCS, except for
-     * vmx_vmcs_enter/exit critical regions.
+     * vmx_vmcs_enter/exit and scheduling tail critical regions.
      */
     if ( likely(v == current) )
-        return;
+        return v->arch.hvm_vmx.vmcs == this_cpu(current_vmcs);
 
     fv = &this_cpu(foreign_vmcs);
 
@@ -633,6 +633,15 @@ void vmx_vmcs_enter(struct vcpu *v)
     }
 
     fv->count++;
+
+    return 1;
+}
+
+void vmx_vmcs_enter(struct vcpu *v)
+{
+    bool_t okay = vmx_vmcs_try_enter(v);
+
+    ASSERT(okay);
 }
 
 void vmx_vmcs_exit(struct vcpu *v)
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -675,7 +675,27 @@ void vmx_get_segment_register(struct vcp
 {
     unsigned long attr = 0, sel = 0, limit;
 
-    vmx_vmcs_enter(v);
+    /*
+     * We may get here in the context of dump_execstate(), which may have
+     * interrupted context switching between setting "current" and
+     * vmx_do_resume() reaching the end of vmx_load_vmcs(). That would make
+     * all the VMREADs below fail if we don't bail right away.
+     */
+    if ( unlikely(!vmx_vmcs_try_enter(v)) )
+    {
+        static bool_t warned;
+
+        if ( !warned )
+        {
+            warned = 1;
+            printk(XENLOG_WARNING "Segment register inaccessible for d%dv%d\n"
+                   "(If you see this outside of debugging activity,"
+                   " please report to xen-devel@lists.xenproject.org)\n",
+                   v->domain->domain_id, v->vcpu_id);
+        }
+        memset(reg, 0, sizeof(*reg));
+        return;
+    }
 
     switch ( seg )
     {
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -144,6 +144,7 @@ struct arch_vmx_struct {
 int vmx_create_vmcs(struct vcpu *v);
 void vmx_destroy_vmcs(struct vcpu *v);
 void vmx_vmcs_enter(struct vcpu *v);
+bool_t __must_check vmx_vmcs_try_enter(struct vcpu *v);
 void vmx_vmcs_exit(struct vcpu *v);
 
 #define CPU_BASED_VIRTUAL_INTR_PENDING        0x00000004




[-- Attachment #2: VMX-vmcs_enter-may-fail.patch --]
[-- Type: text/plain, Size: 3377 bytes --]

VMX: don't crash processing 'd' debug key

There's a window during scheduling where "current" and the active VMCS
may disagree: The former gets set much earlier than the latter. Since
both vmx_vmcs_enter() and vmx_vmcs_exit() immediately return when the
subject vCPU is "current", accessing VMCS fields would, depending on
whether there is any currently active VMCS, either read wrong data, or
cause a crash.

Going forward we might want to consider reducing the window during
which vmx_vmcs_enter() might fail (e.g. doing a plain __vmptrld() when
v->arch.hvm_vmx.vmcs != this_cpu(current_vmcs) but arch_vmx->active_cpu
== -1), but that would add complexities (acquiring and - more
importantly - properly dropping v->arch.hvm_vmx.vmcs_lock) that don't
look worthwhile adding right now.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v3: Introduce vmx_vmcs_try_enter() (as suggested by Tim).

--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -601,16 +601,16 @@ struct foreign_vmcs {
 };
 static DEFINE_PER_CPU(struct foreign_vmcs, foreign_vmcs);
 
-void vmx_vmcs_enter(struct vcpu *v)
+bool_t vmx_vmcs_try_enter(struct vcpu *v)
 {
     struct foreign_vmcs *fv;
 
     /*
      * NB. We must *always* run an HVM VCPU on its own VMCS, except for
-     * vmx_vmcs_enter/exit critical regions.
+     * vmx_vmcs_enter/exit and scheduling tail critical regions.
      */
     if ( likely(v == current) )
-        return;
+        return v->arch.hvm_vmx.vmcs == this_cpu(current_vmcs);
 
     fv = &this_cpu(foreign_vmcs);
 
@@ -633,6 +633,15 @@ void vmx_vmcs_enter(struct vcpu *v)
     }
 
     fv->count++;
+
+    return 1;
+}
+
+void vmx_vmcs_enter(struct vcpu *v)
+{
+    bool_t okay = vmx_vmcs_try_enter(v);
+
+    ASSERT(okay);
 }
 
 void vmx_vmcs_exit(struct vcpu *v)
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -675,7 +675,27 @@ void vmx_get_segment_register(struct vcp
 {
     unsigned long attr = 0, sel = 0, limit;
 
-    vmx_vmcs_enter(v);
+    /*
+     * We may get here in the context of dump_execstate(), which may have
+     * interrupted context switching between setting "current" and
+     * vmx_do_resume() reaching the end of vmx_load_vmcs(). That would make
+     * all the VMREADs below fail if we don't bail right away.
+     */
+    if ( unlikely(!vmx_vmcs_try_enter(v)) )
+    {
+        static bool_t warned;
+
+        if ( !warned )
+        {
+            warned = 1;
+            printk(XENLOG_WARNING "Segment register inaccessible for d%dv%d\n"
+                   "(If you see this outside of debugging activity,"
+                   " please report to xen-devel@lists.xenproject.org)\n",
+                   v->domain->domain_id, v->vcpu_id);
+        }
+        memset(reg, 0, sizeof(*reg));
+        return;
+    }
 
     switch ( seg )
     {
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -144,6 +144,7 @@ struct arch_vmx_struct {
 int vmx_create_vmcs(struct vcpu *v);
 void vmx_destroy_vmcs(struct vcpu *v);
 void vmx_vmcs_enter(struct vcpu *v);
+bool_t __must_check vmx_vmcs_try_enter(struct vcpu *v);
 void vmx_vmcs_exit(struct vcpu *v);
 
 #define CPU_BASED_VIRTUAL_INTR_PENDING        0x00000004

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3] VMX: don't crash processing 'd' debug key

[Keir Fraser]

On 11/11/2013 12:55, "Jan Beulich" <JBeulich@suse.com> wrote:

> There's a window during scheduling where "current" and the active VMCS
> may disagree: The former gets set much earlier than the latter. Since
> both vmx_vmcs_enter() and vmx_vmcs_exit() immediately return when the
> subject vCPU is "current", accessing VMCS fields would, depending on
> whether there is any currently active VMCS, either read wrong data, or
> cause a crash.
> 
> Going forward we might want to consider reducing the window during
> which vmx_vmcs_enter() might fail (e.g. doing a plain __vmptrld() when
> v->arch.hvm_vmx.vmcs != this_cpu(current_vmcs) but arch_vmx->active_cpu
> == -1), but that would add complexities (acquiring and - more
> importantly - properly dropping v->arch.hvm_vmx.vmcs_lock) that don't
> look worthwhile adding right now.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

This is a little ugly but I can't think of a nicer way.

Acked-by: Keir Fraser <keir@xen.org>

[PATCH v2] VMX: don't crash processing 'd' debug key

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3276 bytes --]

There's a window during scheduling where "current" and the active VMCS
may disagree: The former gets set much earlier than the latter. Since
both vmx_vmcs_enter() and vmx_vmcs_exit() immediately return when the
subject vCPU is "current", accessing VMCS fields would, depending on
whether there is any currently active VMCS, either read wrong data, or
cause a crash.

Going forward we might want to consider reducing the window during
which vmx_vmcs_enter() might fail (e.g. doing a plain __vmptrld() when
v->arch.hvm_vmx.vmcs != this_cpu(current_vmcs) but arch_vmx->active_cpu
== -1), but that would add complexities (acquiring and - more
importantly - properly dropping v->arch.hvm_vmx.vmcs_lock) that don't
look worthwhile adding right now.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v2: Log a message from vmx_get_segment_register() the first time we end
    up returning blank register state (as suggested by Tim).

--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -601,16 +601,16 @@ struct foreign_vmcs {
 };
 static DEFINE_PER_CPU(struct foreign_vmcs, foreign_vmcs);
 
-void vmx_vmcs_enter(struct vcpu *v)
+bool_t vmx_vmcs_enter(struct vcpu *v)
 {
     struct foreign_vmcs *fv;
 
     /*
      * NB. We must *always* run an HVM VCPU on its own VMCS, except for
-     * vmx_vmcs_enter/exit critical regions.
+     * vmx_vmcs_enter/exit and scheduling tail critical regions.
      */
     if ( likely(v == current) )
-        return;
+        return v->arch.hvm_vmx.vmcs == this_cpu(current_vmcs);
 
     fv = &this_cpu(foreign_vmcs);
 
@@ -633,6 +633,8 @@ void vmx_vmcs_enter(struct vcpu *v)
     }
 
     fv->count++;
+
+    return 1;
 }
 
 void vmx_vmcs_exit(struct vcpu *v)
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -675,7 +675,27 @@ void vmx_get_segment_register(struct vcp
 {
     unsigned long attr = 0, sel = 0, limit;
 
-    vmx_vmcs_enter(v);
+    /*
+     * We may get here in the context of dump_execstate(), which may have
+     * interrupted context switching between setting "current" and
+     * vmx_do_resume() reaching the end of vmx_load_vmcs(). That would make
+     * all the VMREADs below fail if we don't bail right away.
+     */
+    if ( unlikely(!vmx_vmcs_enter(v)) )
+    {
+        static bool_t warned;
+
+        if ( !warned )
+        {
+            warned = 1;
+            printk(XENLOG_WARNING "Segment register inaccessible for d%dv%d\n"
+                   "(If you see this outside of debugging activity,"
+                   " please report to xen-devel@lists.xenproject.org)\n",
+                   v->domain->domain_id, v->vcpu_id);
+        }
+        memset(reg, 0, sizeof(*reg));
+        return;
+    }
 
     switch ( seg )
     {
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -143,7 +143,7 @@ struct arch_vmx_struct {
 
 int vmx_create_vmcs(struct vcpu *v);
 void vmx_destroy_vmcs(struct vcpu *v);
-void vmx_vmcs_enter(struct vcpu *v);
+bool_t vmx_vmcs_enter(struct vcpu *v);
 void vmx_vmcs_exit(struct vcpu *v);
 
 #define CPU_BASED_VIRTUAL_INTR_PENDING        0x00000004




[-- Attachment #2: VMX-vmcs_enter-may-fail.patch --]
[-- Type: text/plain, Size: 3315 bytes --]

VMX: don't crash processing 'd' debug key

There's a window during scheduling where "current" and the active VMCS
may disagree: The former gets set much earlier than the latter. Since
both vmx_vmcs_enter() and vmx_vmcs_exit() immediately return when the
subject vCPU is "current", accessing VMCS fields would, depending on
whether there is any currently active VMCS, either read wrong data, or
cause a crash.

Going forward we might want to consider reducing the window during
which vmx_vmcs_enter() might fail (e.g. doing a plain __vmptrld() when
v->arch.hvm_vmx.vmcs != this_cpu(current_vmcs) but arch_vmx->active_cpu
== -1), but that would add complexities (acquiring and - more
importantly - properly dropping v->arch.hvm_vmx.vmcs_lock) that don't
look worthwhile adding right now.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v2: Log a message from vmx_get_segment_register() the first time we end
    up returning blank register state (as suggested by Tim).

--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -601,16 +601,16 @@ struct foreign_vmcs {
 };
 static DEFINE_PER_CPU(struct foreign_vmcs, foreign_vmcs);
 
-void vmx_vmcs_enter(struct vcpu *v)
+bool_t vmx_vmcs_enter(struct vcpu *v)
 {
     struct foreign_vmcs *fv;
 
     /*
      * NB. We must *always* run an HVM VCPU on its own VMCS, except for
-     * vmx_vmcs_enter/exit critical regions.
+     * vmx_vmcs_enter/exit and scheduling tail critical regions.
      */
     if ( likely(v == current) )
-        return;
+        return v->arch.hvm_vmx.vmcs == this_cpu(current_vmcs);
 
     fv = &this_cpu(foreign_vmcs);
 
@@ -633,6 +633,8 @@ void vmx_vmcs_enter(struct vcpu *v)
     }
 
     fv->count++;
+
+    return 1;
 }
 
 void vmx_vmcs_exit(struct vcpu *v)
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -675,7 +675,27 @@ void vmx_get_segment_register(struct vcp
 {
     unsigned long attr = 0, sel = 0, limit;
 
-    vmx_vmcs_enter(v);
+    /*
+     * We may get here in the context of dump_execstate(), which may have
+     * interrupted context switching between setting "current" and
+     * vmx_do_resume() reaching the end of vmx_load_vmcs(). That would make
+     * all the VMREADs below fail if we don't bail right away.
+     */
+    if ( unlikely(!vmx_vmcs_enter(v)) )
+    {
+        static bool_t warned;
+
+        if ( !warned )
+        {
+            warned = 1;
+            printk(XENLOG_WARNING "Segment register inaccessible for d%dv%d\n"
+                   "(If you see this outside of debugging activity,"
+                   " please report to xen-devel@lists.xenproject.org)\n",
+                   v->domain->domain_id, v->vcpu_id);
+        }
+        memset(reg, 0, sizeof(*reg));
+        return;
+    }
 
     switch ( seg )
     {
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -143,7 +143,7 @@ struct arch_vmx_struct {
 
 int vmx_create_vmcs(struct vcpu *v);
 void vmx_destroy_vmcs(struct vcpu *v);
-void vmx_vmcs_enter(struct vcpu *v);
+bool_t vmx_vmcs_enter(struct vcpu *v);
 void vmx_vmcs_exit(struct vcpu *v);
 
 #define CPU_BASED_VIRTUAL_INTR_PENDING        0x00000004

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel