thr3ads.net - Xen devel - [PATCH] VMX: don''t crash processing ''d'' debug key [Nov 2013]

If this information is useful, please help other people find it:
Share via:

Jan Beulich

2013-Nov-07 10:44 UTC

[PATCH] 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





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

Andrew Cooper

2013-Nov-07 14:34 UTC

head link

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

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


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

Jan Beulich

2013-Nov-07 14:49 UTC

head link

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

>>> 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

Andrew Cooper

2013-Nov-07 15:35 UTC

head link

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

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>

Tim Deegan

2013-Nov-07 19:08 UTC

head link

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

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.

Jan Beulich

2013-Nov-08 16:04 UTC

head link

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

>>> 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

Jan Beulich

2013-Nov-08 16:07 UTC

head link

[PATCH v2] 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





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

Andrew Cooper

2013-Nov-08 16:09 UTC

head link

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

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

Andrew Cooper

2013-Nov-08 16:11 UTC

head link

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

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

Jan Beulich

2013-Nov-08 16:15 UTC

head link

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

>>> 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

Tim Deegan

2013-Nov-08 16:55 UTC

head link

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

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.

Jan Beulich

2013-Nov-11 12:55 UTC

head link

[PATCH v3] 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





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

Keir Fraser

2013-Nov-11 13:13 UTC

head link

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

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>

Xen devel - Nov 2013 - [PATCH] VMX: don't crash processing 'd' debug key

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

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

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

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

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

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

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

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

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

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

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

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

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