| Message ID | 20190927021927.23057-5-weijiang.yang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce support for Guest CET feature | expand |
On Thu, Sep 26, 2019 at 7:17 PM Yang Weijiang <weijiang.yang@intel.com> wrote: > > "Load Guest CET state" bit controls whether Guest CET states > will be loaded at Guest entry. Before doing that, KVM needs > to check if CPU CET feature is enabled on host and available > to Guest. > > Note: SHSTK and IBT features share one control MSR: > MSR_IA32_{U,S}_CET, which means it's difficult to hide > one feature from another in the case of SHSTK != IBT, > after discussed in community, it's agreed to allow Guest > control two features independently as it won't introduce > security hole. > > Co-developed-by: Zhang Yi Z <yi.z.zhang@linux.intel.com> > Signed-off-by: Zhang Yi Z <yi.z.zhang@linux.intel.com> > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > --- > arch/x86/kvm/vmx/vmx.c | 35 +++++++++++++++++++++++++++++++++++ > 1 file changed, 35 insertions(+) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index f720baa7a9ba..ba1a83d11e69 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -44,6 +44,7 @@ > #include <asm/spec-ctrl.h> > #include <asm/virtext.h> > #include <asm/vmx.h> > +#include <asm/cet.h> > > #include "capabilities.h" > #include "cpuid.h" > @@ -2918,6 +2919,37 @@ void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) > vmcs_writel(GUEST_CR3, guest_cr3); > } > > +static int set_cet_bit(struct kvm_vcpu *vcpu, unsigned long cr4) Nit: This function does not appear to set CR4.CET, as the name would imply. > +{ > + struct vcpu_vmx *vmx = to_vmx(vcpu); > + const u64 cet_bits = XFEATURE_MASK_CET_USER | XFEATURE_MASK_CET_KERNEL; > + bool cet_xss = vmx_xsaves_supported() && > + (kvm_supported_xss() & cet_bits); > + bool cet_cpuid = guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) || > + guest_cpuid_has(vcpu, X86_FEATURE_IBT); > + bool cet_on = !!(cr4 & X86_CR4_CET); > + > + if (cet_on && vmx->nested.vmxon) > + return 1; This constraint doesn't appear to be architected. Also, this prevents enabling CR4.CET when in VMX operation, but what about the other way around (i.e. entering VMX operation with CR4.CET enabled)? > + if (cet_on && !cpu_x86_cet_enabled()) > + return 1; This seems odd. Why is kernel support for (SHSTK or IBT) required for the guest to use (SHSTK or IBT)? If there's a constraint, shouldn't it be 1:1? (i.e. kernel support for SHSTK is required for the guest to use SHSTK and kernel support for IBT is required for the guest to use IBT?) Either way, enforcement of this constraint seems late, here, when the guest is trying to set CR4 to a value that, per the guest CPUID information, should be legal. Shouldn't this constraint be applied when setting the guest CPUID information, disallowing the enumeration of SHSTK and/or IBT support on a platform where these features are unavailable or disabled in the kernel? > + if (cet_on && !cet_xss) > + return 1; Again, this constraint seems like it's being applied too late. Returning 1 here will result in the guest's MOV-to-CR4 raising #GP, even though there is no architected reason for it to do so. > + if (cet_on && !cet_cpuid) > + return 1; What about the constraint that CR4.CET can't be set when CR0.WP is clear? (And the reverse needs to be handled in vmx_set_cr0). > + if (cet_on) > + vmcs_set_bits(VM_ENTRY_CONTROLS, > + VM_ENTRY_LOAD_GUEST_CET_STATE); Have we ensured that this VM-entry control is supported on the platform? > + else > + vmcs_clear_bits(VM_ENTRY_CONTROLS, > + VM_ENTRY_LOAD_GUEST_CET_STATE); > + return 0; > +} > + > int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) > { > struct vcpu_vmx *vmx = to_vmx(vcpu); > @@ -2958,6 +2990,9 @@ int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) > return 1; > } > > + if (set_cet_bit(vcpu, cr4)) > + return 1; > + > if (vmx->nested.vmxon && !nested_cr4_valid(vcpu, cr4)) > return 1; > > -- > 2.17.2 >
On Wed, Oct 02, 2019 at 11:54:26AM -0700, Jim Mattson wrote: > On Thu, Sep 26, 2019 at 7:17 PM Yang Weijiang <weijiang.yang@intel.com> wrote: > > > > "Load Guest CET state" bit controls whether Guest CET states > > will be loaded at Guest entry. Before doing that, KVM needs > > to check if CPU CET feature is enabled on host and available > > to Guest. > > > > Note: SHSTK and IBT features share one control MSR: > > MSR_IA32_{U,S}_CET, which means it's difficult to hide > > one feature from another in the case of SHSTK != IBT, > > after discussed in community, it's agreed to allow Guest > > control two features independently as it won't introduce > > security hole. > > > > Co-developed-by: Zhang Yi Z <yi.z.zhang@linux.intel.com> > > Signed-off-by: Zhang Yi Z <yi.z.zhang@linux.intel.com> > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > > --- > > arch/x86/kvm/vmx/vmx.c | 35 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 35 insertions(+) > > > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > > index f720baa7a9ba..ba1a83d11e69 100644 > > --- a/arch/x86/kvm/vmx/vmx.c > > +++ b/arch/x86/kvm/vmx/vmx.c > > @@ -44,6 +44,7 @@ > > #include <asm/spec-ctrl.h> > > #include <asm/virtext.h> > > #include <asm/vmx.h> > > +#include <asm/cet.h> > > > > #include "capabilities.h" > > #include "cpuid.h" > > @@ -2918,6 +2919,37 @@ void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) > > vmcs_writel(GUEST_CR3, guest_cr3); > > } > > > > +static int set_cet_bit(struct kvm_vcpu *vcpu, unsigned long cr4) > > Nit: This function does not appear to set CR4.CET, as the name would imply. > OK, will change it, thank you! > > +{ > > + struct vcpu_vmx *vmx = to_vmx(vcpu); > > + const u64 cet_bits = XFEATURE_MASK_CET_USER | XFEATURE_MASK_CET_KERNEL; > > + bool cet_xss = vmx_xsaves_supported() && > > + (kvm_supported_xss() & cet_bits); > > + bool cet_cpuid = guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) || > > + guest_cpuid_has(vcpu, X86_FEATURE_IBT); > > + bool cet_on = !!(cr4 & X86_CR4_CET); > > + > > + if (cet_on && vmx->nested.vmxon) > > + return 1; > > This constraint doesn't appear to be architected. Also, this prevents > enabling CR4.CET when in VMX operation, but what about the other way > around (i.e. entering VMX operation with CR4.CET enabled)? > Yes, will think more for nested case in next release. > > + if (cet_on && !cpu_x86_cet_enabled()) > > + return 1; > > This seems odd. Why is kernel support for (SHSTK or IBT) required for > the guest to use (SHSTK or IBT)? If there's a constraint, shouldn't it > be 1:1? (i.e. kernel support for SHSTK is required for the guest to > use SHSTK and kernel support for IBT is required for the guest to use > IBT?) Either way, enforcement of this constraint seems late, here, > when the guest is trying to set CR4 to a value that, per the guest > CPUID information, should be legal. Shouldn't this constraint be > applied when setting the guest CPUID information, disallowing the > enumeration of SHSTK and/or IBT support on a platform where these > features are unavailable or disabled in the kernel? > In KVM do_cpuid_7_mask(), SHSTK/IBT flags are emulated with cpuid(0x7,0), there in cpuid_mask(), it'll enforce the check against host kernel CET status, and host boot_cpu_data.x86_capability[X86_FEATURE_SHSTK/IBT] will be cleared during host boot up if the feature is disabled or unavailable. You may check the kernel CET patch. Rgarding the 1:1 check, I'll add more strict check in next version, thanks! > > + if (cet_on && !cet_xss) > > + return 1; > > Again, this constraint seems like it's being applied too late. > Returning 1 here will result in the guest's MOV-to-CR4 raising #GP, > even though there is no architected reason for it to do so. > see above. > > + if (cet_on && !cet_cpuid) > > + return 1; > > What about the constraint that CR4.CET can't be set when CR0.WP is > clear? (And the reverse needs to be handled in vmx_set_cr0). > OK, will check this case, thank you! > > + if (cet_on) > > + vmcs_set_bits(VM_ENTRY_CONTROLS, > > + VM_ENTRY_LOAD_GUEST_CET_STATE); > > Have we ensured that this VM-entry control is supported on the platform? > If all the checks pass, is it enought to ensure the control bit supported? > > + else > > + vmcs_clear_bits(VM_ENTRY_CONTROLS, > > + VM_ENTRY_LOAD_GUEST_CET_STATE); > > + return 0; > > +} > > + > > int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) > > { > > struct vcpu_vmx *vmx = to_vmx(vcpu); > > @@ -2958,6 +2990,9 @@ int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) > > return 1; > > } > > > > + if (set_cet_bit(vcpu, cr4)) > > + return 1; > > + > > if (vmx->nested.vmxon && !nested_cr4_valid(vcpu, cr4)) > > return 1; > > > > -- > > 2.17.2 > >
On Tue, Oct 8, 2019 at 11:41 PM Yang Weijiang <weijiang.yang@intel.com> wrote: > > On Wed, Oct 02, 2019 at 11:54:26AM -0700, Jim Mattson wrote: > > On Thu, Sep 26, 2019 at 7:17 PM Yang Weijiang <weijiang.yang@intel.com> wrote: > > > + if (cet_on) > > > + vmcs_set_bits(VM_ENTRY_CONTROLS, > > > + VM_ENTRY_LOAD_GUEST_CET_STATE); > > > > Have we ensured that this VM-entry control is supported on the platform? > > > If all the checks pass, is it enought to ensure the control bit supported? I don't think so. The only way to check to see if a VM-entry control is supported is to check the relevant VMX capability MSR. BTW, what about the corresponding VM-exit control?
On Wed, Oct 09, 2019 at 04:08:50PM -0700, Jim Mattson wrote: > On Tue, Oct 8, 2019 at 11:41 PM Yang Weijiang <weijiang.yang@intel.com> wrote: > > > > On Wed, Oct 02, 2019 at 11:54:26AM -0700, Jim Mattson wrote: > > > On Thu, Sep 26, 2019 at 7:17 PM Yang Weijiang <weijiang.yang@intel.com> wrote: > > > > + if (cet_on) > > > > + vmcs_set_bits(VM_ENTRY_CONTROLS, > > > > + VM_ENTRY_LOAD_GUEST_CET_STATE); > > > > > > Have we ensured that this VM-entry control is supported on the platform? > > > > > If all the checks pass, is it enought to ensure the control bit supported? > > I don't think so. The only way to check to see if a VM-entry control > is supported is to check the relevant VMX capability MSR. > It's a bit odd, there's no relevant CET bit in VMX cap. MSR, so I have to check like this. > BTW, what about the corresponding VM-exit control? The kernel supervisor mode CET is not implemented yet, so I don't load host CET states on VM-exit, in future, I'll add it.
On Wed, Oct 9, 2019 at 6:28 PM Yang Weijiang <weijiang.yang@intel.com> wrote: > > On Wed, Oct 09, 2019 at 04:08:50PM -0700, Jim Mattson wrote: > > On Tue, Oct 8, 2019 at 11:41 PM Yang Weijiang <weijiang.yang@intel.com> wrote: > > > > > > On Wed, Oct 02, 2019 at 11:54:26AM -0700, Jim Mattson wrote: > > > > On Thu, Sep 26, 2019 at 7:17 PM Yang Weijiang <weijiang.yang@intel.com> wrote: > > > > > + if (cet_on) > > > > > + vmcs_set_bits(VM_ENTRY_CONTROLS, > > > > > + VM_ENTRY_LOAD_GUEST_CET_STATE); > > > > > > > > Have we ensured that this VM-entry control is supported on the platform? > > > > > > > If all the checks pass, is it enought to ensure the control bit supported? > > > > I don't think so. The only way to check to see if a VM-entry control > > is supported is to check the relevant VMX capability MSR. > > > It's a bit odd, there's no relevant CET bit in VMX cap. MSR, so I have > to check like this. Bit 52 of the IA32_VMX_ENTRY_CTLS MSR (index 484H) [and bit 52 of the IA32_VMX_TRUE_ENTRY_CTLS MSR (index 490H), on hardware that supports the "true" VMX capability MSRs] will be 1 if it is legal to set bit 20 of the VM-entry controls field to 1. > > BTW, what about the corresponding VM-exit control? > The kernel supervisor mode CET is not implemented yet, so I don't load host CET > states on VM-exit, in future, I'll add it. If you don't clear the supervisor mode CET state on VM-exit and the guest has set IA32_S_CET.SH_STK_EN, doesn't that mean that supervisor-mode shadow stacks will then be enabled on the host after VM-exit?
On Thu, Oct 10, 2019 at 04:44:17PM -0700, Jim Mattson wrote: > On Wed, Oct 9, 2019 at 6:28 PM Yang Weijiang <weijiang.yang@intel.com> wrote: > > > > On Wed, Oct 09, 2019 at 04:08:50PM -0700, Jim Mattson wrote: > > > On Tue, Oct 8, 2019 at 11:41 PM Yang Weijiang <weijiang.yang@intel.com> wrote: > > > > > > > > On Wed, Oct 02, 2019 at 11:54:26AM -0700, Jim Mattson wrote: > > > > > On Thu, Sep 26, 2019 at 7:17 PM Yang Weijiang <weijiang.yang@intel.com> wrote: > > > > > > + if (cet_on) > > > > > > + vmcs_set_bits(VM_ENTRY_CONTROLS, > > > > > > + VM_ENTRY_LOAD_GUEST_CET_STATE); > > > > > > > > > > Have we ensured that this VM-entry control is supported on the platform? > > > > > > > > > If all the checks pass, is it enought to ensure the control bit supported? > > > > > > I don't think so. The only way to check to see if a VM-entry control > > > is supported is to check the relevant VMX capability MSR. > > > > > It's a bit odd, there's no relevant CET bit in VMX cap. MSR, so I have > > to check like this. > > Bit 52 of the IA32_VMX_ENTRY_CTLS MSR (index 484H) [and bit 52 of the > IA32_VMX_TRUE_ENTRY_CTLS MSR (index 490H), on hardware that supports > the "true" VMX capability MSRs] will be 1 if it is legal to set bit 20 > of the VM-entry controls field to 1. > Oh, you meant this, I'll add the check, thanks. > > > BTW, what about the corresponding VM-exit control? > > The kernel supervisor mode CET is not implemented yet, so I don't load host CET > > states on VM-exit, in future, I'll add it. > > If you don't clear the supervisor mode CET state on VM-exit and the > guest has set IA32_S_CET.SH_STK_EN, doesn't that mean that > supervisor-mode shadow stacks will then be enabled on the host after > VM-exit? Yeah, I should clear the MSR on VM-exit before supervisor-mode is implemented. Thank you! BTW, I'll move this bit-set before VM-entry, vmx_set_cr4() is not a suitable place to set the bit.
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index f720baa7a9ba..ba1a83d11e69 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -44,6 +44,7 @@ #include <asm/spec-ctrl.h> #include <asm/virtext.h> #include <asm/vmx.h> +#include <asm/cet.h> #include "capabilities.h" #include "cpuid.h" @@ -2918,6 +2919,37 @@ void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) vmcs_writel(GUEST_CR3, guest_cr3); } +static int set_cet_bit(struct kvm_vcpu *vcpu, unsigned long cr4) +{ + struct vcpu_vmx *vmx = to_vmx(vcpu); + const u64 cet_bits = XFEATURE_MASK_CET_USER | XFEATURE_MASK_CET_KERNEL; + bool cet_xss = vmx_xsaves_supported() && + (kvm_supported_xss() & cet_bits); + bool cet_cpuid = guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) || + guest_cpuid_has(vcpu, X86_FEATURE_IBT); + bool cet_on = !!(cr4 & X86_CR4_CET); + + if (cet_on && vmx->nested.vmxon) + return 1; + + if (cet_on && !cpu_x86_cet_enabled()) + return 1; + + if (cet_on && !cet_xss) + return 1; + + if (cet_on && !cet_cpuid) + return 1; + + if (cet_on) + vmcs_set_bits(VM_ENTRY_CONTROLS, + VM_ENTRY_LOAD_GUEST_CET_STATE); + else + vmcs_clear_bits(VM_ENTRY_CONTROLS, + VM_ENTRY_LOAD_GUEST_CET_STATE); + return 0; +} + int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) { struct vcpu_vmx *vmx = to_vmx(vcpu); @@ -2958,6 +2990,9 @@ int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) return 1; } + if (set_cet_bit(vcpu, cr4)) + return 1; + if (vmx->nested.vmxon && !nested_cr4_valid(vcpu, cr4)) return 1;
"Load Guest CET state" bit controls whether Guest CET states will be loaded at Guest entry. Before doing that, KVM needs to check if CPU CET feature is enabled on host and available to Guest. Note: SHSTK and IBT features share one control MSR: MSR_IA32_{U,S}_CET, which means it's difficult to hide one feature from another in the case of SHSTK != IBT, after discussed in community, it's agreed to allow Guest control two features independently as it won't introduce security hole. Co-developed-by: Zhang Yi Z <yi.z.zhang@linux.intel.com> Signed-off-by: Zhang Yi Z <yi.z.zhang@linux.intel.com> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> --- arch/x86/kvm/vmx/vmx.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)