| Message ID | 20210507150636.94389-1-jon@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: use X86_FEATURE_RSB_CTXSW for RSB stuffing in vmexit | expand |
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;
> 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.
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
> 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=
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
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(-)