[PATCH 7/8] x86/spec-ctrl: Issue VERW during IST exit to Xen

Andrew Cooper posted 8 patches 5 months, 1 week ago
There is a newer version of this series
[PATCH 7/8] x86/spec-ctrl: Issue VERW during IST exit to Xen
Posted by Andrew Cooper 5 months, 1 week ago
There is a corner case where e.g. an NMI hitting an exit-to-guest path after
SPEC_CTRL_EXIT_TO_* would have run the entire NMI handler *after* the VERW
flush to scrub potentially sensitive data from uarch buffers.

In order to compensate, issue VERW when exiting to Xen from an IST entry.

SPEC_CTRL_EXIT_TO_XEN already has two reads of spec_ctrl_flags off the stack,
and we're about to add a third.  Load the field into %ebx, and list the
register as clobbered.

%r12 has been arranged to be the ist_exit signal, so add this as an input
dependency and use it to identify when to issue a VERW.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/include/asm/spec_ctrl_asm.h | 20 +++++++++++++++-----
 xen/arch/x86/x86_64/entry.S              |  2 +-
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/include/asm/spec_ctrl_asm.h b/xen/arch/x86/include/asm/spec_ctrl_asm.h
index acdb526d292d..9740697114ad 100644
--- a/xen/arch/x86/include/asm/spec_ctrl_asm.h
+++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h
@@ -344,10 +344,12 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise):
  */
 .macro SPEC_CTRL_EXIT_TO_XEN
 /*
- * Requires %r14=stack_end
- * Clobbers %rax, %rcx, %rdx
+ * Requires %r12=ist_exit, %r14=stack_end
+ * Clobbers %rax, %rbx, %rcx, %rdx
  */
-    testb $SCF_ist_sc_msr, STACK_CPUINFO_FIELD(spec_ctrl_flags)(%r14)
+    movzbl STACK_CPUINFO_FIELD(spec_ctrl_flags)(%r14), %ebx
+
+    testb $SCF_ist_sc_msr, %bl
     jz .L\@_skip_sc_msr
 
     /*
@@ -358,7 +360,7 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise):
      */
     xor %edx, %edx
 
-    testb $SCF_use_shadow, STACK_CPUINFO_FIELD(spec_ctrl_flags)(%r14)
+    testb $SCF_use_shadow, %bl
     jz .L\@_skip_sc_msr
 
     mov STACK_CPUINFO_FIELD(shadow_spec_ctrl)(%r14), %eax
@@ -367,8 +369,16 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise):
 
 .L\@_skip_sc_msr:
 
-    /* TODO VERW */
+    test %r12, %r12
+    jz .L\@_skip_ist_exit
+
+    /* Logically DO_SPEC_CTRL_COND_VERW but without the %rsp=cpuinfo dependency */
+    testb $SCF_verw, %bl
+    jz .L\@_verw_skip
+    verw STACK_CPUINFO_FIELD(verw_sel)(%r14)
+.L\@_verw_skip:
 
+.L\@_skip_ist_exit:
 .endm
 
 #endif /* __ASSEMBLY__ */
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index da084a7e8e54..f70752fa36c1 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -686,7 +686,7 @@ UNLIKELY_START(ne, exit_cr3)
 UNLIKELY_END(exit_cr3)
 
         /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
-        SPEC_CTRL_EXIT_TO_XEN     /* Req: %r14=end, Clob: acd */
+        SPEC_CTRL_EXIT_TO_XEN     /* Req: %r12=ist_exit %r14=end, Clob: abcd */
 
         RESTORE_ALL adj=8
         iretq
-- 
2.30.2


Re: [PATCH 7/8] x86/spec-ctrl: Issue VERW during IST exit to Xen
Posted by Jan Beulich 5 months, 1 week ago
On 13.09.2023 22:27, Andrew Cooper wrote:
> There is a corner case where e.g. an NMI hitting an exit-to-guest path after
> SPEC_CTRL_EXIT_TO_* would have run the entire NMI handler *after* the VERW
> flush to scrub potentially sensitive data from uarch buffers.
> 
> In order to compensate, issue VERW when exiting to Xen from an IST entry.
> 
> SPEC_CTRL_EXIT_TO_XEN already has two reads of spec_ctrl_flags off the stack,
> and we're about to add a third.  Load the field into %ebx, and list the
> register as clobbered.
> 
> %r12 has been arranged to be the ist_exit signal, so add this as an input
> dependency and use it to identify when to issue a VERW.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

While looking technically okay, I still have a couple of remarks:

> --- a/xen/arch/x86/include/asm/spec_ctrl_asm.h
> +++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h
> @@ -344,10 +344,12 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise):
>   */
>  .macro SPEC_CTRL_EXIT_TO_XEN
>  /*
> - * Requires %r14=stack_end
> - * Clobbers %rax, %rcx, %rdx
> + * Requires %r12=ist_exit, %r14=stack_end
> + * Clobbers %rax, %rbx, %rcx, %rdx

While I'd generally be a little concerned of the growing dependency and
clobber lists, there being a single use site makes this pretty okay. The
macro invocation being immediately followed by RESTORE_ALL effectively
means we can clobber almost everything here.

As to register usage and my comment on patch 5: There's no real need
to switch %rbx to %r14 there if I'm not mistaken; in particular you
don't re-use any of the other macros which require use of %r14. You
could as well use %r14b (or about any other register that isn't already
used for something, yet whichever was picked apparently wouldn't make a
difference) for the flags here, getting away with fewer new REX prefixes
overall.

>   */
> -    testb $SCF_ist_sc_msr, STACK_CPUINFO_FIELD(spec_ctrl_flags)(%r14)
> +    movzbl STACK_CPUINFO_FIELD(spec_ctrl_flags)(%r14), %ebx
> +
> +    testb $SCF_ist_sc_msr, %bl
>      jz .L\@_skip_sc_msr
>  
>      /*
> @@ -358,7 +360,7 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise):
>       */
>      xor %edx, %edx
>  
> -    testb $SCF_use_shadow, STACK_CPUINFO_FIELD(spec_ctrl_flags)(%r14)
> +    testb $SCF_use_shadow, %bl
>      jz .L\@_skip_sc_msr
>  
>      mov STACK_CPUINFO_FIELD(shadow_spec_ctrl)(%r14), %eax
> @@ -367,8 +369,16 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise):
>  
>  .L\@_skip_sc_msr:
>  
> -    /* TODO VERW */
> +    test %r12, %r12
> +    jz .L\@_skip_ist_exit
> +
> +    /* Logically DO_SPEC_CTRL_COND_VERW but without the %rsp=cpuinfo dependency */
> +    testb $SCF_verw, %bl
> +    jz .L\@_verw_skip
> +    verw STACK_CPUINFO_FIELD(verw_sel)(%r14)
> +.L\@_verw_skip:

Nit: .L\@_skip_verw would be more consistent with the other label names.

> +.L\@_skip_ist_exit:

I was going to ask why the separate label (and whether the two JZ above
couldn't sensibly be folded), but the to both answer lies in the next
patch.

Jan
Re: [PATCH 7/8] x86/spec-ctrl: Issue VERW during IST exit to Xen
Posted by Andrew Cooper 5 months, 1 week ago
On 14/09/2023 11:01 am, Jan Beulich wrote:
> On 13.09.2023 22:27, Andrew Cooper wrote:
>> There is a corner case where e.g. an NMI hitting an exit-to-guest path after
>> SPEC_CTRL_EXIT_TO_* would have run the entire NMI handler *after* the VERW
>> flush to scrub potentially sensitive data from uarch buffers.
>>
>> In order to compensate, issue VERW when exiting to Xen from an IST entry.
>>
>> SPEC_CTRL_EXIT_TO_XEN already has two reads of spec_ctrl_flags off the stack,
>> and we're about to add a third.  Load the field into %ebx, and list the
>> register as clobbered.
>>
>> %r12 has been arranged to be the ist_exit signal, so add this as an input
>> dependency and use it to identify when to issue a VERW.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> While looking technically okay, I still have a couple of remarks:
>
>> --- a/xen/arch/x86/include/asm/spec_ctrl_asm.h
>> +++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h
>> @@ -344,10 +344,12 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise):
>>   */
>>  .macro SPEC_CTRL_EXIT_TO_XEN
>>  /*
>> - * Requires %r14=stack_end
>> - * Clobbers %rax, %rcx, %rdx
>> + * Requires %r12=ist_exit, %r14=stack_end
>> + * Clobbers %rax, %rbx, %rcx, %rdx
> While I'd generally be a little concerned of the growing dependency and
> clobber lists, there being a single use site makes this pretty okay. The
> macro invocation being immediately followed by RESTORE_ALL effectively
> means we can clobber almost everything here.
>
> As to register usage and my comment on patch 5: There's no real need
> to switch %rbx to %r14 there if I'm not mistaken

As said, it's about consistency of the helpers.


>>   */
>> -    testb $SCF_ist_sc_msr, STACK_CPUINFO_FIELD(spec_ctrl_flags)(%r14)
>> +    movzbl STACK_CPUINFO_FIELD(spec_ctrl_flags)(%r14), %ebx
>> +
>> +    testb $SCF_ist_sc_msr, %bl
>>      jz .L\@_skip_sc_msr
>>  
>>      /*
>> @@ -358,7 +360,7 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise):
>>       */
>>      xor %edx, %edx
>>  
>> -    testb $SCF_use_shadow, STACK_CPUINFO_FIELD(spec_ctrl_flags)(%r14)
>> +    testb $SCF_use_shadow, %bl
>>      jz .L\@_skip_sc_msr
>>  
>>      mov STACK_CPUINFO_FIELD(shadow_spec_ctrl)(%r14), %eax
>> @@ -367,8 +369,16 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise):
>>  
>>  .L\@_skip_sc_msr:
>>  
>> -    /* TODO VERW */
>> +    test %r12, %r12
>> +    jz .L\@_skip_ist_exit
>> +
>> +    /* Logically DO_SPEC_CTRL_COND_VERW but without the %rsp=cpuinfo dependency */
>> +    testb $SCF_verw, %bl
>> +    jz .L\@_verw_skip
>> +    verw STACK_CPUINFO_FIELD(verw_sel)(%r14)
>> +.L\@_verw_skip:
> Nit: .L\@_skip_verw would be more consistent with the other label names.

So it would.  I'll tweak.

~Andrew

Re: [PATCH 7/8] x86/spec-ctrl: Issue VERW during IST exit to Xen
Posted by Jan Beulich 5 months, 1 week ago
On 14.09.2023 21:49, Andrew Cooper wrote:
> On 14/09/2023 11:01 am, Jan Beulich wrote:
>> On 13.09.2023 22:27, Andrew Cooper wrote:
>>> There is a corner case where e.g. an NMI hitting an exit-to-guest path after
>>> SPEC_CTRL_EXIT_TO_* would have run the entire NMI handler *after* the VERW
>>> flush to scrub potentially sensitive data from uarch buffers.
>>>
>>> In order to compensate, issue VERW when exiting to Xen from an IST entry.
>>>
>>> SPEC_CTRL_EXIT_TO_XEN already has two reads of spec_ctrl_flags off the stack,
>>> and we're about to add a third.  Load the field into %ebx, and list the
>>> register as clobbered.
>>>
>>> %r12 has been arranged to be the ist_exit signal, so add this as an input
>>> dependency and use it to identify when to issue a VERW.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> While looking technically okay, I still have a couple of remarks:
>>
>>> --- a/xen/arch/x86/include/asm/spec_ctrl_asm.h
>>> +++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h
>>> @@ -344,10 +344,12 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise):
>>>   */
>>>  .macro SPEC_CTRL_EXIT_TO_XEN
>>>  /*
>>> - * Requires %r14=stack_end
>>> - * Clobbers %rax, %rcx, %rdx
>>> + * Requires %r12=ist_exit, %r14=stack_end
>>> + * Clobbers %rax, %rbx, %rcx, %rdx
>> While I'd generally be a little concerned of the growing dependency and
>> clobber lists, there being a single use site makes this pretty okay. The
>> macro invocation being immediately followed by RESTORE_ALL effectively
>> means we can clobber almost everything here.
>>
>> As to register usage and my comment on patch 5: There's no real need
>> to switch %rbx to %r14 there if I'm not mistaken
> 
> As said, it's about consistency of the helpers.

While I'm not entirely happy with this, ...

>>> @@ -367,8 +369,16 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise):
>>>  
>>>  .L\@_skip_sc_msr:
>>>  
>>> -    /* TODO VERW */
>>> +    test %r12, %r12
>>> +    jz .L\@_skip_ist_exit
>>> +
>>> +    /* Logically DO_SPEC_CTRL_COND_VERW but without the %rsp=cpuinfo dependency */
>>> +    testb $SCF_verw, %bl
>>> +    jz .L\@_verw_skip
>>> +    verw STACK_CPUINFO_FIELD(verw_sel)(%r14)
>>> +.L\@_verw_skip:
>> Nit: .L\@_skip_verw would be more consistent with the other label names.
> 
> So it would.  I'll tweak.

... then
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan