tl;dr: The existing mitigation for eIBRS PBRSB predictions uses an INT3 to
ensure a call instruction retires before a following unbalanced RET. Replace
this with a WRMSR serialising instruction which has a lower performance
penalty.
== Background ==
eIBRS (enhanced indirect branch restricted speculation) is used to prevent
predictor addresses from one privilege domain from being used for prediction
in a higher privilege domain.
== Problem ==
On processors with eIBRS protections there can be a case where upon VM exit
a guest address may be used as an RSB prediction for an unbalanced RET if a
CALL instruction hasn't yet been retired. This is termed PBRSB (Post-Barrier
Return Stack Buffer).
A mitigation for this was introduced in:
(2b1299322016731d56807aa49254a5ea3080b6b3 x86/speculation: Add RSB VM Exit protections)
This mitigation [1] has a ~1% performance impact on VM exit compared to without
it [2].
== Solution ==
The WRMSR instruction can be used as a speculation barrier and a serialising
instruction. Use this on the VM exit path instead to ensure that a CALL
instruction (in this case the call to vmx_spec_ctrl_restore_host) has retired
before the prediction of a following unbalanced RET.
This mitigation [3] has a negligible performance impact.
== Testing ==
Run the outl_to_kernel kvm-unit-tests test 200 times per configuration which
counts the cycles for an exit to kernel mode.
[1] With existing mitigation:
Average: 2026 cycles
[2] With no mitigation:
Average: 2008 cycles
[3] With proposed mitigation:
Average: 2008 cycles
Signed-off-by: Suraj Jitindar Singh <[email protected]>
Cc: [email protected]
---
arch/x86/include/asm/nospec-branch.h | 7 +++----
arch/x86/kvm/vmx/vmenter.S | 3 +--
arch/x86/kvm/vmx/vmx.c | 5 +++++
3 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index c936ce9f0c47..e5723e024b47 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -159,10 +159,9 @@
* A simpler FILL_RETURN_BUFFER macro. Don't make people use the CPP
* monstrosity above, manually.
*/
-.macro FILL_RETURN_BUFFER reg:req nr:req ftr:req ftr2=ALT_NOT(X86_FEATURE_ALWAYS)
- ALTERNATIVE_2 "jmp .Lskip_rsb_\@", \
- __stringify(__FILL_RETURN_BUFFER(\reg,\nr)), \ftr, \
- __stringify(__FILL_ONE_RETURN), \ftr2
+.macro FILL_RETURN_BUFFER reg:req nr:req ftr:req
+ ALTERNATIVE "jmp .Lskip_rsb_\@", \
+ __stringify(__FILL_RETURN_BUFFER(\reg,\nr)), \ftr
.Lskip_rsb_\@:
.endm
diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
index 6de96b943804..eb82797bd7bf 100644
--- a/arch/x86/kvm/vmx/vmenter.S
+++ b/arch/x86/kvm/vmx/vmenter.S
@@ -231,8 +231,7 @@ SYM_INNER_LABEL(vmx_vmexit, SYM_L_GLOBAL)
* single call to retire, before the first unbalanced RET.
*/
- FILL_RETURN_BUFFER %_ASM_CX, RSB_CLEAR_LOOPS, X86_FEATURE_RSB_VMEXIT,\
- X86_FEATURE_RSB_VMEXIT_LITE
+ FILL_RETURN_BUFFER %_ASM_CX, RSB_CLEAR_LOOPS, X86_FEATURE_RSB_VMEXIT
pop %_ASM_ARG2 /* @flags */
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index c9b49a09e6b5..fdcd8e10c2ab 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7049,8 +7049,13 @@ void noinstr vmx_spec_ctrl_restore_host(struct vcpu_vmx *vmx,
* For legacy IBRS, the IBRS bit always needs to be written after
* transitioning from a less privileged predictor mode, regardless of
* whether the guest/host values differ.
+ *
+ * For eIBRS affected by Post Barrier RSB Predictions a serialising
+ * instruction (wrmsr) must be executed to ensure a call instruction has
+ * retired before the prediction of a following unbalanced ret.
*/
if (cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS) ||
+ cpu_feature_enabled(X86_FEATURE_RSB_VMEXIT_LITE) ||
vmx->spec_ctrl != hostval)
native_wrmsrl(MSR_IA32_SPEC_CTRL, hostval);
--
2.17.1
On Wed, Oct 5, 2022 at 3:03 PM Suraj Jitindar Singh <[email protected]> wrote:
>
> tl;dr: The existing mitigation for eIBRS PBRSB predictions uses an INT3 to
> ensure a call instruction retires before a following unbalanced RET. Replace
> this with a WRMSR serialising instruction which has a lower performance
> penalty.
The INT3 is only on a speculative path and should not impact performance.
> == Background ==
>
> eIBRS (enhanced indirect branch restricted speculation) is used to prevent
> predictor addresses from one privilege domain from being used for prediction
> in a higher privilege domain.
>
> == Problem ==
>
> On processors with eIBRS protections there can be a case where upon VM exit
> a guest address may be used as an RSB prediction for an unbalanced RET if a
> CALL instruction hasn't yet been retired. This is termed PBRSB (Post-Barrier
> Return Stack Buffer).
>
> A mitigation for this was introduced in:
> (2b1299322016731d56807aa49254a5ea3080b6b3 x86/speculation: Add RSB VM Exit protections)
>
> This mitigation [1] has a ~1% performance impact on VM exit compared to without
> it [2].
>
> == Solution ==
>
> The WRMSR instruction can be used as a speculation barrier and a serialising
> instruction. Use this on the VM exit path instead to ensure that a CALL
> instruction (in this case the call to vmx_spec_ctrl_restore_host) has retired
> before the prediction of a following unbalanced RET.
I don't buy this solution. According to
https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/advisory-guidance/post-barrier-return-stack-buffer-predictions.html:
"Note that a WRMSR instruction (used to set IBRS, for example), could
also serve as a speculation barrier for such a sequence in place of
LFENCE."
This says only that you can replace the LFENCE with a WRMSR. It
doesn't say that you can drop the rest of the sequence.
> This mitigation [3] has a negligible performance impact.
>
> == Testing ==
>
> Run the outl_to_kernel kvm-unit-tests test 200 times per configuration which
> counts the cycles for an exit to kernel mode.
>
> [1] With existing mitigation:
> Average: 2026 cycles
> [2] With no mitigation:
> Average: 2008 cycles
> [3] With proposed mitigation:
> Average: 2008 cycles
What testing did you do to see that this is an effective mitigation?
Improved timings are irrelevant if it doesn't work.
On Wed, Oct 5, 2022 at 3:03 PM Suraj Jitindar Singh <[email protected]> wrote:
>
> tl;dr: The existing mitigation for eIBRS PBRSB predictions uses an INT3 to
> ensure a call instruction retires before a following unbalanced RET. Replace
> this with a WRMSR serialising instruction which has a lower performance
> penalty.
>
> == Background ==
>
> eIBRS (enhanced indirect branch restricted speculation) is used to prevent
> predictor addresses from one privilege domain from being used for prediction
> in a higher privilege domain.
>
> == Problem ==
>
> On processors with eIBRS protections there can be a case where upon VM exit
> a guest address may be used as an RSB prediction for an unbalanced RET if a
> CALL instruction hasn't yet been retired. This is termed PBRSB (Post-Barrier
> Return Stack Buffer).
>
> A mitigation for this was introduced in:
> (2b1299322016731d56807aa49254a5ea3080b6b3 x86/speculation: Add RSB VM Exit protections)
>
> This mitigation [1] has a ~1% performance impact on VM exit compared to without
> it [2].
>
> == Solution ==
>
> The WRMSR instruction can be used as a speculation barrier and a serialising
> instruction. Use this on the VM exit path instead to ensure that a CALL
> instruction (in this case the call to vmx_spec_ctrl_restore_host) has retired
> before the prediction of a following unbalanced RET.
>
> This mitigation [3] has a negligible performance impact.
>
> == Testing ==
>
> Run the outl_to_kernel kvm-unit-tests test 200 times per configuration which
> counts the cycles for an exit to kernel mode.
>
> [1] With existing mitigation:
> Average: 2026 cycles
> [2] With no mitigation:
> Average: 2008 cycles
> [3] With proposed mitigation:
> Average: 2008 cycles
>
> Signed-off-by: Suraj Jitindar Singh <[email protected]>
> Cc: [email protected]
> ---
> arch/x86/include/asm/nospec-branch.h | 7 +++----
> arch/x86/kvm/vmx/vmenter.S | 3 +--
> arch/x86/kvm/vmx/vmx.c | 5 +++++
> 3 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
> index c936ce9f0c47..e5723e024b47 100644
> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h
> @@ -159,10 +159,9 @@
> * A simpler FILL_RETURN_BUFFER macro. Don't make people use the CPP
> * monstrosity above, manually.
> */
> -.macro FILL_RETURN_BUFFER reg:req nr:req ftr:req ftr2=ALT_NOT(X86_FEATURE_ALWAYS)
> - ALTERNATIVE_2 "jmp .Lskip_rsb_\@", \
> - __stringify(__FILL_RETURN_BUFFER(\reg,\nr)), \ftr, \
> - __stringify(__FILL_ONE_RETURN), \ftr2
> +.macro FILL_RETURN_BUFFER reg:req nr:req ftr:req
> + ALTERNATIVE "jmp .Lskip_rsb_\@", \
> + __stringify(__FILL_RETURN_BUFFER(\reg,\nr)), \ftr
>
> .Lskip_rsb_\@:
> .endm
> diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
> index 6de96b943804..eb82797bd7bf 100644
> --- a/arch/x86/kvm/vmx/vmenter.S
> +++ b/arch/x86/kvm/vmx/vmenter.S
> @@ -231,8 +231,7 @@ SYM_INNER_LABEL(vmx_vmexit, SYM_L_GLOBAL)
> * single call to retire, before the first unbalanced RET.
> */
>
> - FILL_RETURN_BUFFER %_ASM_CX, RSB_CLEAR_LOOPS, X86_FEATURE_RSB_VMEXIT,\
> - X86_FEATURE_RSB_VMEXIT_LITE
> + FILL_RETURN_BUFFER %_ASM_CX, RSB_CLEAR_LOOPS, X86_FEATURE_RSB_VMEXIT
>
>
> pop %_ASM_ARG2 /* @flags */
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index c9b49a09e6b5..fdcd8e10c2ab 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7049,8 +7049,13 @@ void noinstr vmx_spec_ctrl_restore_host(struct vcpu_vmx *vmx,
> * For legacy IBRS, the IBRS bit always needs to be written after
> * transitioning from a less privileged predictor mode, regardless of
> * whether the guest/host values differ.
> + *
> + * For eIBRS affected by Post Barrier RSB Predictions a serialising
> + * instruction (wrmsr) must be executed to ensure a call instruction has
> + * retired before the prediction of a following unbalanced ret.
> */
> if (cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS) ||
> + cpu_feature_enabled(X86_FEATURE_RSB_VMEXIT_LITE) ||
> vmx->spec_ctrl != hostval)
> native_wrmsrl(MSR_IA32_SPEC_CTRL, hostval);
Okay. I see how this almost meets the requirements. But this WRMSR is
conditional, which means that there's a speculative path through this
code that ends up at the unbalanced RET without executing the WRMSR.
Also, for your timings of "no mitigation" and this proposed mitigation
to be the same, I assume that the guest in your timing test has a
different IA32_SPEC_CTRL value than the host, which isn't always going
to be the case in practice. How much does this WRMSR cost if the guest
and the host have the same IA32_SPEC_CTRL value?
> --
> 2.17.1
>
On Wed, Oct 05, 2022 at 04:24:54PM -0700, Jim Mattson wrote:
>On Wed, Oct 5, 2022 at 3:03 PM Suraj Jitindar Singh <[email protected]> wrote:
>>
>> tl;dr: The existing mitigation for eIBRS PBRSB predictions uses an INT3 to
>> ensure a call instruction retires before a following unbalanced RET. Replace
>> this with a WRMSR serialising instruction which has a lower performance
>> penalty.
>>
>> == Background ==
>>
>> eIBRS (enhanced indirect branch restricted speculation) is used to prevent
>> predictor addresses from one privilege domain from being used for prediction
>> in a higher privilege domain.
>>
>> == Problem ==
>>
>> On processors with eIBRS protections there can be a case where upon VM exit
>> a guest address may be used as an RSB prediction for an unbalanced RET if a
>> CALL instruction hasn't yet been retired. This is termed PBRSB (Post-Barrier
>> Return Stack Buffer).
>>
>> A mitigation for this was introduced in:
>> (2b1299322016731d56807aa49254a5ea3080b6b3 x86/speculation: Add RSB VM Exit protections)
>>
>> This mitigation [1] has a ~1% performance impact on VM exit compared to without
>> it [2].
>>
>> == Solution ==
>>
>> The WRMSR instruction can be used as a speculation barrier and a serialising
>> instruction. Use this on the VM exit path instead to ensure that a CALL
>> instruction (in this case the call to vmx_spec_ctrl_restore_host) has retired
>> before the prediction of a following unbalanced RET.
>>
>> This mitigation [3] has a negligible performance impact.
>>
>> == Testing ==
>>
>> Run the outl_to_kernel kvm-unit-tests test 200 times per configuration which
>> counts the cycles for an exit to kernel mode.
>>
>> [1] With existing mitigation:
>> Average: 2026 cycles
>> [2] With no mitigation:
>> Average: 2008 cycles
>> [3] With proposed mitigation:
>> Average: 2008 cycles
>>
>> Signed-off-by: Suraj Jitindar Singh <[email protected]>
>> Cc: [email protected]
>> ---
>> arch/x86/include/asm/nospec-branch.h | 7 +++----
>> arch/x86/kvm/vmx/vmenter.S | 3 +--
>> arch/x86/kvm/vmx/vmx.c | 5 +++++
>> 3 files changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
>> index c936ce9f0c47..e5723e024b47 100644
>> --- a/arch/x86/include/asm/nospec-branch.h
>> +++ b/arch/x86/include/asm/nospec-branch.h
>> @@ -159,10 +159,9 @@
>> * A simpler FILL_RETURN_BUFFER macro. Don't make people use the CPP
>> * monstrosity above, manually.
>> */
>> -.macro FILL_RETURN_BUFFER reg:req nr:req ftr:req ftr2=ALT_NOT(X86_FEATURE_ALWAYS)
>> - ALTERNATIVE_2 "jmp .Lskip_rsb_\@", \
>> - __stringify(__FILL_RETURN_BUFFER(\reg,\nr)), \ftr, \
>> - __stringify(__FILL_ONE_RETURN), \ftr2
>> +.macro FILL_RETURN_BUFFER reg:req nr:req ftr:req
>> + ALTERNATIVE "jmp .Lskip_rsb_\@", \
>> + __stringify(__FILL_RETURN_BUFFER(\reg,\nr)), \ftr
>>
>> .Lskip_rsb_\@:
>> .endm
>> diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
>> index 6de96b943804..eb82797bd7bf 100644
>> --- a/arch/x86/kvm/vmx/vmenter.S
>> +++ b/arch/x86/kvm/vmx/vmenter.S
>> @@ -231,8 +231,7 @@ SYM_INNER_LABEL(vmx_vmexit, SYM_L_GLOBAL)
>> * single call to retire, before the first unbalanced RET.
>> */
>>
>> - FILL_RETURN_BUFFER %_ASM_CX, RSB_CLEAR_LOOPS, X86_FEATURE_RSB_VMEXIT,\
>> - X86_FEATURE_RSB_VMEXIT_LITE
>> + FILL_RETURN_BUFFER %_ASM_CX, RSB_CLEAR_LOOPS, X86_FEATURE_RSB_VMEXIT
>>
>>
>> pop %_ASM_ARG2 /* @flags */
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index c9b49a09e6b5..fdcd8e10c2ab 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -7049,8 +7049,13 @@ void noinstr vmx_spec_ctrl_restore_host(struct vcpu_vmx *vmx,
>> * For legacy IBRS, the IBRS bit always needs to be written after
>> * transitioning from a less privileged predictor mode, regardless of
>> * whether the guest/host values differ.
>> + *
>> + * For eIBRS affected by Post Barrier RSB Predictions a serialising
>> + * instruction (wrmsr) must be executed to ensure a call instruction has
>> + * retired before the prediction of a following unbalanced ret.
>> */
>> if (cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS) ||
>> + cpu_feature_enabled(X86_FEATURE_RSB_VMEXIT_LITE) ||
>> vmx->spec_ctrl != hostval)
>> native_wrmsrl(MSR_IA32_SPEC_CTRL, hostval);
>
>Okay. I see how this almost meets the requirements. But this WRMSR is
>conditional, which means that there's a speculative path through this
>code that ends up at the unbalanced RET without executing the WRMSR.
Agree. I was just about to post this.
On Wed, Oct 5, 2022 at 3:03 PM Suraj Jitindar Singh <[email protected]> wrote:
>
> tl;dr: The existing mitigation for eIBRS PBRSB predictions uses an INT3 to
> ensure a call instruction retires before a following unbalanced RET. Replace
> this with a WRMSR serialising instruction which has a lower performance
> penalty.
>
> == Background ==
>
> eIBRS (enhanced indirect branch restricted speculation) is used to prevent
> predictor addresses from one privilege domain from being used for prediction
> in a higher privilege domain.
>
> == Problem ==
>
> On processors with eIBRS protections there can be a case where upon VM exit
> a guest address may be used as an RSB prediction for an unbalanced RET if a
> CALL instruction hasn't yet been retired. This is termed PBRSB (Post-Barrier
> Return Stack Buffer).
>
> A mitigation for this was introduced in:
> (2b1299322016731d56807aa49254a5ea3080b6b3 x86/speculation: Add RSB VM Exit protections)
>
> This mitigation [1] has a ~1% performance impact on VM exit compared to without
> it [2].
>
> == Solution ==
>
> The WRMSR instruction can be used as a speculation barrier and a serialising
> instruction. Use this on the VM exit path instead to ensure that a CALL
> instruction (in this case the call to vmx_spec_ctrl_restore_host) has retired
> before the prediction of a following unbalanced RET.
>
> This mitigation [3] has a negligible performance impact.
>
> == Testing ==
>
> Run the outl_to_kernel kvm-unit-tests test 200 times per configuration which
> counts the cycles for an exit to kernel mode.
>
> [1] With existing mitigation:
> Average: 2026 cycles
> [2] With no mitigation:
> Average: 2008 cycles
> [3] With proposed mitigation:
> Average: 2008 cycles
>
> Signed-off-by: Suraj Jitindar Singh <[email protected]>
> Cc: [email protected]
> ---
> arch/x86/include/asm/nospec-branch.h | 7 +++----
> arch/x86/kvm/vmx/vmenter.S | 3 +--
> arch/x86/kvm/vmx/vmx.c | 5 +++++
> 3 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
> index c936ce9f0c47..e5723e024b47 100644
> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h
> @@ -159,10 +159,9 @@
> * A simpler FILL_RETURN_BUFFER macro. Don't make people use the CPP
> * monstrosity above, manually.
> */
> -.macro FILL_RETURN_BUFFER reg:req nr:req ftr:req ftr2=ALT_NOT(X86_FEATURE_ALWAYS)
> - ALTERNATIVE_2 "jmp .Lskip_rsb_\@", \
> - __stringify(__FILL_RETURN_BUFFER(\reg,\nr)), \ftr, \
> - __stringify(__FILL_ONE_RETURN), \ftr2
> +.macro FILL_RETURN_BUFFER reg:req nr:req ftr:req
> + ALTERNATIVE "jmp .Lskip_rsb_\@", \
> + __stringify(__FILL_RETURN_BUFFER(\reg,\nr)), \ftr
>
> .Lskip_rsb_\@:
> .endm
> diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
> index 6de96b943804..eb82797bd7bf 100644
> --- a/arch/x86/kvm/vmx/vmenter.S
> +++ b/arch/x86/kvm/vmx/vmenter.S
> @@ -231,8 +231,7 @@ SYM_INNER_LABEL(vmx_vmexit, SYM_L_GLOBAL)
> * single call to retire, before the first unbalanced RET.
> */
>
> - FILL_RETURN_BUFFER %_ASM_CX, RSB_CLEAR_LOOPS, X86_FEATURE_RSB_VMEXIT,\
> - X86_FEATURE_RSB_VMEXIT_LITE
> + FILL_RETURN_BUFFER %_ASM_CX, RSB_CLEAR_LOOPS, X86_FEATURE_RSB_VMEXIT
>
>
> pop %_ASM_ARG2 /* @flags */
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index c9b49a09e6b5..fdcd8e10c2ab 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7049,8 +7049,13 @@ void noinstr vmx_spec_ctrl_restore_host(struct vcpu_vmx *vmx,
> * For legacy IBRS, the IBRS bit always needs to be written after
> * transitioning from a less privileged predictor mode, regardless of
> * whether the guest/host values differ.
> + *
> + * For eIBRS affected by Post Barrier RSB Predictions a serialising
> + * instruction (wrmsr) must be executed to ensure a call instruction has
> + * retired before the prediction of a following unbalanced ret.
> */
> if (cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS) ||
> + cpu_feature_enabled(X86_FEATURE_RSB_VMEXIT_LITE) ||
> vmx->spec_ctrl != hostval)
> native_wrmsrl(MSR_IA32_SPEC_CTRL, hostval);
Better, I think, would be to leave the condition alone and put an
LFENCE on the 'else' path:
if (cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS) ||
vmx->spec_ctrl != hostval)
native_wrmsrl(MSR_IA32_SPEC_CTRL, hostval);
else
rmb();
When the guest and host have different IA32_SPEC_CTRL values, you get
the serialization from the WRMSR. Otherwise, you get it from the
cheaper LFENCE.
This is still more convoluted than having the mitigation in one place.
On 10/5/22 16:46, Jim Mattson wrote:
> On Wed, Oct 5, 2022 at 3:03 PM Suraj Jitindar Singh <[email protected]> wrote:
>>
>> tl;dr: The existing mitigation for eIBRS PBRSB predictions uses an INT3 to
>> ensure a call instruction retires before a following unbalanced RET. Replace
>> this with a WRMSR serialising instruction which has a lower performance
>> penalty.
>>
>> == Background ==
>>
>> eIBRS (enhanced indirect branch restricted speculation) is used to prevent
>> predictor addresses from one privilege domain from being used for prediction
>> in a higher privilege domain.
>>
>> == Problem ==
>>
>> On processors with eIBRS protections there can be a case where upon VM exit
>> a guest address may be used as an RSB prediction for an unbalanced RET if a
>> CALL instruction hasn't yet been retired. This is termed PBRSB (Post-Barrier
>> Return Stack Buffer).
>>
>> A mitigation for this was introduced in:
>> (2b1299322016731d56807aa49254a5ea3080b6b3 x86/speculation: Add RSB VM Exit protections)
>>
>> This mitigation [1] has a ~1% performance impact on VM exit compared to without
>> it [2].
>>
>> == Solution ==
>>
>> The WRMSR instruction can be used as a speculation barrier and a serialising
>> instruction. Use this on the VM exit path instead to ensure that a CALL
>> instruction (in this case the call to vmx_spec_ctrl_restore_host) has retired
>> before the prediction of a following unbalanced RET.
>>
>> This mitigation [3] has a negligible performance impact.
>>
>> == Testing ==
>>
>> Run the outl_to_kernel kvm-unit-tests test 200 times per configuration which
>> counts the cycles for an exit to kernel mode.
>>
>> [1] With existing mitigation:
>> Average: 2026 cycles
>> [2] With no mitigation:
>> Average: 2008 cycles
>> [3] With proposed mitigation:
>> Average: 2008 cycles
>>
>> Signed-off-by: Suraj Jitindar Singh <[email protected]>
>> Cc: [email protected]
>> ---
>> arch/x86/include/asm/nospec-branch.h | 7 +++----
>> arch/x86/kvm/vmx/vmenter.S | 3 +--
>> arch/x86/kvm/vmx/vmx.c | 5 +++++
>> 3 files changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
>> index c936ce9f0c47..e5723e024b47 100644
>> --- a/arch/x86/include/asm/nospec-branch.h
>> +++ b/arch/x86/include/asm/nospec-branch.h
>> @@ -159,10 +159,9 @@
>> * A simpler FILL_RETURN_BUFFER macro. Don't make people use the CPP
>> * monstrosity above, manually.
>> */
>> -.macro FILL_RETURN_BUFFER reg:req nr:req ftr:req ftr2=ALT_NOT(X86_FEATURE_ALWAYS)
>> - ALTERNATIVE_2 "jmp .Lskip_rsb_\@", \
>> - __stringify(__FILL_RETURN_BUFFER(\reg,\nr)), \ftr, \
>> - __stringify(__FILL_ONE_RETURN), \ftr2
>> +.macro FILL_RETURN_BUFFER reg:req nr:req ftr:req
>> + ALTERNATIVE "jmp .Lskip_rsb_\@", \
>> + __stringify(__FILL_RETURN_BUFFER(\reg,\nr)), \ftr
>>
>> .Lskip_rsb_\@:
>> .endm
>> diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
>> index 6de96b943804..eb82797bd7bf 100644
>> --- a/arch/x86/kvm/vmx/vmenter.S
>> +++ b/arch/x86/kvm/vmx/vmenter.S
>> @@ -231,8 +231,7 @@ SYM_INNER_LABEL(vmx_vmexit, SYM_L_GLOBAL)
>> * single call to retire, before the first unbalanced RET.
>> */
>>
>> - FILL_RETURN_BUFFER %_ASM_CX, RSB_CLEAR_LOOPS, X86_FEATURE_RSB_VMEXIT,\
>> - X86_FEATURE_RSB_VMEXIT_LITE
>> + FILL_RETURN_BUFFER %_ASM_CX, RSB_CLEAR_LOOPS, X86_FEATURE_RSB_VMEXIT
>>
>>
>> pop %_ASM_ARG2 /* @flags */
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index c9b49a09e6b5..fdcd8e10c2ab 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -7049,8 +7049,13 @@ void noinstr vmx_spec_ctrl_restore_host(struct vcpu_vmx *vmx,
>> * For legacy IBRS, the IBRS bit always needs to be written after
>> * transitioning from a less privileged predictor mode, regardless of
>> * whether the guest/host values differ.
>> + *
>> + * For eIBRS affected by Post Barrier RSB Predictions a serialising
>> + * instruction (wrmsr) must be executed to ensure a call instruction has
>> + * retired before the prediction of a following unbalanced ret.
>> */
>> if (cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS) ||
>> + cpu_feature_enabled(X86_FEATURE_RSB_VMEXIT_LITE) ||
>> vmx->spec_ctrl != hostval)
>> native_wrmsrl(MSR_IA32_SPEC_CTRL, hostval);
>
> Better, I think, would be to leave the condition alone and put an
> LFENCE on the 'else' path:
>
> if (cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS) ||
> vmx->spec_ctrl != hostval)
> native_wrmsrl(MSR_IA32_SPEC_CTRL, hostval);
> else
> rmb();
>
> When the guest and host have different IA32_SPEC_CTRL values, you get
> the serialization from the WRMSR. Otherwise, you get it from the
> cheaper LFENCE.
In this case systems that don't suffer from PBRSB (i.e. don've have
X86_FEATURE_RSB_VMEXIT_LITE set) would be doing a barrier for no reason. We're
just trading performance on vulnerable systems for a performance hit on systems
that aren't vulnerable.
>
> This is still more convoluted than having the mitigation in one place.
Agreed.
On Wed, Oct 5, 2022 at 5:26 PM Daniel Sneddon
<[email protected]> wrote:
>
> On 10/5/22 16:46, Jim Mattson wrote:
> > On Wed, Oct 5, 2022 at 3:03 PM Suraj Jitindar Singh <[email protected]> wrote:
> >>
> >> tl;dr: The existing mitigation for eIBRS PBRSB predictions uses an INT3 to
> >> ensure a call instruction retires before a following unbalanced RET. Replace
> >> this with a WRMSR serialising instruction which has a lower performance
> >> penalty.
> >>
> >> == Background ==
> >>
> >> eIBRS (enhanced indirect branch restricted speculation) is used to prevent
> >> predictor addresses from one privilege domain from being used for prediction
> >> in a higher privilege domain.
> >>
> >> == Problem ==
> >>
> >> On processors with eIBRS protections there can be a case where upon VM exit
> >> a guest address may be used as an RSB prediction for an unbalanced RET if a
> >> CALL instruction hasn't yet been retired. This is termed PBRSB (Post-Barrier
> >> Return Stack Buffer).
> >>
> >> A mitigation for this was introduced in:
> >> (2b1299322016731d56807aa49254a5ea3080b6b3 x86/speculation: Add RSB VM Exit protections)
> >>
> >> This mitigation [1] has a ~1% performance impact on VM exit compared to without
> >> it [2].
> >>
> >> == Solution ==
> >>
> >> The WRMSR instruction can be used as a speculation barrier and a serialising
> >> instruction. Use this on the VM exit path instead to ensure that a CALL
> >> instruction (in this case the call to vmx_spec_ctrl_restore_host) has retired
> >> before the prediction of a following unbalanced RET.
> >>
> >> This mitigation [3] has a negligible performance impact.
> >>
> >> == Testing ==
> >>
> >> Run the outl_to_kernel kvm-unit-tests test 200 times per configuration which
> >> counts the cycles for an exit to kernel mode.
> >>
> >> [1] With existing mitigation:
> >> Average: 2026 cycles
> >> [2] With no mitigation:
> >> Average: 2008 cycles
> >> [3] With proposed mitigation:
> >> Average: 2008 cycles
> >>
> >> Signed-off-by: Suraj Jitindar Singh <[email protected]>
> >> Cc: [email protected]
> >> ---
> >> arch/x86/include/asm/nospec-branch.h | 7 +++----
> >> arch/x86/kvm/vmx/vmenter.S | 3 +--
> >> arch/x86/kvm/vmx/vmx.c | 5 +++++
> >> 3 files changed, 9 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
> >> index c936ce9f0c47..e5723e024b47 100644
> >> --- a/arch/x86/include/asm/nospec-branch.h
> >> +++ b/arch/x86/include/asm/nospec-branch.h
> >> @@ -159,10 +159,9 @@
> >> * A simpler FILL_RETURN_BUFFER macro. Don't make people use the CPP
> >> * monstrosity above, manually.
> >> */
> >> -.macro FILL_RETURN_BUFFER reg:req nr:req ftr:req ftr2=ALT_NOT(X86_FEATURE_ALWAYS)
> >> - ALTERNATIVE_2 "jmp .Lskip_rsb_\@", \
> >> - __stringify(__FILL_RETURN_BUFFER(\reg,\nr)), \ftr, \
> >> - __stringify(__FILL_ONE_RETURN), \ftr2
> >> +.macro FILL_RETURN_BUFFER reg:req nr:req ftr:req
> >> + ALTERNATIVE "jmp .Lskip_rsb_\@", \
> >> + __stringify(__FILL_RETURN_BUFFER(\reg,\nr)), \ftr
> >>
> >> .Lskip_rsb_\@:
> >> .endm
> >> diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
> >> index 6de96b943804..eb82797bd7bf 100644
> >> --- a/arch/x86/kvm/vmx/vmenter.S
> >> +++ b/arch/x86/kvm/vmx/vmenter.S
> >> @@ -231,8 +231,7 @@ SYM_INNER_LABEL(vmx_vmexit, SYM_L_GLOBAL)
> >> * single call to retire, before the first unbalanced RET.
> >> */
> >>
> >> - FILL_RETURN_BUFFER %_ASM_CX, RSB_CLEAR_LOOPS, X86_FEATURE_RSB_VMEXIT,\
> >> - X86_FEATURE_RSB_VMEXIT_LITE
> >> + FILL_RETURN_BUFFER %_ASM_CX, RSB_CLEAR_LOOPS, X86_FEATURE_RSB_VMEXIT
> >>
> >>
> >> pop %_ASM_ARG2 /* @flags */
> >> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> >> index c9b49a09e6b5..fdcd8e10c2ab 100644
> >> --- a/arch/x86/kvm/vmx/vmx.c
> >> +++ b/arch/x86/kvm/vmx/vmx.c
> >> @@ -7049,8 +7049,13 @@ void noinstr vmx_spec_ctrl_restore_host(struct vcpu_vmx *vmx,
> >> * For legacy IBRS, the IBRS bit always needs to be written after
> >> * transitioning from a less privileged predictor mode, regardless of
> >> * whether the guest/host values differ.
> >> + *
> >> + * For eIBRS affected by Post Barrier RSB Predictions a serialising
> >> + * instruction (wrmsr) must be executed to ensure a call instruction has
> >> + * retired before the prediction of a following unbalanced ret.
> >> */
> >> if (cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS) ||
> >> + cpu_feature_enabled(X86_FEATURE_RSB_VMEXIT_LITE) ||
> >> vmx->spec_ctrl != hostval)
> >> native_wrmsrl(MSR_IA32_SPEC_CTRL, hostval);
> >
> > Better, I think, would be to leave the condition alone and put an
> > LFENCE on the 'else' path:
> >
> > if (cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS) ||
> > vmx->spec_ctrl != hostval)
> > native_wrmsrl(MSR_IA32_SPEC_CTRL, hostval);
> > else
> > rmb();
> >
> > When the guest and host have different IA32_SPEC_CTRL values, you get
> > the serialization from the WRMSR. Otherwise, you get it from the
> > cheaper LFENCE.
> In this case systems that don't suffer from PBRSB (i.e. don've have
> X86_FEATURE_RSB_VMEXIT_LITE set) would be doing a barrier for no reason. We're
> just trading performance on vulnerable systems for a performance hit on systems
> that aren't vulnerable.
The lfence could be buried in an ALTERNATIVE keyed to
X86_FEATURE_RSB_VMEXIT_LITE.
> > This is still more convoluted than having the mitigation in one place.
> Agreed.
For a mere 18 cycles, it doesn't really seem worth the obfuscation.
On Wed, Oct 05, 2022 at 04:46:12PM -0700, Jim Mattson wrote:
> rmb();
Since we're not doing memory ordering but speculation hardening, this is
completely the wrong way to spell LFENCE. Please use barrier_nospec() in
those cases.
And as has already been pointed out, there's one of those just two lines
down already.
Also, what Andrew said.
From: Jim Mattson
> Sent: 05 October 2022 23:29
>
> On Wed, Oct 5, 2022 at 3:03 PM Suraj Jitindar Singh <[email protected]> wrote:
> >
> > tl;dr: The existing mitigation for eIBRS PBRSB predictions uses an INT3 to
> > ensure a call instruction retires before a following unbalanced RET. Replace
> > this with a WRMSR serialising instruction which has a lower performance
> > penalty.
>
> The INT3 is only on a speculative path and should not impact performance.
Doesn't that depend on how quickly the cpu can abort the
decode and execution of the INT3 instruction?
INT3 is bound to generate a lot of uops and/or be microcoded.
Old cpu couldn't abort fpu instructions.
IIRC the Intel performance guide even suggested not interleaving
code and data because the data might get speculatively executed
and take a long time to abort.
I actually wonder whether 'JMPS .' (eb fe) shouldn't be used
instead of INT3 (cc) because it is fast to decode and execute.
But I'm no expect here.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Thu, Oct 06, 2022 at 08:25:15AM +0000, David Laight wrote:
>From: Jim Mattson
>> Sent: 05 October 2022 23:29
>>
>> On Wed, Oct 5, 2022 at 3:03 PM Suraj Jitindar Singh <[email protected]> wrote:
>> >
>> > tl;dr: The existing mitigation for eIBRS PBRSB predictions uses an INT3 to
>> > ensure a call instruction retires before a following unbalanced RET. Replace
>> > this with a WRMSR serialising instruction which has a lower performance
>> > penalty.
>>
>> The INT3 is only on a speculative path and should not impact performance.
>
>Doesn't that depend on how quickly the cpu can abort the
>decode and execution of the INT3 instruction?
>INT3 is bound to generate a lot of uops and/or be microcoded.
>
>Old cpu couldn't abort fpu instructions.
>IIRC the Intel performance guide even suggested not interleaving
>code and data because the data might get speculatively executed
>and take a long time to abort.
>
>I actually wonder whether 'JMPS .' (eb fe) shouldn't be used
>instead of INT3 (cc) because it is fast to decode and execute.
>But I'm no expect here.
I have been told that INT3 is better in this case because 'JMP .' would
waste CPU resources.
On Thu, Oct 06, 2022 at 02:42:10AM +0000, Andrew Cooper wrote:
>On 05/10/2022 23:02, Suraj Jitindar Singh wrote:
>> == Solution ==
>>
>> The WRMSR instruction can be used as a speculation barrier and a serialising
>> instruction. Use this on the VM exit path instead to ensure that a CALL
>> instruction (in this case the call to vmx_spec_ctrl_restore_host) has retired
>> before the prediction of a following unbalanced RET.
>
>While both of these sentences are true statements, you've missed the
>necessary safety property.
>
>One CALL has to retire before *any* RET can execute.
>
>There are several ways the frontend can end up eventually consuming the
>bad RSB entry; they all stem from an execute (not prediction) of the
>next RET instruction.
>
>As to the change, ...
>
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index c9b49a09e6b5..fdcd8e10c2ab 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -7049,8 +7049,13 @@ void noinstr vmx_spec_ctrl_restore_host(struct vcpu_vmx *vmx,
>
>... out of context above this hunk is:
>
> if (!cpu_feature_enabled(X86_FEATURE_MSR_SPEC_CTRL))
> return;
>
>meaning that there is a return instruction which is programmatically
>reachable ahead of the WRMSR.
>
>Whether it is speculatively reachable depends on whether the frontend
>can see through the _static_cpu_has(), as well as
>X86_FEATURE_MSR_SPEC_CTRL never becoming compile time evaluable.
In this case wouldn't _static_cpu_has() be runtime patched to a JMP
(<+8> below) or a NOP? RET (at <+13>) should not be reachable even
speculatively. What am I missing?
Dump of assembler code for function vmx_spec_ctrl_restore_host:
arch/x86/kvm/vmx/vmx.c:
u64 hostval = this_cpu_read(x86_spec_ctrl_current);
<+0>: mov %gs:0x7e022e60(%rip),%r8 # 0x1ad48 <x86_spec_ctrl_current>
./arch/x86/include/asm/cpufeature.h:
asm_volatile_goto(
<+8>: jmp <vmx_spec_ctrl_restore_host+14>
<+10>: nopl (%rax)
<+13>: ret
arch/x86/kvm/vmx/vmx.c:
if (flags & VMX_RUN_SAVE_SPEC_CTRL)
<+14>: and $0x2,%esi
<+17>: je <vmx_spec_ctrl_restore_host+40>
[...]
Hi Suraj,
On Wed, Oct 05, 2022 at 03:02:27PM -0700, Suraj Jitindar Singh wrote:
>tl;dr: The existing mitigation for eIBRS PBRSB predictions uses an INT3 to
>ensure a call instruction retires before a following unbalanced RET. Replace
>this with a WRMSR serialising instruction which has a lower performance
>penalty.
>
>== Background ==
>
>eIBRS (enhanced indirect branch restricted speculation) is used to prevent
>predictor addresses from one privilege domain from being used for prediction
>in a higher privilege domain.
>
>== Problem ==
>
>On processors with eIBRS protections there can be a case where upon VM exit
>a guest address may be used as an RSB prediction for an unbalanced RET if a
>CALL instruction hasn't yet been retired. This is termed PBRSB (Post-Barrier
>Return Stack Buffer).
>
>A mitigation for this was introduced in:
>(2b1299322016731d56807aa49254a5ea3080b6b3 x86/speculation: Add RSB VM Exit protections)
>
>This mitigation [1] has a ~1% performance impact on VM exit compared to without
>it [2].
>
>== Solution ==
>
>The WRMSR instruction can be used as a speculation barrier and a serialising
>instruction. Use this on the VM exit path instead to ensure that a CALL
>instruction (in this case the call to vmx_spec_ctrl_restore_host) has retired
>before the prediction of a following unbalanced RET.
>
>This mitigation [3] has a negligible performance impact.
>
>== Testing ==
>
>Run the outl_to_kernel kvm-unit-tests test 200 times per configuration which
>counts the cycles for an exit to kernel mode.
>
>[1] With existing mitigation:
>Average: 2026 cycles
>[2] With no mitigation:
>Average: 2008 cycles
During these tests was the value of MSR SPEC_CTRL for host and guest different?
>[3] With proposed mitigation:
>Average: 2008 cycles