Fwd: Xen-4.1.6.1 backport for XSA156

Fwd: Xen-4.1.6.1 backport for XSA156

[Stefan Bader]

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


Hi Jan, hi Andrew,

I am currently trying to backport the changes of XSA156 back to Xen-4.1.x and I
am struggling with the VMX side. I did see the backports made for 4.2 and 3.4 on
the security mailing list but I am not sure the 3.4 backport is not having the
same issues (or similar ones).

Trying to write down my understanding of the changes: For the 3.4 backport there
are only changes to the toggles for debugging and the general trap mask. So if I
understand this right, before the change, TRAP_debug and TRAP_int3 were only
handled in vmexit when a debugger was attached to the domain. Now, only
TRAP_int3 will be toggled and TRAP_debug is always handled.

My testing does (beside other things) involve some verification of ptrace
handling. Which on 4.1.x with the changes, now causes a crash of the HVM guest
in vm_resume (vm_resume_fail error code 7).

I think this is caused by TRAP_debug being handled in vmexit. I don't have the
3.4 code so not sure whether there is anything handling it. In the 4.1.x case
and without changing the vmexit code in xen/arch/x86/hvm/vmx/vmx.c it would be a
certain crash as no domain debugging is done. The problem seems to be that I do
inject (as the 4.2 patch does) an exception. Though 4.1.x does not, yet, have
the changes from "xen: Define new struct hvm_trap and cleanup vmx exception", so
I only have either hvm_inject_exception or vmx_inject_hw_exception. The former
ends up calling the latter. What I think is the problem (which svm does not
have) is the debug/int3 handling in the function below. This seems to convert
the exception unconditionally into a software exception that has an opcode
associated. Would you also think this is the issue? And if yes, is there any
sane way you can think of to prevent this without having to resort to pulling in
large hunks of rewrite?

Regards,
Stefan

vmx_inject_hw_exception(
  ...
    switch ( trap )
    {
    case TRAP_debug:
        type = X86_EVENTTYPE_SW_EXCEPTION;
        if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
        {
            __restore_debug_registers(curr);
            write_debugreg(6, read_debugreg(6) | 0x4000);
        }
        if ( cpu_has_monitor_trap_flag )
            break;
    case TRAP_int3:
        if ( curr->domain->debugger_attached )
        {
            /* Debug/Int3: Trap to debugger. */
            domain_pause_for_debugger();
            return;
        }

        type = X86_EVENTTYPE_SW_EXCEPTION;
        __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 1); /* int3 */
  ...
}






[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 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: Fwd: Xen-4.1.6.1 backport for XSA156

[Jan Beulich]

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

>>> On 20.11.15 at 16:03, <stefan.bader@canonical.com> wrote:
> I am currently trying to backport the changes of XSA156 back to Xen-4.1.x and I
> am struggling with the VMX side. I did see the backports made for 4.2 and 3.4 on
> the security mailing list but I am not sure the 3.4 backport is not having the
> same issues (or similar ones).
> 
> Trying to write down my understanding of the changes: For the 3.4 backport there
> are only changes to the toggles for debugging and the general trap mask. So if I
> understand this right, before the change, TRAP_debug and TRAP_int3 were only
> handled in vmexit when a debugger was attached to the domain. Now, only
> TRAP_int3 will be toggled and TRAP_debug is always handled.

I've never looked at that 3.4 backport, but not changing the VMEXIT
handling certainly sounds wrong. I'll attach what I have done for 4.1.
Please report back any problems you encounter.

Jan


[-- Attachment #2: CVE-2015-5307-xsa156.patch --]
[-- Type: text/plain, Size: 4989 bytes --]

References: bsc#953527 CVE-2015-5307 XSA-156

x86/HVM: always intercept #AC and #DB

Both being benign exceptions, and both being possible to get triggered
by exception delivery, this is required to prevent a guest from locking
up a CPU (resulting from no other VM exits occurring once getting into
such a loop).

The specific scenarios:

1) #AC may be raised during exception delivery if the handler is set to
be a ring-3 one by a 32-bit guest, and the stack is misaligned.

2) #DB may be raised during exception delivery when a breakpoint got
placed on a data structure involved in delivering the exception. This
can result in an endless loop when a 64-bit guest uses a non-zero IST
for the vector 1 IDT entry, but even without use of IST the time it
takes until a contributory fault would get raised (results depending
on the handler) may be quite long.

This is XSA-156.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Tested-by: Andrew Cooper <andrew.cooper3@citrix.com>

--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -764,10 +764,11 @@ static void svm_do_resume(struct vcpu *v
     if ( unlikely(v->arch.hvm_vcpu.debug_state_latch != debug_state) )
     {
         uint32_t intercepts = vmcb_get_exception_intercepts(vmcb);
-        uint32_t mask = (1U << TRAP_debug) | (1U << TRAP_int3);
+
         v->arch.hvm_vcpu.debug_state_latch = debug_state;
         vmcb_set_exception_intercepts(
-            vmcb, debug_state ? (intercepts | mask) : (intercepts & ~mask));
+            vmcb, debug_state ? (intercepts | (1U << TRAP_int3))
+                              : (intercepts & ~(1U << TRAP_int3)));
     }
 
     if ( v->arch.hvm_svm.launch_core != smp_processor_id() )
@@ -1640,8 +1641,9 @@ void svm_vmexit_handler(struct cpu_user_
 
     case VMEXIT_EXCEPTION_DB:
         if ( !v->domain->debugger_attached )
-            goto exit_and_crash;
-        domain_pause_for_debugger();
+            svm_inject_exception(TRAP_debug, HVM_DELIVER_NO_ERROR_CODE, 0);
+        else
+            domain_pause_for_debugger();
         break;
 
     case VMEXIT_EXCEPTION_BP:
@@ -1689,6 +1691,10 @@ void svm_vmexit_handler(struct cpu_user_
         break;
     }
 
+    case VMEXIT_EXCEPTION_AC:
+        svm_inject_exception(TRAP_alignment_check, vmcb->exitinfo1, 0);
+        break;
+
     case VMEXIT_EXCEPTION_UD:
         svm_vmexit_ud_intercept(regs);
         break;
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1129,18 +1129,12 @@ static void vmx_update_host_cr3(struct v
 
 void vmx_update_debug_state(struct vcpu *v)
 {
-    unsigned long mask;
-
     ASSERT(v == current);
 
-    mask = 1u << TRAP_int3;
-    if ( !cpu_has_monitor_trap_flag )
-        mask |= 1u << TRAP_debug;
-
     if ( v->arch.hvm_vcpu.debug_state_latch )
-        v->arch.hvm_vmx.exception_bitmap |= mask;
+        v->arch.hvm_vmx.exception_bitmap |= 1U << TRAP_int3;
     else
-        v->arch.hvm_vmx.exception_bitmap &= ~mask;
+        v->arch.hvm_vmx.exception_bitmap &= ~(1U << TRAP_int3);
     vmx_update_exception_bitmap(v);
 }
 
@@ -2341,9 +2335,10 @@ void vmx_vmexit_handler(struct cpu_user_
              */
             exit_qualification = __vmread(EXIT_QUALIFICATION);
             write_debugreg(6, exit_qualification | 0xffff0ff0);
-            if ( !v->domain->debugger_attached || cpu_has_monitor_trap_flag )
-                goto exit_and_crash;
-            domain_pause_for_debugger();
+            if ( !v->domain->debugger_attached )
+                vmx_inject_hw_exception(vector, HVM_DELIVER_NO_ERROR_CODE);
+            else
+                domain_pause_for_debugger();
             break;
         case TRAP_int3: 
         {
@@ -2397,6 +2392,10 @@ void vmx_vmexit_handler(struct cpu_user_
             v->arch.hvm_vcpu.guest_cr[2] = exit_qualification;
             vmx_inject_hw_exception(TRAP_page_fault, regs->error_code);
             break;
+        case TRAP_alignment_check:
+            vmx_inject_hw_exception(vector,
+                                    __vmread(VM_EXIT_INTR_ERROR_CODE));
+            break;
         case TRAP_nmi:
             if ( (intr_info & INTR_INFO_INTR_TYPE_MASK) !=
                  (X86_EVENTTYPE_NMI << 8) )
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -307,7 +307,10 @@ static inline int hvm_do_pmu_interrupt(s
         (cpu_has_xsave ? X86_CR4_OSXSAVE : 0))))
 
 /* These exceptions must always be intercepted. */
-#define HVM_TRAP_MASK ((1U << TRAP_machine_check) | (1U << TRAP_invalid_op))
+#define HVM_TRAP_MASK ((1U << TRAP_debug)           | \
+                       (1U << TRAP_invalid_op)      | \
+                       (1U << TRAP_alignment_check) | \
+                       (1U << TRAP_machine_check))
 
 /*
  * x86 event types. This enumeration is valid for:

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

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

Re: Fwd: Xen-4.1.6.1 backport for XSA156

[Stefan Bader]

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

On 20.11.2015 16:59, Jan Beulich wrote:
>>>> On 20.11.15 at 16:03, <stefan.bader@canonical.com> wrote:
>> I am currently trying to backport the changes of XSA156 back to Xen-4.1.x and I
>> am struggling with the VMX side. I did see the backports made for 4.2 and 3.4 on
>> the security mailing list but I am not sure the 3.4 backport is not having the
>> same issues (or similar ones).
>>
>> Trying to write down my understanding of the changes: For the 3.4 backport there
>> are only changes to the toggles for debugging and the general trap mask. So if I
>> understand this right, before the change, TRAP_debug and TRAP_int3 were only
>> handled in vmexit when a debugger was attached to the domain. Now, only
>> TRAP_int3 will be toggled and TRAP_debug is always handled.
> 
> I've never looked at that 3.4 backport, but not changing the VMEXIT
> handling certainly sounds wrong. I'll attach what I have done for 4.1.
> Please report back any problems you encounter.

If I am not missing any detail your 4.1 patch looks exactly the same as the
version I ended up with (basically dropping some trace).
Have you tested the resulting HV on an Intel/VMX box and tried to use ptrace
inside the HVM guest?

This is where my problems come from. Or potentially your vmx_inject_hw_exception
has been modified since stable-4.1.6.1?

-Stefan
> 
> Jan
> 



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 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: Fwd: Xen-4.1.6.1 backport for XSA156

[Stefan Bader]

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

On 20.11.2015 17:10, Stefan Bader wrote:
> On 20.11.2015 16:59, Jan Beulich wrote:
>>>>> On 20.11.15 at 16:03, <stefan.bader@canonical.com> wrote:
>>> I am currently trying to backport the changes of XSA156 back to Xen-4.1.x and I
>>> am struggling with the VMX side. I did see the backports made for 4.2 and 3.4 on
>>> the security mailing list but I am not sure the 3.4 backport is not having the
>>> same issues (or similar ones).
>>>
>>> Trying to write down my understanding of the changes: For the 3.4 backport there
>>> are only changes to the toggles for debugging and the general trap mask. So if I
>>> understand this right, before the change, TRAP_debug and TRAP_int3 were only
>>> handled in vmexit when a debugger was attached to the domain. Now, only
>>> TRAP_int3 will be toggled and TRAP_debug is always handled.
>>
>> I've never looked at that 3.4 backport, but not changing the VMEXIT
>> handling certainly sounds wrong. I'll attach what I have done for 4.1.
>> Please report back any problems you encounter.
> 
> If I am not missing any detail your 4.1 patch looks exactly the same as the
> version I ended up with (basically dropping some trace).
> Have you tested the resulting HV on an Intel/VMX box and tried to use ptrace
> inside the HVM guest?
> 
> This is where my problems come from. Or potentially your vmx_inject_hw_exception
> has been modified since stable-4.1.6.1?

So this is a quick hack I just tried and that keeps the HVM alive:

@@ -1294,7 +1288,6 @@ void vmx_inject_hw_exception(int trap, i
     switch ( trap )
     {
     case TRAP_debug:
-        type = X86_EVENTTYPE_SW_EXCEPTION;
         if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
         {
             __restore_debug_registers(curr);
@@ -1302,6 +1295,13 @@ void vmx_inject_hw_exception(int trap, i
         }
         if ( cpu_has_monitor_trap_flag )
             break;
+        if ( curr->domain->debugger_attached )
+        {
+            /* Debug/Int3: Trap to debugger. */
+            domain_pause_for_debugger();
+            return;
+        }
+        break;
     case TRAP_int3:
         if ( curr->domain->debugger_attached )
         {

Though this looks like an ugly hack and probably is wrong in the other case of
TRAP_debug caused by an opcode...

-Stefan

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



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 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: Fwd: Xen-4.1.6.1 backport for XSA156

[Jan Beulich]

>>> On 20.11.15 at 17:15, <stefan.bader@canonical.com> wrote:
> So this is a quick hack I just tried and that keeps the HVM alive:
> 
> @@ -1294,7 +1288,6 @@ void vmx_inject_hw_exception(int trap, i
>      switch ( trap )
>      {
>      case TRAP_debug:
> -        type = X86_EVENTTYPE_SW_EXCEPTION;
>          if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
>          {
>              __restore_debug_registers(curr);
> @@ -1302,6 +1295,13 @@ void vmx_inject_hw_exception(int trap, i
>          }
>          if ( cpu_has_monitor_trap_flag )
>              break;
> +        if ( curr->domain->debugger_attached )
> +        {
> +            /* Debug/Int3: Trap to debugger. */
> +            domain_pause_for_debugger();
> +            return;
> +        }
> +        break;
>      case TRAP_int3:
>          if ( curr->domain->debugger_attached )
>          {
> 
> Though this looks like an ugly hack and probably is wrong in the other case of
> TRAP_debug caused by an opcode...

Right, and I'm afraid this case doesn't get handled correctly even on
-unstable now. But apart from that aspect I think the change above
is okay.

Jan

Re: Fwd: Xen-4.1.6.1 backport for XSA156

[Stefan Bader]

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

On 20.11.2015 17:54, Jan Beulich wrote:
>>>> On 20.11.15 at 17:15, <stefan.bader@canonical.com> wrote:
>> So this is a quick hack I just tried and that keeps the HVM alive:
>>
>> @@ -1294,7 +1288,6 @@ void vmx_inject_hw_exception(int trap, i
>>      switch ( trap )
>>      {
>>      case TRAP_debug:
>> -        type = X86_EVENTTYPE_SW_EXCEPTION;
>>          if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
>>          {
>>              __restore_debug_registers(curr);
>> @@ -1302,6 +1295,13 @@ void vmx_inject_hw_exception(int trap, i
>>          }
>>          if ( cpu_has_monitor_trap_flag )
>>              break;
>> +        if ( curr->domain->debugger_attached )
>> +        {
>> +            /* Debug/Int3: Trap to debugger. */
>> +            domain_pause_for_debugger();
>> +            return;
>> +        }
>> +        break;
>>      case TRAP_int3:
>>          if ( curr->domain->debugger_attached )
>>          {
>>
>> Though this looks like an ugly hack and probably is wrong in the other case of
>> TRAP_debug caused by an opcode...
> 
> Right, and I'm afraid this case doesn't get handled correctly even on
> -unstable now. But apart from that aspect I think the change above
> is okay.

Oh, ok. Thanks for the review. I guess then I go with that. At least this does
no longer crash.

-Stefan

> 
> Jan
> 



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 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: Fwd: Xen-4.1.6.1 backport for XSA156

[Jan Beulich]

>>> On 20.11.15 at 18:07, <stefan.bader@canonical.com> wrote:
> On 20.11.2015 17:54, Jan Beulich wrote:
>>>>> On 20.11.15 at 17:15, <stefan.bader@canonical.com> wrote:
>>> So this is a quick hack I just tried and that keeps the HVM alive:
>>>
>>> @@ -1294,7 +1288,6 @@ void vmx_inject_hw_exception(int trap, i
>>>      switch ( trap )
>>>      {
>>>      case TRAP_debug:
>>> -        type = X86_EVENTTYPE_SW_EXCEPTION;
>>>          if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
>>>          {
>>>              __restore_debug_registers(curr);
>>> @@ -1302,6 +1295,13 @@ void vmx_inject_hw_exception(int trap, i
>>>          }
>>>          if ( cpu_has_monitor_trap_flag )
>>>              break;
>>> +        if ( curr->domain->debugger_attached )
>>> +        {
>>> +            /* Debug/Int3: Trap to debugger. */
>>> +            domain_pause_for_debugger();
>>> +            return;
>>> +        }
>>> +        break;
>>>      case TRAP_int3:
>>>          if ( curr->domain->debugger_attached )
>>>          {
>>>
>>> Though this looks like an ugly hack and probably is wrong in the other case of
>>> TRAP_debug caused by an opcode...
>> 
>> Right, and I'm afraid this case doesn't get handled correctly even on
>> -unstable now. But apart from that aspect I think the change above
>> is okay.
> 
> Oh, ok. Thanks for the review. I guess then I go with that. At least this 
> does no longer crash.

Actually there's no problem with ICEBP - just like INTnn it isn't itself
interceptable (and the injection of vector 0x01 from the x86
emulator path can't fully distinguish between ICEBP and INT $1  in
these old versions anyway). So what you have should be good
enough, albeit I think I'll code it slightly differently (keeping the fall-
through in place).

Jan

Re: Fwd: Xen-4.1.6.1 backport for XSA156

[Jan Beulich]

>>> On 23.11.15 at 08:37, <JBeulich@suse.com> wrote:
> Actually there's no problem with ICEBP - just like INTnn it isn't itself
> interceptable (and the injection of vector 0x01 from the x86
> emulator path can't fully distinguish between ICEBP and INT $1  in
> these old versions anyway). So what you have should be good
> enough, albeit I think I'll code it slightly differently (keeping the fall-
> through in place).

Like this:

@@ -1364,7 +1358,6 @@ void vmx_inject_hw_exception(int trap, i
     switch ( trap )
     {
     case TRAP_debug:
-        type = X86_EVENTTYPE_SW_EXCEPTION;
         if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
         {
             __restore_debug_registers(curr);
@@ -1379,9 +1372,11 @@ void vmx_inject_hw_exception(int trap, i
             domain_pause_for_debugger();
             return;
         }
-
-        type = X86_EVENTTYPE_SW_EXCEPTION;
-        __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 1); /* int3 */
+        if ( trap == TRAP_int3 )
+        {
+            type = X86_EVENTTYPE_SW_EXCEPTION;
+            __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 1);
+        }
     }
 
     if ( unlikely(intr_info & INTR_INFO_VALID_MASK) &&

Jan

Re: Fwd: Xen-4.1.6.1 backport for XSA156

[Stefan Bader]

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

On 23.11.2015 08:51, Jan Beulich wrote:
>>>> On 23.11.15 at 08:37, <JBeulich@suse.com> wrote:
>> Actually there's no problem with ICEBP - just like INTnn it isn't itself
>> interceptable (and the injection of vector 0x01 from the x86
>> emulator path can't fully distinguish between ICEBP and INT $1  in
>> these old versions anyway). So what you have should be good
>> enough, albeit I think I'll code it slightly differently (keeping the fall-
>> through in place).
> 
> Like this:
> 
> @@ -1364,7 +1358,6 @@ void vmx_inject_hw_exception(int trap, i
>      switch ( trap )
>      {
>      case TRAP_debug:
> -        type = X86_EVENTTYPE_SW_EXCEPTION;
>          if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
>          {
>              __restore_debug_registers(curr);
> @@ -1379,9 +1372,11 @@ void vmx_inject_hw_exception(int trap, i
>              domain_pause_for_debugger();
>              return;
>          }
> -
> -        type = X86_EVENTTYPE_SW_EXCEPTION;
> -        __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 1); /* int3 */
> +        if ( trap == TRAP_int3 )
> +        {
> +            type = X86_EVENTTYPE_SW_EXCEPTION;
> +            __vmwrite(VM_ENTRY_INSTRUCTION_LEN, 1);
> +        }
>      }
>  
>      if ( unlikely(intr_info & INTR_INFO_VALID_MASK) &&
> 

Yeah, for my side I think I stick with what I had because I already have now run
that variant through testing. But I will include both variants when talking to
the Debian guys.

-Stefan
> Jan
> 



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

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