[PATCH v3] x86: fix a crash in SPEC_CTRL_ENTRY_FROM_INTR_IST

[PATCH v3] x86: fix a crash in SPEC_CTRL_ENTRY_FROM_INTR_IST

[Zhenzhong Duan]

In an IBRS available env, bootup panic when bti=0 like below:

(XEN) Speculative mitigation facilities:
(XEN)   Hardware features: SMEP IBRS/IBPB STIBP
(XEN) BTI mitigations: Thunk N/A, Others: IBRS- SMEP
(XEN) ----[ Xen-4.4.4OVM  x86_64  debug=n  Tainted:    C ]----
(XEN) CPU:    0
(XEN) RIP:    e008:[<ffff82d0802041bb>] entry.o#handle_ist_exception+0xd1/0x176
(XEN) RFLAGS: 0000000000010046   CONTEXT: hypervisor
(XEN) rax: 0000000000000000   rbx: 0000000000000000   rcx: 0000000000000048
(XEN) rdx: 0000000000000001   rsi: 0000000000000000   rdi: 0000000000000000
(XEN) rbp: 0000000000000000   rsp: ffff82d080529f58   r8:  0000000000000000
(XEN) r9:  0000000000000000   r10: 0000000000000000   r11: 0000000000000000
(XEN) r12: 0000000000000000   r13: 0000000000000000   r14: ffff82d08052ffff
(XEN) r15: 0000000000000000   cr0: 000000008005003b   cr4: 00000000001506f0
(XEN) cr3: 0000000076fbe000   cr2: 0000000000000000
(XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: 0000   cs: e008
(XEN) Xen stack trace from rsp=ffff82d080529f58:
(XEN)    0000000000000018 00000000ffffffff 0000000000000002 ffff82d080528000
(XEN)    0000000000000000 ffff82d0802a50e0 ffff82d08052fd98 ffff82d08072fc00
(XEN)    0000000000000000 0000000000010000 0000000000000400 0000000000000830
(XEN)    0000000000000000 000000000000000a ffff82d0803f0fc0 0000000200000000
(XEN)    ffff82d080298876 000000000000e008 0000000000000046 ffff82d08052fdf8
(XEN)    0000000000000000
(XEN) Xen call trace:
(XEN)    [<ffff82d0802041bb>] entry.o#handle_ist_exception+0xd1/0x176
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) GENERAL PROTECTION FAULT
(XEN) [error_code=0000]
(XEN) ****************************************

It's due to %edx isn't cleared to zero before wrmsr.

DO_OVERWRITE_RSB clobbers %eax and happend to cover the bug in certain case so
we didn't reproduce without bti=0. Change to use %edx.

Also drop an unused label per Jan.

Reported-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
 xen/include/asm-x86/spec_ctrl_asm.h |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/xen/include/asm-x86/spec_ctrl_asm.h b/xen/include/asm-x86/spec_ctrl_asm.h
index 814f53d..7b259c4 100644
--- a/xen/include/asm-x86/spec_ctrl_asm.h
+++ b/xen/include/asm-x86/spec_ctrl_asm.h
@@ -269,28 +269,29 @@
  * This is logical merge of DO_OVERWRITE_RSB and DO_SPEC_CTRL_ENTRY
  * maybexen=1, but with conditionals rather than alternatives.
  */
-    movzbl STACK_CPUINFO_FIELD(bti_ist_info)(%r14), %eax
+    movzbl STACK_CPUINFO_FIELD(bti_ist_info)(%r14), %edx
 
-    testb $BTI_IST_RSB, %al
+    testb $BTI_IST_RSB, %dl
     jz .L\@_skip_rsb
 
     DO_OVERWRITE_RSB
 
 .L\@_skip_rsb:
 
-    testb $BTI_IST_WRMSR, %al
+    testb $BTI_IST_WRMSR, %dl
     jz .L\@_skip_wrmsr
 
+    mov %edx, %eax
     xor %edx, %edx
     testb $3, UREGS_cs(%rsp)
     setz %dl
     and %dl, STACK_CPUINFO_FIELD(use_shadow_spec_ctrl)(%r14)
 
-.L\@_entry_from_xen:
     /*
      * Load Xen's intended value.  SPEC_CTRL_IBRS vs 0 is encoded in the
      * bottom bit of bti_ist_info, via a deliberate alias with BTI_IST_IBRS.
      */
+    xor %edx, %edx
     mov $MSR_SPEC_CTRL, %ecx
     and $BTI_IST_IBRS, %eax
     wrmsr
-- 
1.7.3

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3] x86: fix a crash in SPEC_CTRL_ENTRY_FROM_INTR_IST

[Jan Beulich]

>>> On 14.02.18 at 10:25, <zhenzhong.duan@oracle.com> wrote:
> --- a/xen/include/asm-x86/spec_ctrl_asm.h
> +++ b/xen/include/asm-x86/spec_ctrl_asm.h
> @@ -269,28 +269,29 @@
>   * This is logical merge of DO_OVERWRITE_RSB and DO_SPEC_CTRL_ENTRY
>   * maybexen=1, but with conditionals rather than alternatives.
>   */
> -    movzbl STACK_CPUINFO_FIELD(bti_ist_info)(%r14), %eax
> +    movzbl STACK_CPUINFO_FIELD(bti_ist_info)(%r14), %edx
>  
> -    testb $BTI_IST_RSB, %al
> +    testb $BTI_IST_RSB, %dl
>      jz .L\@_skip_rsb
>  
>      DO_OVERWRITE_RSB
>  
>  .L\@_skip_rsb:
>  
> -    testb $BTI_IST_WRMSR, %al
> +    testb $BTI_IST_WRMSR, %dl
>      jz .L\@_skip_wrmsr
>  
> +    mov %edx, %eax
>      xor %edx, %edx
>      testb $3, UREGS_cs(%rsp)
>      setz %dl
>      and %dl, STACK_CPUINFO_FIELD(use_shadow_spec_ctrl)(%r14)
>  
> -.L\@_entry_from_xen:
>      /*
>       * Load Xen's intended value.  SPEC_CTRL_IBRS vs 0 is encoded in the
>       * bottom bit of bti_ist_info, via a deliberate alias with BTI_IST_IBRS.
>       */
> +    xor %edx, %edx
>      mov $MSR_SPEC_CTRL, %ecx
>      and $BTI_IST_IBRS, %eax
>      wrmsr

While indeed you add one less instruction, you don't shrink overall
code size compared to v2. I also prefer v2 because of being more
explicit about the register needing to be preserved across
DO_OVERWRITE_RSB.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3] x86: fix a crash in SPEC_CTRL_ENTRY_FROM_INTR_IST

[Zhenzhong Duan]

在 2018/2/14 17:58, Jan Beulich 写道:
>>>> On 14.02.18 at 10:25, <zhenzhong.duan@oracle.com> wrote:
>> --- a/xen/include/asm-x86/spec_ctrl_asm.h
>> +++ b/xen/include/asm-x86/spec_ctrl_asm.h
>> @@ -269,28 +269,29 @@
>>    * This is logical merge of DO_OVERWRITE_RSB and DO_SPEC_CTRL_ENTRY
>>    * maybexen=1, but with conditionals rather than alternatives.
>>    */
>> -    movzbl STACK_CPUINFO_FIELD(bti_ist_info)(%r14), %eax
>> +    movzbl STACK_CPUINFO_FIELD(bti_ist_info)(%r14), %edx
>>   
>> -    testb $BTI_IST_RSB, %al
>> +    testb $BTI_IST_RSB, %dl
>>       jz .L\@_skip_rsb
>>   
>>       DO_OVERWRITE_RSB
>>   
>>   .L\@_skip_rsb:
>>   
>> -    testb $BTI_IST_WRMSR, %al
>> +    testb $BTI_IST_WRMSR, %dl
>>       jz .L\@_skip_wrmsr
>>   
>> +    mov %edx, %eax
>>       xor %edx, %edx
>>       testb $3, UREGS_cs(%rsp)
>>       setz %dl
>>       and %dl, STACK_CPUINFO_FIELD(use_shadow_spec_ctrl)(%r14)
>>   
>> -.L\@_entry_from_xen:
>>       /*
>>        * Load Xen's intended value.  SPEC_CTRL_IBRS vs 0 is encoded in the
>>        * bottom bit of bti_ist_info, via a deliberate alias with BTI_IST_IBRS.
>>        */
>> +    xor %edx, %edx
>>       mov $MSR_SPEC_CTRL, %ecx
>>       and $BTI_IST_IBRS, %eax
>>       wrmsr
> While indeed you add one less instruction, you don't shrink overall
> code size compared to v2. I also prefer v2 because of being more
> explicit about the register needing to be preserved across
> DO_OVERWRITE_RSB.
Then Ok, in fact my inital thought is to avoid unnecessory mov 
instructions around DO_OVERWRITE_RSB in the 'jmp _skip_wrmsr' case, so 
tried to remove them.

-- 
thanks
zduan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel