Control-flow Enforcement Technology (CET) provides protection against
return/jump-oriented programming (ROP) attacks. To make kvm Guest OS
own the capability, this patch-set is required. It enables CET related
CPUID report, xsaves/xrstors, vmx entry configuration etc for Guest OS.
PATCH 1/3/4: Enable CET support in VMCS.
PATCH 2 : Define CR4.CET master enable bit.
PATCH 5 : Enable xsave components for CET in XSS.
PATCH 6/7/8: Report CET feature support in CPUID.
Yang Weijiang (8):
kvm:vmx Introduce CET related VMCS field definitions.
kvm: Define CR4.CET[bit 23] (master enable bit) for guest OS.
kvm:vmx Enable loading CET state bit while guest CR4.CET is being
set.
kvm:vmx Pass through host CET related MSRs to Guest.
kvm:x86 Enable MSR_IA32_XSS bit 11 and 12 for CET xsaves/xrstors.
kvm:cpuid Add CPUID support for CET xsaves component query.
kvm:cpuid Fix xsaves area size calculation for CPUID.(EAX=0xD,ECX=1).
kvm:cpuid Report CET SHSTK and IBT support in CPUID.(EAX=0x7,ECX=0).
arch/x86/include/asm/kvm_host.h | 3 ++-
arch/x86/include/asm/vmx.h | 8 +++++++
arch/x86/kvm/cpuid.c | 23 +++++++++++++-------
arch/x86/kvm/vmx.c | 37 ++++++++++++++++++++++++++++++---
4 files changed, 60 insertions(+), 11 deletions(-)
--
2.17.1
This bit controls CET feature availability in guest OS.
Signed-off-by: Zhang Yi Z <[email protected]>
Signed-off-by: Yang Weijiang <[email protected]>
---
arch/x86/include/asm/kvm_host.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 55e51ff7e421..df002936088f 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -90,7 +90,8 @@
| X86_CR4_PGE | X86_CR4_PCE | X86_CR4_OSFXSR | X86_CR4_PCIDE \
| X86_CR4_OSXSAVE | X86_CR4_SMEP | X86_CR4_FSGSBASE \
| X86_CR4_OSXMMEXCPT | X86_CR4_LA57 | X86_CR4_VMXE \
- | X86_CR4_SMAP | X86_CR4_PKE | X86_CR4_UMIP))
+ | X86_CR4_SMAP | X86_CR4_PKE | X86_CR4_UMIP \
+ | X86_CR4_CET))
#define CR8_RESERVED_BITS (~(unsigned long)X86_CR8_TPR)
--
2.17.1
During Guest OS execution, it accesses these MSRs to
configure CET runtime settings.
Signed-off-by: Zhang Yi Z <[email protected]>
Signed-off-by: Yang Weijiang <[email protected]>
---
arch/x86/kvm/vmx.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 25fa6bd2fb95..fa2db6248404 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -11550,6 +11550,13 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_CS, MSR_TYPE_RW);
vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_ESP, MSR_TYPE_RW);
vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_EIP, MSR_TYPE_RW);
+
+ vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_U_CET, MSR_TYPE_RW);
+ vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_S_CET, MSR_TYPE_RW);
+ vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_INT_SSP_TAB, MSR_TYPE_RW);
+ vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_PL0_SSP, MSR_TYPE_RW);
+ vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_PL3_SSP, MSR_TYPE_RW);
+
vmx->msr_bitmap_mode = 0;
vmx->loaded_vmcs = &vmx->vmcs01;
--
2.17.1
According to latest Software Development Manual vol.2/3.2,
for CPUID.(EAX=0xD,ECX=1), it should report xsaves area size
containing all states enabled by XCR0|IA32_MSR_XSS.
Signed-off-by: Zhang Yi Z <[email protected]>
Signed-off-by: Yang Weijiang <[email protected]>
---
arch/x86/kvm/cpuid.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 5bac31e58955..55c282f71f93 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -121,7 +121,8 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
best = kvm_find_cpuid_entry(vcpu, 0xD, 1);
if (best && (best->eax & (F(XSAVES) | F(XSAVEC))))
- best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
+ best->ebx = xstate_required_size(vcpu->arch.xcr0 |
+ best->ecx | ((u64)best->edx << 32), true);
/*
* The existing code assumes virtual address is 48-bit or 57-bit in the
--
2.17.1
For kvm Guest OS, right now, only bit 11(user mode CET) and bit 12
(supervisor CET) are supported in XSS MSR, if other bits are being set,
the write to XSS will be skipped.
Signed-off-by: Zhang Yi Z <[email protected]>
Signed-off-by: Yang Weijiang <[email protected]>
---
arch/x86/kvm/vmx.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index fa2db6248404..5739ab393b90 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -47,6 +47,7 @@
#include <asm/virtext.h>
#include <asm/mce.h>
#include <asm/fpu/internal.h>
+#include <asm/fpu/types.h>
#include <asm/perf_event.h>
#include <asm/debugreg.h>
#include <asm/kexec.h>
@@ -4323,12 +4324,16 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
case MSR_IA32_XSS:
if (!vmx_xsaves_supported())
return 1;
+
/*
- * The only supported bit as of Skylake is bit 8, but
- * it is not supported on KVM.
+ * Right now, only support XSS_CET_U[bit 11] and
+ * XSS_CET_S[bit 12] in MSR_IA32_XSS.
*/
- if (data != 0)
+
+ if (data & ~(XFEATURE_MASK_SHSTK_USER
+ | XFEATURE_MASK_SHSTK_KERNEL))
return 1;
+
vcpu->arch.ia32_xss = data;
if (vcpu->arch.ia32_xss != host_xss)
add_atomic_switch_msr(vmx, MSR_IA32_XSS,
--
2.17.1
Guest OS can query CET SHSTK and IBT support by CPUID.(EAX=0x7,ECX=0),
in return, ECX[bit 7] corresponds to SHSTK feature, and EDX[bit 20]
corresponds to IBT feature.
Signed-off-by: Zhang Yi Z <[email protected]>
Signed-off-by: Yang Weijiang <[email protected]>
---
arch/x86/kvm/cpuid.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 55c282f71f93..d7b60b3c4326 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -67,6 +67,8 @@ u64 kvm_supported_xcr0(void)
#define F(x) bit(X86_FEATURE_##x)
+#define CET_IBT_BIT bit(20)
+
/* For scattered features from cpufeatures.h; we currently expose none */
#define KF(x) bit(KVM_CPUID_BIT_##x)
@@ -407,12 +409,12 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
F(AVX512VBMI) | F(LA57) | F(PKU) | 0 /*OSPKE*/ |
F(AVX512_VPOPCNTDQ) | F(UMIP) | F(AVX512_VBMI2) | F(GFNI) |
F(VAES) | F(VPCLMULQDQ) | F(AVX512_VNNI) | F(AVX512_BITALG) |
- F(CLDEMOTE);
+ F(CLDEMOTE) | F(SHSTK);
/* cpuid 7.0.edx*/
const u32 kvm_cpuid_7_0_edx_x86_features =
F(AVX512_4VNNIW) | F(AVX512_4FMAPS) | F(SPEC_CTRL) |
- F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES);
+ F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES) | CET_IBT_BIT;
/* all calls to cpuid_count() should be made on the same cpu */
get_cpu();
--
2.17.1
CET xsaves component size is queried through CPUID.(EAX=0xD, ECX=11)
and CPUID.(EAX=0xD, ECX=12).
Signed-off-by: Zhang Yi Z <[email protected]>
Signed-off-by: Yang Weijiang <[email protected]>
---
arch/x86/kvm/cpuid.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 7bcfa61375c0..5bac31e58955 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -565,6 +565,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
case 0xd: {
int idx, i;
u64 supported = kvm_supported_xcr0();
+ u64 sv_supported;
entry->eax &= supported;
entry->ebx = xstate_required_size(supported, false);
@@ -584,18 +585,23 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
entry[i].eax &= kvm_cpuid_D_1_eax_x86_features;
cpuid_mask(&entry[i].eax, CPUID_D_1_EAX);
entry[i].ebx = 0;
+ sv_supported = entry[i].ecx +
+ ((u64)entry[i].edx << 32);
if (entry[i].eax & (F(XSAVES)|F(XSAVEC)))
entry[i].ebx =
xstate_required_size(supported,
true);
- } else {
+ } else if (!(entry[i].ecx & 1)) {
if (entry[i].eax == 0 || !(supported & mask))
continue;
- if (WARN_ON_ONCE(entry[i].ecx & 1))
+ entry[i].ecx = 0;
+ entry[i].edx = 0;
+ } else {
+ if (entry[i].eax == 0 || !(sv_supported & mask))
continue;
+ entry[i].ecx = 1;
+ entry[i].edx = 0;
}
- entry[i].ecx = 0;
- entry[i].edx = 0;
entry[i].flags |=
KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
++*nent;
--
2.17.1
VMX relies on these fields to save/restore CET states for
guest and host. They are added here as VMCS placeholders
for the function.
Signed-off-by: Zhang Yi Z <[email protected]>
Signed-off-by: Yang Weijiang <[email protected]>
---
arch/x86/include/asm/vmx.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
index ade0f153947d..db745a1d49ae 100644
--- a/arch/x86/include/asm/vmx.h
+++ b/arch/x86/include/asm/vmx.h
@@ -98,6 +98,7 @@
#define VM_EXIT_LOAD_IA32_EFER 0x00200000
#define VM_EXIT_SAVE_VMX_PREEMPTION_TIMER 0x00400000
#define VM_EXIT_CLEAR_BNDCFGS 0x00800000
+#define VM_EXIT_LOAD_HOST_CET_STATE 0x10000000
#define VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR 0x00036dff
@@ -109,6 +110,7 @@
#define VM_ENTRY_LOAD_IA32_PAT 0x00004000
#define VM_ENTRY_LOAD_IA32_EFER 0x00008000
#define VM_ENTRY_LOAD_BNDCFGS 0x00010000
+#define VM_ENTRY_LOAD_GUEST_CET_STATE 0x00100000
#define VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR 0x000011ff
@@ -325,6 +327,9 @@ enum vmcs_field {
GUEST_PENDING_DBG_EXCEPTIONS = 0x00006822,
GUEST_SYSENTER_ESP = 0x00006824,
GUEST_SYSENTER_EIP = 0x00006826,
+ GUEST_IA32_S_CET = 0x00006828,
+ GUEST_SSP = 0x0000682a,
+ GUEST_INT_SSP_TABL = 0x0000682c,
HOST_CR0 = 0x00006c00,
HOST_CR3 = 0x00006c02,
HOST_CR4 = 0x00006c04,
@@ -337,6 +342,9 @@ enum vmcs_field {
HOST_IA32_SYSENTER_EIP = 0x00006c12,
HOST_RSP = 0x00006c14,
HOST_RIP = 0x00006c16,
+ HOST_IA32_S_CET = 0x00006c18,
+ HOST_SSP = 0x00006c1a,
+ HOST_INT_SSP_TABL = 0x00006c1c
};
/*
--
2.17.1
This bit controls whether guest CET states will be loaded on guest entry.
Signed-off-by: Zhang Yi Z <[email protected]>
Signed-off-by: Yang Weijiang <[email protected]>
---
arch/x86/kvm/vmx.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 7bbb8b26e901..25fa6bd2fb95 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -1045,6 +1045,8 @@ struct vcpu_vmx {
bool req_immediate_exit;
+ bool vcpu_cet_on;
+
/* Support for PML */
#define PML_ENTITY_NUM 512
struct page *pml_pg;
@@ -5409,6 +5411,23 @@ static int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
return 1;
}
+ /*
+ * When CET.CR4 is being set, it means we're enabling CET for
+ * the guest, then enable loading CET state bit in entry control.
+ * Otherwise, clear loading CET bit to disable guest CET.
+ */
+ if (cr4 & X86_CR4_CET) {
+ if (!to_vmx(vcpu)->vcpu_cet_on) {
+ vmcs_set_bits(VM_ENTRY_CONTROLS,
+ VM_ENTRY_LOAD_GUEST_CET_STATE);
+ to_vmx(vcpu)->vcpu_cet_on = 1;
+ }
+ } else if (to_vmx(vcpu)->vcpu_cet_on) {
+ vmcs_clear_bits(VM_ENTRY_CONTROLS,
+ VM_ENTRY_LOAD_GUEST_CET_STATE);
+ to_vmx(vcpu)->vcpu_cet_on = 0;
+ }
+
if (to_vmx(vcpu)->nested.vmxon && !nested_cr4_valid(vcpu, cr4))
return 1;
--
2.17.1
> On 26 Dec 2018, at 10:15, Yang Weijiang <[email protected]> wrote:
>
> This bit controls whether guest CET states will be loaded on guest entry.
>
> Signed-off-by: Zhang Yi Z <[email protected]>
> Signed-off-by: Yang Weijiang <[email protected]>
> ---
> arch/x86/kvm/vmx.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 7bbb8b26e901..25fa6bd2fb95 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -1045,6 +1045,8 @@ struct vcpu_vmx {
>
> bool req_immediate_exit;
>
> + bool vcpu_cet_on;
> +
> /* Support for PML */
> #define PML_ENTITY_NUM 512
> struct page *pml_pg;
> @@ -5409,6 +5411,23 @@ static int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
> return 1;
> }
>
> + /*
> + * When CET.CR4 is being set, it means we're enabling CET for
You probably meant to write here CR4.CET.
> + * the guest, then enable loading CET state bit in entry control.
> + * Otherwise, clear loading CET bit to disable guest CET.
> + */
> + if (cr4 & X86_CR4_CET) {
> + if (!to_vmx(vcpu)->vcpu_cet_on) {
> + vmcs_set_bits(VM_ENTRY_CONTROLS,
> + VM_ENTRY_LOAD_GUEST_CET_STATE);
> + to_vmx(vcpu)->vcpu_cet_on = 1;
> + }
> + } else if (to_vmx(vcpu)->vcpu_cet_on) {
> + vmcs_clear_bits(VM_ENTRY_CONTROLS,
> + VM_ENTRY_LOAD_GUEST_CET_STATE);
> + to_vmx(vcpu)->vcpu_cet_on = 0;
> + }
> +
> if (to_vmx(vcpu)->nested.vmxon && !nested_cr4_valid(vcpu, cr4))
> return 1;
>
> --
> 2.17.1
>
I haven’t seen a patch in the series that modifies kvm_set_cr4() to verify CR4.CET is not set when CET is not reported as supported by CPUID.
I think that is missing from the series.
-Liran
On Wed, Dec 26, 2018 at 04:52:38PM +0800, Liran Alon wrote:
Thanks Liran for pointing out the issues!
I'll fix them in next version.
>
>
> > On 26 Dec 2018, at 10:15, Yang Weijiang <[email protected]> wrote:
> >
> > This bit controls whether guest CET states will be loaded on guest entry.
> >
> > Signed-off-by: Zhang Yi Z <[email protected]>
> > Signed-off-by: Yang Weijiang <[email protected]>
> > ---
> > arch/x86/kvm/vmx.c | 19 +++++++++++++++++++
> > 1 file changed, 19 insertions(+)
> >
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index 7bbb8b26e901..25fa6bd2fb95 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -1045,6 +1045,8 @@ struct vcpu_vmx {
> >
> > bool req_immediate_exit;
> >
> > + bool vcpu_cet_on;
> > +
> > /* Support for PML */
> > #define PML_ENTITY_NUM 512
> > struct page *pml_pg;
> > @@ -5409,6 +5411,23 @@ static int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
> > return 1;
> > }
> >
> > + /*
> > + * When CET.CR4 is being set, it means we're enabling CET for
>
> You probably meant to write here CR4.CET.
>
> > + * the guest, then enable loading CET state bit in entry control.
> > + * Otherwise, clear loading CET bit to disable guest CET.
> > + */
> > + if (cr4 & X86_CR4_CET) {
> > + if (!to_vmx(vcpu)->vcpu_cet_on) {
> > + vmcs_set_bits(VM_ENTRY_CONTROLS,
> > + VM_ENTRY_LOAD_GUEST_CET_STATE);
> > + to_vmx(vcpu)->vcpu_cet_on = 1;
> > + }
> > + } else if (to_vmx(vcpu)->vcpu_cet_on) {
> > + vmcs_clear_bits(VM_ENTRY_CONTROLS,
> > + VM_ENTRY_LOAD_GUEST_CET_STATE);
> > + to_vmx(vcpu)->vcpu_cet_on = 0;
> > + }
> > +
> > if (to_vmx(vcpu)->nested.vmxon && !nested_cr4_valid(vcpu, cr4))
> > return 1;
> >
> > --
> > 2.17.1
> >
>
> I haven’t seen a patch in the series that modifies kvm_set_cr4() to verify CR4.CET is not set when CET is not reported as supported by CPUID.
> I think that is missing from the series.
>
> -Liran
>
>
On Wed, Dec 26, 2018 at 04:15:25PM +0800, Yang Weijiang wrote:
> VMX relies on these fields to save/restore CET states for
> guest and host. They are added here as VMCS placeholders
> for the function.
The changelog needs a lot more detail on what is saved where and when,
e.g. hardware unconditionally saves the guest MSRs to the VMCS fields
on exit, hence there is no VM_EXIT_SAVE_GUEST_CET_STATE control bit.
It's also worth calling out that the other CET MSRs are saved/loaded
via XSAVES/XRSTORS.
>
> Signed-off-by: Zhang Yi Z <[email protected]>
> Signed-off-by: Yang Weijiang <[email protected]>
> ---
> arch/x86/include/asm/vmx.h | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
> index ade0f153947d..db745a1d49ae 100644
> --- a/arch/x86/include/asm/vmx.h
> +++ b/arch/x86/include/asm/vmx.h
> @@ -98,6 +98,7 @@
> #define VM_EXIT_LOAD_IA32_EFER 0x00200000
> #define VM_EXIT_SAVE_VMX_PREEMPTION_TIMER 0x00400000
> #define VM_EXIT_CLEAR_BNDCFGS 0x00800000
> +#define VM_EXIT_LOAD_HOST_CET_STATE 0x10000000
>
> #define VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR 0x00036dff
>
> @@ -109,6 +110,7 @@
> #define VM_ENTRY_LOAD_IA32_PAT 0x00004000
> #define VM_ENTRY_LOAD_IA32_EFER 0x00008000
> #define VM_ENTRY_LOAD_BNDCFGS 0x00010000
> +#define VM_ENTRY_LOAD_GUEST_CET_STATE 0x00100000
>
> #define VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR 0x000011ff
>
> @@ -325,6 +327,9 @@ enum vmcs_field {
> GUEST_PENDING_DBG_EXCEPTIONS = 0x00006822,
> GUEST_SYSENTER_ESP = 0x00006824,
> GUEST_SYSENTER_EIP = 0x00006826,
> + GUEST_IA32_S_CET = 0x00006828,
> + GUEST_SSP = 0x0000682a,
> + GUEST_INT_SSP_TABL = 0x0000682c,
Dropping a single letter from "TABLE" seems pointless, "INTERRUPT" is
usually "INTR", and the docs I've seen list the name as
IA32_INTERRUPT_SSP_TABLE_ADDR. So maybe?
{GUEST,HOST}_INTR_SSP_TABLE_ADDR
Also, the SSP and INTERRUPT_SSP_TABL_ADDR fields are 64-bit fields, we
we should define the _HIGH variations.
> HOST_CR0 = 0x00006c00,
> HOST_CR3 = 0x00006c02,
> HOST_CR4 = 0x00006c04,
> @@ -337,6 +342,9 @@ enum vmcs_field {
> HOST_IA32_SYSENTER_EIP = 0x00006c12,
> HOST_RSP = 0x00006c14,
> HOST_RIP = 0x00006c16,
> + HOST_IA32_S_CET = 0x00006c18,
> + HOST_SSP = 0x00006c1a,
> + HOST_INT_SSP_TABL = 0x00006c1c
> };
>
> /*
> --
> 2.17.1
>
On Wed, Dec 26, 2018 at 04:15:28PM +0800, Yang Weijiang wrote:
> During Guest OS execution, it accesses these MSRs to
> configure CET runtime settings.
The changelog needs to explain why it's ok to expose these MSRS to the
guest, e.g. call out which are saved/loaded through the VMCS and which
are saved/loaded via XSAVES/XRSTORS. And maybe expand "runtime settings"
to elaborate on when most guest kernels will access the MSRs, i.e. to
justify exposing them to the guest.
It should also clarify why IA32_PL1_SSP and IA32_PL2_SSP are NOT exposed
to the guest.
>
> Signed-off-by: Zhang Yi Z <[email protected]>
> Signed-off-by: Yang Weijiang <[email protected]>
> ---
> arch/x86/kvm/vmx.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 25fa6bd2fb95..fa2db6248404 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -11550,6 +11550,13 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
> vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_CS, MSR_TYPE_RW);
> vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_ESP, MSR_TYPE_RW);
> vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_EIP, MSR_TYPE_RW);
> +
> + vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_U_CET, MSR_TYPE_RW);
> + vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_S_CET, MSR_TYPE_RW);
> + vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_INT_SSP_TAB, MSR_TYPE_RW);
> + vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_PL0_SSP, MSR_TYPE_RW);
> + vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_PL3_SSP, MSR_TYPE_RW);
> +
> vmx->msr_bitmap_mode = 0;
>
> vmx->loaded_vmcs = &vmx->vmcs01;
> --
> 2.17.1
>
On Wed, Dec 26, 2018 at 04:15:29PM +0800, Yang Weijiang wrote:
> For kvm Guest OS, right now, only bit 11(user mode CET) and bit 12
> (supervisor CET) are supported in XSS MSR, if other bits are being set,
> the write to XSS will be skipped.
>
> Signed-off-by: Zhang Yi Z <[email protected]>
> Signed-off-by: Yang Weijiang <[email protected]>
> ---
> arch/x86/kvm/vmx.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index fa2db6248404..5739ab393b90 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -47,6 +47,7 @@
> #include <asm/virtext.h>
> #include <asm/mce.h>
> #include <asm/fpu/internal.h>
> +#include <asm/fpu/types.h>
> #include <asm/perf_event.h>
> #include <asm/debugreg.h>
> #include <asm/kexec.h>
> @@ -4323,12 +4324,16 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> case MSR_IA32_XSS:
> if (!vmx_xsaves_supported())
> return 1;
> +
> /*
> - * The only supported bit as of Skylake is bit 8, but
> - * it is not supported on KVM.
> + * Right now, only support XSS_CET_U[bit 11] and
> + * XSS_CET_S[bit 12] in MSR_IA32_XSS.
> */
> - if (data != 0)
> +
> + if (data & ~(XFEATURE_MASK_SHSTK_USER
> + | XFEATURE_MASK_SHSTK_KERNEL))
New lines are usually after the operator, e.g.:
if (data & ~(XFEATURE_MASK_SHSTK_USER |
XFEATURE_MASK_SHSTK_KERNEL))
And doesn't this flow need to check that the bits are actually supported?
> return 1;
> +
> vcpu->arch.ia32_xss = data;
> if (vcpu->arch.ia32_xss != host_xss)
> add_atomic_switch_msr(vmx, MSR_IA32_XSS,
> --
> 2.17.1
>
On Wed, Dec 26, 2018 at 04:15:30PM +0800, Yang Weijiang wrote:
> CET xsaves component size is queried through CPUID.(EAX=0xD, ECX=11)
> and CPUID.(EAX=0xD, ECX=12).
>
> Signed-off-by: Zhang Yi Z <[email protected]>
> Signed-off-by: Yang Weijiang <[email protected]>
> ---
> arch/x86/kvm/cpuid.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 7bcfa61375c0..5bac31e58955 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -565,6 +565,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
> case 0xd: {
> int idx, i;
> u64 supported = kvm_supported_xcr0();
> + u64 sv_supported;
What about u_supported and s_supported? sv_supported doesn't make me
think "supervisor"e.
> entry->eax &= supported;
> entry->ebx = xstate_required_size(supported, false);
> @@ -584,18 +585,23 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
> entry[i].eax &= kvm_cpuid_D_1_eax_x86_features;
> cpuid_mask(&entry[i].eax, CPUID_D_1_EAX);
> entry[i].ebx = 0;
> + sv_supported = entry[i].ecx +
> + ((u64)entry[i].edx << 32);
Use '|' instead of '+' to smush ECX and EDX together, as is it looks
like the code is calculating a size or something.
> if (entry[i].eax & (F(XSAVES)|F(XSAVEC)))
> entry[i].ebx =
> xstate_required_size(supported,
> true);
> - } else {
> + } else if (!(entry[i].ecx & 1)) {
Now that we're actually consuming bit 0, it'd be nice to formally define
it as referring to supervisor state.
What about styling the code like this? Might make it more obvious that
the logic for user vs. supervisor is identical, i.e. ECX bit 0 only
determines which mask is applied.
if (idx == 1) {
...
} else {
supported = (entry[i].ecx & 1) ? s_supported :
u_supported;
if (entry[i].eax == 0 || !(supported & mask))
continue;
entry[i].ecx &= 1;
entry[i].edx = 0;
}
> if (entry[i].eax == 0 || !(supported & mask))
> continue;
> - if (WARN_ON_ONCE(entry[i].ecx & 1))
> + entry[i].ecx = 0;
> + entry[i].edx = 0;
> + } else {
> + if (entry[i].eax == 0 || !(sv_supported & mask))
> continue;
> + entry[i].ecx = 1;
> + entry[i].edx = 0;
> }
> - entry[i].ecx = 0;
> - entry[i].edx = 0;
Removing this entirely isn't correct for CPUID.0xD.0x1, KVM should still
zero out the bits it doesn't handle, e.g. KVM will signal a fault if the
guest attempts to set any bits in IA32_XSS other than the CET bits.
> entry[i].flags |=
> KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
> ++*nent;
> --
> 2.17.1
>
On Wed, Dec 26, 2018 at 12:13 AM Yang Weijiang <[email protected]> wrote:
>
> During Guest OS execution, it accesses these MSRs to
> configure CET runtime settings.
>
> Signed-off-by: Zhang Yi Z <[email protected]>
> Signed-off-by: Yang Weijiang <[email protected]>
> ---
> arch/x86/kvm/vmx.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 25fa6bd2fb95..fa2db6248404 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -11550,6 +11550,13 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
> vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_CS, MSR_TYPE_RW);
> vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_ESP, MSR_TYPE_RW);
> vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_SYSENTER_EIP, MSR_TYPE_RW);
> +
> + vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_U_CET, MSR_TYPE_RW);
> + vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_S_CET, MSR_TYPE_RW);
> + vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_INT_SSP_TAB, MSR_TYPE_RW);
> + vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_PL0_SSP, MSR_TYPE_RW);
> + vmx_disable_intercept_for_msr(msr_bitmap, MSR_IA32_PL3_SSP, MSR_TYPE_RW);
> +
Shouldn't this be conditional on the guest having CET support
enumerated in CPUID?
> vmx->msr_bitmap_mode = 0;
>
> vmx->loaded_vmcs = &vmx->vmcs01;
> --
> 2.17.1
>
On Wed, Dec 26, 2018 at 04:15:31PM +0800, Yang Weijiang wrote:
> According to latest Software Development Manual vol.2/3.2,
> for CPUID.(EAX=0xD,ECX=1), it should report xsaves area size
> containing all states enabled by XCR0|IA32_MSR_XSS.
>
> Signed-off-by: Zhang Yi Z <[email protected]>
> Signed-off-by: Yang Weijiang <[email protected]>
> ---
> arch/x86/kvm/cpuid.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 5bac31e58955..55c282f71f93 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -121,7 +121,8 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
>
> best = kvm_find_cpuid_entry(vcpu, 0xD, 1);
> if (best && (best->eax & (F(XSAVES) | F(XSAVEC))))
> - best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
> + best->ebx = xstate_required_size(vcpu->arch.xcr0 |
> + best->ecx | ((u64)best->edx << 32), true);
Passing the CPUID bits is wrong, it needs to be the guest's IA32_MSR_XSS.
The CPUID bits enumerate what is legal to enable, not what features are
actually enabled by software.
>
> /*
> * The existing code assumes virtual address is 48-bit or 57-bit in the
> --
> 2.17.1
>
On Wed, Dec 26, 2018 at 04:15:32PM +0800, Yang Weijiang wrote:
> Guest OS can query CET SHSTK and IBT support by CPUID.(EAX=0x7,ECX=0),
> in return, ECX[bit 7] corresponds to SHSTK feature, and EDX[bit 20]
> corresponds to IBT feature.
>
> Signed-off-by: Zhang Yi Z <[email protected]>
> Signed-off-by: Yang Weijiang <[email protected]>
> ---
> arch/x86/kvm/cpuid.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 55c282f71f93..d7b60b3c4326 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -67,6 +67,8 @@ u64 kvm_supported_xcr0(void)
>
> #define F(x) bit(X86_FEATURE_##x)
>
> +#define CET_IBT_BIT bit(20)
Properly define X86_FEATURE_CET_IBT if it's not being defined by some
other series, even if KVM is the only immediate user of the bit. Linux
reserves all of word 18 for CPUID_7_0_EDX, i.e. defining the bit won't
bloat some random scattered word.
> +
> /* For scattered features from cpufeatures.h; we currently expose none */
> #define KF(x) bit(KVM_CPUID_BIT_##x)
>
> @@ -407,12 +409,12 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
> F(AVX512VBMI) | F(LA57) | F(PKU) | 0 /*OSPKE*/ |
> F(AVX512_VPOPCNTDQ) | F(UMIP) | F(AVX512_VBMI2) | F(GFNI) |
> F(VAES) | F(VPCLMULQDQ) | F(AVX512_VNNI) | F(AVX512_BITALG) |
> - F(CLDEMOTE);
> + F(CLDEMOTE) | F(SHSTK);
>
> /* cpuid 7.0.edx*/
> const u32 kvm_cpuid_7_0_edx_x86_features =
> F(AVX512_4VNNIW) | F(AVX512_4FMAPS) | F(SPEC_CTRL) |
> - F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES);
> + F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES) | CET_IBT_BIT;
>
> /* all calls to cpuid_count() should be made on the same cpu */
> get_cpu();
> --
> 2.17.1
>
On Wed, Jan 2, 2019 at 10:24 AM Sean Christopherson
<[email protected]> wrote:
>
> On Wed, Dec 26, 2018 at 04:15:29PM +0800, Yang Weijiang wrote:
> > For kvm Guest OS, right now, only bit 11(user mode CET) and bit 12
> > (supervisor CET) are supported in XSS MSR, if other bits are being set,
> > the write to XSS will be skipped.
> >
> > Signed-off-by: Zhang Yi Z <[email protected]>
> > Signed-off-by: Yang Weijiang <[email protected]>
> > ---
> > arch/x86/kvm/vmx.c | 11 ++++++++---
> > 1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index fa2db6248404..5739ab393b90 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -47,6 +47,7 @@
> > #include <asm/virtext.h>
> > #include <asm/mce.h>
> > #include <asm/fpu/internal.h>
> > +#include <asm/fpu/types.h>
> > #include <asm/perf_event.h>
> > #include <asm/debugreg.h>
> > #include <asm/kexec.h>
> > @@ -4323,12 +4324,16 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > case MSR_IA32_XSS:
> > if (!vmx_xsaves_supported())
> > return 1;
> > +
> > /*
> > - * The only supported bit as of Skylake is bit 8, but
> > - * it is not supported on KVM.
> > + * Right now, only support XSS_CET_U[bit 11] and
> > + * XSS_CET_S[bit 12] in MSR_IA32_XSS.
> > */
> > - if (data != 0)
> > +
> > + if (data & ~(XFEATURE_MASK_SHSTK_USER
> > + | XFEATURE_MASK_SHSTK_KERNEL))
>
> New lines are usually after the operator, e.g.:
>
> if (data & ~(XFEATURE_MASK_SHSTK_USER |
> XFEATURE_MASK_SHSTK_KERNEL))
>
> And doesn't this flow need to check that the bits are actually supported?
Supported on the host and in the guest.
On Wed, Jan 02, 2019 at 11:19:34AM -0800, Jim Mattson wrote:
Thanks Sean and Jim for your valuable comments, I'll consolidate them in
next release!
> On Wed, Jan 2, 2019 at 10:24 AM Sean Christopherson
> <[email protected]> wrote:
> >
> > On Wed, Dec 26, 2018 at 04:15:29PM +0800, Yang Weijiang wrote:
> > > For kvm Guest OS, right now, only bit 11(user mode CET) and bit 12
> > > (supervisor CET) are supported in XSS MSR, if other bits are being set,
> > > the write to XSS will be skipped.
> > >
> > > Signed-off-by: Zhang Yi Z <[email protected]>
> > > Signed-off-by: Yang Weijiang <[email protected]>
> > > ---
> > > arch/x86/kvm/vmx.c | 11 ++++++++---
> > > 1 file changed, 8 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > > index fa2db6248404..5739ab393b90 100644
> > > --- a/arch/x86/kvm/vmx.c
> > > +++ b/arch/x86/kvm/vmx.c
> > > @@ -47,6 +47,7 @@
> > > #include <asm/virtext.h>
> > > #include <asm/mce.h>
> > > #include <asm/fpu/internal.h>
> > > +#include <asm/fpu/types.h>
> > > #include <asm/perf_event.h>
> > > #include <asm/debugreg.h>
> > > #include <asm/kexec.h>
> > > @@ -4323,12 +4324,16 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > > case MSR_IA32_XSS:
> > > if (!vmx_xsaves_supported())
> > > return 1;
> > > +
> > > /*
> > > - * The only supported bit as of Skylake is bit 8, but
> > > - * it is not supported on KVM.
> > > + * Right now, only support XSS_CET_U[bit 11] and
> > > + * XSS_CET_S[bit 12] in MSR_IA32_XSS.
> > > */
> > > - if (data != 0)
> > > +
> > > + if (data & ~(XFEATURE_MASK_SHSTK_USER
> > > + | XFEATURE_MASK_SHSTK_KERNEL))
> >
> > New lines are usually after the operator, e.g.:
> >
> > if (data & ~(XFEATURE_MASK_SHSTK_USER |
> > XFEATURE_MASK_SHSTK_KERNEL))
> >
> > And doesn't this flow need to check that the bits are actually supported?
> Supported on the host and in the guest.
On 26/12/18 09:15, Yang Weijiang wrote:
> Guest OS can query CET SHSTK and IBT support by CPUID.(EAX=0x7,ECX=0),
> in return, ECX[bit 7] corresponds to SHSTK feature, and EDX[bit 20]
> corresponds to IBT feature.
>
> Signed-off-by: Zhang Yi Z <[email protected]>
> Signed-off-by: Yang Weijiang <[email protected]>
> ---
> arch/x86/kvm/cpuid.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 55c282f71f93..d7b60b3c4326 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -67,6 +67,8 @@ u64 kvm_supported_xcr0(void)
>
> #define F(x) bit(X86_FEATURE_##x)
>
> +#define CET_IBT_BIT bit(20)
> +
> /* For scattered features from cpufeatures.h; we currently expose none */
> #define KF(x) bit(KVM_CPUID_BIT_##x)
>
> @@ -407,12 +409,12 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
> F(AVX512VBMI) | F(LA57) | F(PKU) | 0 /*OSPKE*/ |
> F(AVX512_VPOPCNTDQ) | F(UMIP) | F(AVX512_VBMI2) | F(GFNI) |
> F(VAES) | F(VPCLMULQDQ) | F(AVX512_VNNI) | F(AVX512_BITALG) |
> - F(CLDEMOTE);
> + F(CLDEMOTE) | F(SHSTK);
>
> /* cpuid 7.0.edx*/
> const u32 kvm_cpuid_7_0_edx_x86_features =
> F(AVX512_4VNNIW) | F(AVX512_4FMAPS) | F(SPEC_CTRL) |
> - F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES);
> + F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES) | CET_IBT_BIT;
>
> /* all calls to cpuid_count() should be made on the same cpu */
> get_cpu();
>
This might change soon, but at least for now KVM's XSAVES/XRSTORS usage
depends on Linux's. Therefore, this should only be enabled if Linux has
set bits 11 and 12 in MSR_IA32_XSS.
Paolo