On Thu, Apr 23, 2020 at 10:17:41AM -0700, Sean Christopherson wrote:
> On Thu, Mar 26, 2020 at 04:18:40PM +0800, Yang Weijiang wrote:
> > "Load {guest,host} CET state" bit controls whether guest/host
> > CET states will be loaded at VM entry/exit.
> > Set default host kernel CET states to 0s in VMCS to avoid guest
> > CET states leakage. When CR4.CET is cleared due to guest mode
> > change, make guest CET states invalid in VMCS, this can happen,
> > e.g., guest reboot.
> >
> > Signed-off-by: Yang Weijiang <weijiang.yang@xxxxxxxxx>
> > ---
> > arch/x86/kvm/vmx/capabilities.h | 10 ++++++
> > arch/x86/kvm/vmx/vmx.c | 56 +++++++++++++++++++++++++++++++--
> > 2 files changed, 63 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx/capabilities.h b/arch/x86/kvm/vmx/capabilities.h
> > index 8903475f751e..565340352260 100644
> > --- a/arch/x86/kvm/vmx/capabilities.h
> > +++ b/arch/x86/kvm/vmx/capabilities.h
> > @@ -107,6 +107,16 @@ static inline bool cpu_has_vmx_mpx(void)
> > (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_BNDCFGS);
> > }
> >
> > +static inline bool cpu_has_cet_guest_load_ctrl(void)
> > +{
> > + return (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_GUEST_CET_STATE);
> > +}
> > +
> > +static inline bool cpu_has_cet_host_load_ctrl(void)
> > +{
> > + return (vmcs_config.vmexit_ctrl & VM_EXIT_LOAD_HOST_CET_STATE);
> > +}
>
> We should bundle these together, same as we do for PERF_GLOBAL_CTRL. Not
> just for code clarity, but also for functionality, e.g. if KVM ends up on a
> frankenstein CPU with VM_ENTRY_LOAD_CET_STATE and !VM_EXIT_LOAD_CET_STATE
> then KVM will end up running with guest state. This is also an argument
> for not qualifying the control names with GUEST vs. HOST.
Cannot agree with you more! Thank you!
Will fix them.
> > +
> > static inline bool cpu_has_vmx_tpr_shadow(void)
> > {
> > int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
> > {
> > struct vcpu_vmx *vmx = to_vmx(vcpu);
> > @@ -3110,6 +3119,12 @@ int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
> > hw_cr4 &= ~(X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_PKE);
> > }
> >
> > + if (!(hw_cr4 & X86_CR4_CET) && is_cet_supported(vcpu)) {
> > + vmcs_writel(GUEST_SSP, 0);
> > + vmcs_writel(GUEST_S_CET, 0);
> > + vmcs_writel(GUEST_INTR_SSP_TABLE, 0);
>
> Can't we simply toggle the VM_{ENTRY,EXIT}_LOAD controls? If CR4.CET=0 in
> the guest, then presumably keeping host state loaded is ok? I.e. won't
> leak host information to the guest.
>
Yes, it's doable, let me change it.
> > + }
> > +
> > vmcs_writel(CR4_READ_SHADOW, cr4);
> > vmcs_writel(GUEST_CR4, hw_cr4);
> > return 0;
> > @@ -3939,6 +3954,12 @@ void vmx_set_constant_host_state(struct vcpu_vmx *vmx)
> >
> > if (cpu_has_load_ia32_efer())
> > vmcs_write64(HOST_IA32_EFER, host_efer);
> > +
> > + if (cpu_has_cet_host_load_ctrl()) {
> > + vmcs_writel(HOST_S_CET, 0);
> > + vmcs_writel(HOST_INTR_SSP_TABLE, 0);
> > + vmcs_writel(HOST_SSP, 0);
>
> This is unnecessary, the VMCS is zeroed on allocation.
>
> And IIUC, this is only correct because the main shadow stack enabling only
> adds user support. Assuming that's correct, (a) it absolutely needs to be
> called out in the changelog and (b) KVM needs a WARN in hardware_setup() to
> guard against kernel shadow stack support being added without updating KVM,
> e.g. something this (not sure the MSRs are correct):
>
> if (boot_cpu_has(X86_FEATURE_IBT) || boot_cpu_has(X86_FEATURE_SHSTK)) {
> rdmsrl(MSR_IA32_S_CET, cet_msr);
> WARN_ONCE(cet_msr, "KVM: S_CET in host will be lost");
>
> }
> if (boot_cpu_has(X86_FEATURE_SHSTK)) {
> rdmsrl(MSR_IA32_PL0_SSP, cet_msr);
> WARN_ONCE(cet_msr, "KVM: PL0_SPP in host will be lost");
> }
>
Yes, just wanted to make host CET stuffs invalid before kernel mode
CET is available. OK, will follow your advice, thanks!
> > + }
> > }
> > /*
> > @@ -7140,8 +7175,23 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
> > }
> >
> > if (guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) ||
> > - guest_cpuid_has(vcpu, X86_FEATURE_IBT))
> > + guest_cpuid_has(vcpu, X86_FEATURE_IBT)) {
> > vmx_update_intercept_for_cet_msr(vcpu);
> > +
> > + if (cpu_has_cet_guest_load_ctrl() && is_cet_supported(vcpu))
> > + vm_entry_controls_setbit(to_vmx(vcpu),
> > + VM_ENTRY_LOAD_GUEST_CET_STATE);
> > + else
> > + vm_entry_controls_clearbit(to_vmx(vcpu),
> > + VM_ENTRY_LOAD_GUEST_CET_STATE);
> > +
> > + if (cpu_has_cet_host_load_ctrl() && is_cet_supported(vcpu))
> > + vm_exit_controls_setbit(to_vmx(vcpu),
> > + VM_EXIT_LOAD_HOST_CET_STATE);
> > + else
> > + vm_exit_controls_clearbit(to_vmx(vcpu),
> > + VM_EXIT_LOAD_HOST_CET_STATE);
>
> As above, I think this can be done in vmx_set_cr4().
>
Hmm, it's in vmx_set_cr4() in early versions, OK, will move them back.
> > + }
> > }
> >
> > static __init void vmx_set_cpu_caps(void)
> > --
> > 2.17.2
> >
