[PATCH] KVM: x86: use X86_FEATURE_RSB_CTXSW for RSB stuffing in vmexit

[PATCH] KVM: x86: use X86_FEATURE_RSB_CTXSW for RSB stuffing in vmexit

[Jon Kohler]

cpufeatures.h defines X86_FEATURE_RSB_CTXSW as "Fill RSB on context
switches" which seems more accurate than using X86_FEATURE_RETPOLINE
in the vmxexit path for RSB stuffing.

X86_FEATURE_RSB_CTXSW is used for FILL_RETURN_BUFFER in
arch/x86/entry/entry_{32|64}.S. This change makes KVM vmx and svm
follow that same pattern. This pairs up nicely with the language in
bugs.c, where this cpu_cap is enabled, which indicates that RSB
stuffing should be unconditional with spectrev2 enabled.
	/*
	 * If spectre v2 protection has been enabled, unconditionally fill
	 * RSB during a context switch; this protects against two independent
	 * issues:
	 *
	 *	- RSB underflow (and switch to BTB) on Skylake+
	 *	- SpectreRSB variant of spectre v2 on X86_BUG_SPECTRE_V2 CPUs
	 */
	setup_force_cpu_cap(X86_FEATURE_RSB_CTXSW);

Furthermore, on X86_FEATURE_IBRS_ENHANCED CPUs && SPECTRE_V2_CMD_AUTO,
we're bypassing setting X86_FEATURE_RETPOLINE, where as far as I could
find, we should still be doing RSB stuffing no matter what when
CONFIG_RETPOLINE is enabled and spectrev2 is set to auto.

Signed-off-by: Jon Kohler <jon@nutanix.com>
---
 arch/x86/kvm/svm/vmenter.S | 4 ++--
 arch/x86/kvm/vmx/vmenter.S | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/svm/vmenter.S b/arch/x86/kvm/svm/vmenter.S
index 4fa17df123cd..fe81012da4b5 100644
--- a/arch/x86/kvm/svm/vmenter.S
+++ b/arch/x86/kvm/svm/vmenter.S
@@ -86,7 +86,7 @@ SYM_FUNC_START(__svm_vcpu_run)
 
 #ifdef CONFIG_RETPOLINE
 	/* IMPORTANT: Stuff the RSB immediately after VM-Exit, before RET! */
-	FILL_RETURN_BUFFER %_ASM_AX, RSB_CLEAR_LOOPS, X86_FEATURE_RETPOLINE
+	FILL_RETURN_BUFFER %_ASM_AX, RSB_CLEAR_LOOPS, X86_FEATURE_RSB_CTXSW
 #endif
 
 	/* "POP" @regs to RAX. */
@@ -187,7 +187,7 @@ SYM_FUNC_START(__svm_sev_es_vcpu_run)
 
 #ifdef CONFIG_RETPOLINE
 	/* IMPORTANT: Stuff the RSB immediately after VM-Exit, before RET! */
-	FILL_RETURN_BUFFER %_ASM_AX, RSB_CLEAR_LOOPS, X86_FEATURE_RETPOLINE
+	FILL_RETURN_BUFFER %_ASM_AX, RSB_CLEAR_LOOPS, X86_FEATURE_RSB_CTXSW
 #endif
 
 	pop %_ASM_BX
diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
index 3a6461694fc2..ede6aac7d8b7 100644
--- a/arch/x86/kvm/vmx/vmenter.S
+++ b/arch/x86/kvm/vmx/vmenter.S
@@ -76,12 +76,12 @@ SYM_FUNC_END(vmx_vmenter)
  */
 SYM_FUNC_START(vmx_vmexit)
 #ifdef CONFIG_RETPOLINE
-	ALTERNATIVE "jmp .Lvmexit_skip_rsb", "", X86_FEATURE_RETPOLINE
+	ALTERNATIVE "jmp .Lvmexit_skip_rsb", "", X86_FEATURE_RSB_CTXSW
 	/* Preserve guest's RAX, it's used to stuff the RSB. */
 	push %_ASM_AX
 
 	/* IMPORTANT: Stuff the RSB immediately after VM-Exit, before RET! */
-	FILL_RETURN_BUFFER %_ASM_AX, RSB_CLEAR_LOOPS, X86_FEATURE_RETPOLINE
+	FILL_RETURN_BUFFER %_ASM_AX, RSB_CLEAR_LOOPS, X86_FEATURE_RSB_CTXSW
 
 	/* Clear RFLAGS.CF and RFLAGS.ZF to preserve VM-Exit, i.e. !VM-Fail. */
 	or $1, %_ASM_AX
-- 
2.30.1 (Apple Git-130)

Re: [PATCH] KVM: x86: use X86_FEATURE_RSB_CTXSW for RSB stuffing in vmexit

[Venkatesh Srinivas]

On Fri, May 7, 2021 at 8:08 AM Jon Kohler <jon@nutanix.com> wrote:
>
> cpufeatures.h defines X86_FEATURE_RSB_CTXSW as "Fill RSB on context
> switches" which seems more accurate than using X86_FEATURE_RETPOLINE
> in the vmxexit path for RSB stuffing.
>
> X86_FEATURE_RSB_CTXSW is used for FILL_RETURN_BUFFER in
> arch/x86/entry/entry_{32|64}.S. This change makes KVM vmx and svm
> follow that same pattern. This pairs up nicely with the language in
> bugs.c, where this cpu_cap is enabled, which indicates that RSB
> stuffing should be unconditional with spectrev2 enabled.
>         /*
>          * If spectre v2 protection has been enabled, unconditionally fill
>          * RSB during a context switch; this protects against two independent
>          * issues:
>          *
>          *      - RSB underflow (and switch to BTB) on Skylake+
>          *      - SpectreRSB variant of spectre v2 on X86_BUG_SPECTRE_V2 CPUs
>          */
>         setup_force_cpu_cap(X86_FEATURE_RSB_CTXSW);
>
> Furthermore, on X86_FEATURE_IBRS_ENHANCED CPUs && SPECTRE_V2_CMD_AUTO,
> we're bypassing setting X86_FEATURE_RETPOLINE, where as far as I could
> find, we should still be doing RSB stuffing no matter what when
> CONFIG_RETPOLINE is enabled and spectrev2 is set to auto.

If I'm reading https://software.intel.com/security-software-guidance/deep-dives/deep-dive-indirect-branch-restricted-speculation
correctly, I don't think an RSB fill sequence is required on VMExit on
processors w/ Enhanced IBRS. Specifically:
"""
On processors with enhanced IBRS, an RSB overwrite sequence may not
suffice to prevent the predicted target of a near return from using an
RSB entry created in a less privileged predictor mode.  Software can
prevent this by enabling SMEP (for transitions from user mode to
supervisor mode) and by having IA32_SPEC_CTRL.IBRS set during VM exits
"""
On Enhanced IBRS processors, it looks like SPEC_CTRL.IBRS is set
across all #VMExits via x86_virt_spec_ctrl in kvm.

So is this patch needed?

Thanks,
-- vs;

Re: [PATCH] KVM: x86: use X86_FEATURE_RSB_CTXSW for RSB stuffing in vmexit

[Jon Kohler]

> On May 7, 2021, at 1:22 PM, Venkatesh Srinivas <venkateshs@chromium.org> wrote:
> 
> On Fri, May 7, 2021 at 8:08 AM Jon Kohler <jon@nutanix.com> wrote:
>> 
>> cpufeatures.h defines X86_FEATURE_RSB_CTXSW as "Fill RSB on context
>> switches" which seems more accurate than using X86_FEATURE_RETPOLINE
>> in the vmxexit path for RSB stuffing.
>> 
>> X86_FEATURE_RSB_CTXSW is used for FILL_RETURN_BUFFER in
>> arch/x86/entry/entry_{32|64}.S. This change makes KVM vmx and svm
>> follow that same pattern. This pairs up nicely with the language in
>> bugs.c, where this cpu_cap is enabled, which indicates that RSB
>> stuffing should be unconditional with spectrev2 enabled.
>>        /*
>>         * If spectre v2 protection has been enabled, unconditionally fill
>>         * RSB during a context switch; this protects against two independent
>>         * issues:
>>         *
>>         *      - RSB underflow (and switch to BTB) on Skylake+
>>         *      - SpectreRSB variant of spectre v2 on X86_BUG_SPECTRE_V2 CPUs
>>         */
>>        setup_force_cpu_cap(X86_FEATURE_RSB_CTXSW);
>> 
>> Furthermore, on X86_FEATURE_IBRS_ENHANCED CPUs && SPECTRE_V2_CMD_AUTO,
>> we're bypassing setting X86_FEATURE_RETPOLINE, where as far as I could
>> find, we should still be doing RSB stuffing no matter what when
>> CONFIG_RETPOLINE is enabled and spectrev2 is set to auto.
> 
> If I'm reading https://urldefense.proofpoint.com/v2/url?u=https-3A__software.intel.com_security-2Dsoftware-2Dguidance_deep-2Ddives_deep-2Ddive-2Dindirect-2Dbranch-2Drestricted-2Dspeculation&d=DwIBaQ&c=s883GpUCOChKOHiocYtGcg&r=NGPRGGo37mQiSXgHKm5rCQ&m=s8fqknrIuUa_jGbbihj0anypC4jz86QQ7UzzAop3B7k&s=oIcZtb8S_gcK5L1yzfPvinSHxjCCsx1PNn-imPMffKU&e= 
> correctly, I don't think an RSB fill sequence is required on VMExit on
> processors w/ Enhanced IBRS. Specifically:
> """
> On processors with enhanced IBRS, an RSB overwrite sequence may not
> suffice to prevent the predicted target of a near return from using an
> RSB entry created in a less privileged predictor mode.  Software can
> prevent this by enabling SMEP (for transitions from user mode to
> supervisor mode) and by having IA32_SPEC_CTRL.IBRS set during VM exits
> """
> On Enhanced IBRS processors, it looks like SPEC_CTRL.IBRS is set
> across all #VMExits via x86_virt_spec_ctrl in kvm.
> 
> So is this patch needed?
> 
> Thanks,
> -- vs;

Venkatesh - Thanks for the reply. I read that the other way around, wherein
RSB overwrite still isn't good enough on eIBRS, so one would need to do all
three of the following to be in good shape: 
a. RSB overwrite sequence
b. enable SMEP
c. toggle IA32_SPEC_CTRL.IBRS on vmexits 

Said another way, the document reads like one would always need to do the
RSB overwrite sequence no matter what. Happy to hear if that is not the
case though, since RSB stuffing is a little expensive.

Note: I also checked the Intel SDM to see if perhaps there was something
there about this, but the document you linked is the only one I could
find on the topic.

Re: [PATCH] KVM: x86: use X86_FEATURE_RSB_CTXSW for RSB stuffing in vmexit

[Sean Christopherson]

On Fri, May 07, 2021, Venkatesh Srinivas wrote:
> On Fri, May 7, 2021 at 8:08 AM Jon Kohler <jon@nutanix.com> wrote:
> >
> > cpufeatures.h defines X86_FEATURE_RSB_CTXSW as "Fill RSB on context
> > switches" which seems more accurate than using X86_FEATURE_RETPOLINE
> > in the vmxexit path for RSB stuffing.
> >
> > X86_FEATURE_RSB_CTXSW is used for FILL_RETURN_BUFFER in
> > arch/x86/entry/entry_{32|64}.S. This change makes KVM vmx and svm
> > follow that same pattern. This pairs up nicely with the language in
> > bugs.c, where this cpu_cap is enabled, which indicates that RSB
> > stuffing should be unconditional with spectrev2 enabled.
> >         /*
> >          * If spectre v2 protection has been enabled, unconditionally fill
> >          * RSB during a context switch; this protects against two independent
> >          * issues:
> >          *
> >          *      - RSB underflow (and switch to BTB) on Skylake+
> >          *      - SpectreRSB variant of spectre v2 on X86_BUG_SPECTRE_V2 CPUs
> >          */
> >         setup_force_cpu_cap(X86_FEATURE_RSB_CTXSW);
> >
> > Furthermore, on X86_FEATURE_IBRS_ENHANCED CPUs && SPECTRE_V2_CMD_AUTO,
> > we're bypassing setting X86_FEATURE_RETPOLINE, where as far as I could
> > find, we should still be doing RSB stuffing no matter what when
> > CONFIG_RETPOLINE is enabled and spectrev2 is set to auto.
> 
> If I'm reading https://software.intel.com/security-software-guidance/deep-dives/deep-dive-indirect-branch-restricted-speculation
> correctly, I don't think an RSB fill sequence is required on VMExit on
> processors w/ Enhanced IBRS. Specifically:
> """
> On processors with enhanced IBRS, an RSB overwrite sequence may not
> suffice to prevent the predicted target of a near return from using an
> RSB entry created in a less privileged predictor mode.  Software can
> prevent this by enabling SMEP (for transitions from user mode to
> supervisor mode) and by having IA32_SPEC_CTRL.IBRS set during VM exits
> """
> On Enhanced IBRS processors, it looks like SPEC_CTRL.IBRS is set
> across all #VMExits via x86_virt_spec_ctrl in kvm.
> 
> So is this patch needed?

Venkatesh belatedly pointed out (off list) that KVM VMX stops intercepting
MSR_IA32_SPEC_CTRL after the first (successful) write by the guest.  But, I 
believe that's a non-issue for ENHANCED_IBRS because of this blurb in Intel's
documentation[*]:

  Processors with enhanced IBRS still support the usage model where IBRS is set
  only in the OS/VMM for OSes that enable SMEP. To do this, such processors will
  ensure that guest behavior cannot control the RSB after a VM exit once IBRS is
  set, even if IBRS was not set at the time of the VM exit.

The code and changelog for commit 706d51681d63 ("x86/speculation: Support
Enhanced IBRS on future CPUs") is more than a little confusing:

  spectre_v2_select_mitigation():
	if (boot_cpu_has(X86_FEATURE_IBRS_ENHANCED)) {
		mode = SPECTRE_V2_IBRS_ENHANCED;
		/* Force it so VMEXIT will restore correctly */
		x86_spec_ctrl_base |= SPEC_CTRL_IBRS;
		wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
		goto specv2_set_mode;
	}


  changelog:
	Kernel also has to make sure that IBRS bit remains set after
	VMEXIT because the guest might have cleared the bit. This is already
	covered by the existing x86_spec_ctrl_set_guest() and
	x86_spec_ctrl_restore_host() speculation control functions.

but I _think_ that is simply saying that MSR_IA32_SPEC_CTRL.IBRS needs to be
restored in order to keep the mitigations active in the host.   I don't think it
contradicts the documentation that says VM-Exit is automagically mitigated if
IBRS has _ever_ been set.

[*] https://software.intel.com/security-software-guidance/deep-dives/deep-dive-indirect-branch-restricted-speculation

Re: [PATCH] KVM: x86: use X86_FEATURE_RSB_CTXSW for RSB stuffing in vmexit

[Jon Kohler]

> On May 7, 2021, at 1:58 PM, Sean Christopherson <seanjc@google.com> wrote:
> 
> On Fri, May 07, 2021, Venkatesh Srinivas wrote:
>> On Fri, May 7, 2021 at 8:08 AM Jon Kohler <jon@nutanix.com> wrote:
>>> 
>>> cpufeatures.h defines X86_FEATURE_RSB_CTXSW as "Fill RSB on context
>>> switches" which seems more accurate than using X86_FEATURE_RETPOLINE
>>> in the vmxexit path for RSB stuffing.
>>> 
>>> X86_FEATURE_RSB_CTXSW is used for FILL_RETURN_BUFFER in
>>> arch/x86/entry/entry_{32|64}.S. This change makes KVM vmx and svm
>>> follow that same pattern. This pairs up nicely with the language in
>>> bugs.c, where this cpu_cap is enabled, which indicates that RSB
>>> stuffing should be unconditional with spectrev2 enabled.
>>>        /*
>>>         * If spectre v2 protection has been enabled, unconditionally fill
>>>         * RSB during a context switch; this protects against two independent
>>>         * issues:
>>>         *
>>>         *      - RSB underflow (and switch to BTB) on Skylake+
>>>         *      - SpectreRSB variant of spectre v2 on X86_BUG_SPECTRE_V2 CPUs
>>>         */
>>>        setup_force_cpu_cap(X86_FEATURE_RSB_CTXSW);
>>> 
>>> Furthermore, on X86_FEATURE_IBRS_ENHANCED CPUs && SPECTRE_V2_CMD_AUTO,
>>> we're bypassing setting X86_FEATURE_RETPOLINE, where as far as I could
>>> find, we should still be doing RSB stuffing no matter what when
>>> CONFIG_RETPOLINE is enabled and spectrev2 is set to auto.
>> 
>> If I'm reading https://urldefense.proofpoint.com/v2/url?u=https-3A__software.intel.com_security-2Dsoftware-2Dguidance_deep-2Ddives_deep-2Ddive-2Dindirect-2Dbranch-2Drestricted-2Dspeculation&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=NGPRGGo37mQiSXgHKm5rCQ&m=d1CkIBCdVFwUtKYx3SRW9dZD0kA_IX9VKEPG2-x4kBo&s=KlK_T41o6UVpLhMKDcK9iZfsJnop72K3CveJVIak5K8&e= 
>> correctly, I don't think an RSB fill sequence is required on VMExit on
>> processors w/ Enhanced IBRS. Specifically:
>> """
>> On processors with enhanced IBRS, an RSB overwrite sequence may not
>> suffice to prevent the predicted target of a near return from using an
>> RSB entry created in a less privileged predictor mode.  Software can
>> prevent this by enabling SMEP (for transitions from user mode to
>> supervisor mode) and by having IA32_SPEC_CTRL.IBRS set during VM exits
>> """
>> On Enhanced IBRS processors, it looks like SPEC_CTRL.IBRS is set
>> across all #VMExits via x86_virt_spec_ctrl in kvm.
>> 
>> So is this patch needed?
> 
> Venkatesh belatedly pointed out (off list) that KVM VMX stops intercepting
> MSR_IA32_SPEC_CTRL after the first (successful) write by the guest.  But, I 
> believe that's a non-issue for ENHANCED_IBRS because of this blurb in Intel's
> documentation[*]:
> 
>  Processors with enhanced IBRS still support the usage model where IBRS is set
>  only in the OS/VMM for OSes that enable SMEP. To do this, such processors will
>  ensure that guest behavior cannot control the RSB after a VM exit once IBRS is
>  set, even if IBRS was not set at the time of the VM exit.
> 
> The code and changelog for commit 706d51681d63 ("x86/speculation: Support
> Enhanced IBRS on future CPUs") is more than a little confusing:
> 
>  spectre_v2_select_mitigation():
> 	if (boot_cpu_has(X86_FEATURE_IBRS_ENHANCED)) {
> 		mode = SPECTRE_V2_IBRS_ENHANCED;
> 		/* Force it so VMEXIT will restore correctly */
> 		x86_spec_ctrl_base |= SPEC_CTRL_IBRS;
> 		wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
> 		goto specv2_set_mode;
> 	}

Thanks Sean, that makes sense. The handling of the MSR part of things
is separate from what I was looking at, which is where it seems like
Intel is still recommending doing an RSB overwrite/stuff even with
eIBRS; however, in KVM we use X86_FEATURE_RETPOLINE to figure
that out, but in bugs.c eIBRS systems skip having set, as
goto specv2_set_mode skips cover retpoline_auto now.

> 
>  changelog:
> 	Kernel also has to make sure that IBRS bit remains set after
> 	VMEXIT because the guest might have cleared the bit. This is already
> 	covered by the existing x86_spec_ctrl_set_guest() and
> 	x86_spec_ctrl_restore_host() speculation control functions.
> 
> but I _think_ that is simply saying that MSR_IA32_SPEC_CTRL.IBRS needs to be
> restored in order to keep the mitigations active in the host.   I don't think it
> contradicts the documentation that says VM-Exit is automagically mitigated if
> IBRS has _ever_ been set.
> 
> [*] https://urldefense.proofpoint.com/v2/url?u=https-3A__software.intel.com_security-2Dsoftware-2Dguidance_deep-2Ddives_deep-2Ddive-2Dindirect-2Dbranch-2Drestricted-2Dspeculation&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=NGPRGGo37mQiSXgHKm5rCQ&m=d1CkIBCdVFwUtKYx3SRW9dZD0kA_IX9VKEPG2-x4kBo&s=KlK_T41o6UVpLhMKDcK9iZfsJnop72K3CveJVIak5K8&e=