diff mbox series

[v2,21/21] KVM:x86: Support CET supervisor shadow stack MSR access

Message ID 20230421134615.62539-22-weijiang.yang@intel.com
State New
Headers
Series Enable CET Virtualization |

Commit Message

Yang, Weijiang April 21, 2023, 1:46 p.m. UTC 
  Add MSR access interfaces for supervisor shadow stack, i.e.,
MSR_IA32_PL{0,1,2} and MSR_IA32_INT_SSP_TAB, meanwhile pass through
them to {L1,L2} guests when {L0,L1} KVM supports supervisor shadow
stack.

Note, currently supervisor shadow stack is not supported on Intel
platforms, i.e., VMX always clears CPUID(EAX=07H,ECX=1).EDX.[bit 18].

The main purpose of this patch is to facilitate AMD folks to enable
supervisor shadow stack for their platforms.

Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
---
 arch/x86/kvm/cpuid.h      |  6 +++++
 arch/x86/kvm/vmx/nested.c | 12 +++++++++
 arch/x86/kvm/vmx/vmx.c    | 51 ++++++++++++++++++++++++++++++++++-----
 3 files changed, 63 insertions(+), 6 deletions(-)

Comments

Edgecombe, Rick P May 3, 2023, 5:06 p.m. UTC | #1 
On Fri, 2023-04-21 at 09:46 -0400, Yang Weijiang wrote:
> @@ -2471,6 +2495,12 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu,
> struct msr_data *msr_info)
>                 else
>                         kvm_set_xsave_msr(msr_info);
>                 break;
> +       case MSR_IA32_PL0_SSP ... MSR_IA32_PL2_SSP:
> +       case MSR_IA32_INT_SSP_TAB:
> +               if (!cet_is_msr_accessible(vcpu, msr_info))
> +                       return 1;
> +               kvm_set_xsave_msr(msr_info);
> +               break;

These are supposed to be canonical too, right?
Edgecombe, Rick P May 3, 2023, 5:07 p.m. UTC | #2 
On Fri, 2023-04-21 at 09:46 -0400, Yang Weijiang wrote:
> +
> +       incpt = !is_cet_state_supported(vcpu,
> XFEATURE_MASK_CET_KERNEL);
> +       incpt |= !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK);
> +
> +       vmx_set_intercept_for_msr(vcpu, MSR_IA32_INT_SSP_TAB,
> MSR_TYPE_RW, incpt);
> +       vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL0_SSP,
> MSR_TYPE_RW, incpt);
> +       vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL1_SSP,
> MSR_TYPE_RW, incpt);
> +       vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL2_SSP,
> MSR_TYPE_RW, incpt);
>  }

Why is this tied to XFEATURE_MASK_CET_KERNEL? I don't know how the SVM
side works, but the host kernel doesn't use this xfeature. Just not
clear on what the intention is. Why not use
kvm_cet_kernel_shstk_supported() again?
Yang, Weijiang May 4, 2023, 1:11 a.m. UTC | #3 
On 5/4/2023 1:06 AM, Edgecombe, Rick P wrote:
> On Fri, 2023-04-21 at 09:46 -0400, Yang Weijiang wrote:
>> @@ -2471,6 +2495,12 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu,
>> struct msr_data *msr_info)
>>                  else
>>                          kvm_set_xsave_msr(msr_info);
>>                  break;
>> +       case MSR_IA32_PL0_SSP ... MSR_IA32_PL2_SSP:
>> +       case MSR_IA32_INT_SSP_TAB:
>> +               if (!cet_is_msr_accessible(vcpu, msr_info))
>> +                       return 1;
>> +               kvm_set_xsave_msr(msr_info);
>> +               break;
> These are supposed to be canonical too, right?

Yes, I'll add check in next version, thanks!
Yang, Weijiang May 4, 2023, 1:20 a.m. UTC | #4 
On 5/4/2023 1:07 AM, Edgecombe, Rick P wrote:
> On Fri, 2023-04-21 at 09:46 -0400, Yang Weijiang wrote:
>> +
>> +       incpt = !is_cet_state_supported(vcpu,
>> XFEATURE_MASK_CET_KERNEL);
>> +       incpt |= !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK);
>> +
>> +       vmx_set_intercept_for_msr(vcpu, MSR_IA32_INT_SSP_TAB,
>> MSR_TYPE_RW, incpt);
>> +       vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL0_SSP,
>> MSR_TYPE_RW, incpt);
>> +       vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL1_SSP,
>> MSR_TYPE_RW, incpt);
>> +       vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL2_SSP,
>> MSR_TYPE_RW, incpt);
>>   }
> Why is this tied to XFEATURE_MASK_CET_KERNEL? I don't know how the SVM
> side works, but the host kernel doesn't use this xfeature. Just not
> clear on what the intention is. Why not use
> kvm_cet_kernel_shstk_supported() again?

I don't know how SVM supports supervisor SHSTK either, here just follows 
the spec.

to add the dependency check. Maybe you're right, I need to use 
kvm_cet_kernel_shstk_supported()

in my patch set and leave the work to SVM enabling patches. I'll change 
it, thanks!
Edgecombe, Rick P May 4, 2023, 4:17 a.m. UTC | #5 
On Thu, 2023-05-04 at 09:20 +0800, Yang, Weijiang wrote:
> 
> On 5/4/2023 1:07 AM, Edgecombe, Rick P wrote:
> > On Fri, 2023-04-21 at 09:46 -0400, Yang Weijiang wrote:
> > > +
> > > +       incpt = !is_cet_state_supported(vcpu,
> > > XFEATURE_MASK_CET_KERNEL);
> > > +       incpt |= !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK);
> > > +
> > > +       vmx_set_intercept_for_msr(vcpu, MSR_IA32_INT_SSP_TAB,
> > > MSR_TYPE_RW, incpt);
> > > +       vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL0_SSP,
> > > MSR_TYPE_RW, incpt);
> > > +       vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL1_SSP,
> > > MSR_TYPE_RW, incpt);
> > > +       vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL2_SSP,
> > > MSR_TYPE_RW, incpt);
> > >    }
> > Why is this tied to XFEATURE_MASK_CET_KERNEL? I don't know how the
> > SVM
> > side works, but the host kernel doesn't use this xfeature. Just not
> > clear on what the intention is. Why not use
> > kvm_cet_kernel_shstk_supported() again?
> 
> I don't know how SVM supports supervisor SHSTK either, here just
> follows 
> the spec.

What aspect of the spec is this?

> 
> to add the dependency check. Maybe you're right, I need to use 
> kvm_cet_kernel_shstk_supported()
> 
> in my patch set and leave the work to SVM enabling patches. I'll
> change 
> it, thanks!

Oh, I see the the SVM patch [0] is adding XFEATURE_MASK_CET_KERNEL to
kvm_caps.supported_xss as long as kvm_cpu_cap_has(X86_FEATURE_SHSTK).
And it does not look to be checking XSS host support like how 
kvm_caps.supported_xss is set in your patch. It should depend on host
support, right? Is that the intent of kvm_caps.supported_xss?

Separate from all that, the code above is in VMX, so not sure how it
affects SVM in any case.

I might be confused here. The code just looked suspicious.

[0]
https://lore.kernel.org/kvm/20221012203910.204793-8-john.allen@amd.com/
Yang, Weijiang May 4, 2023, 6:51 a.m. UTC | #6 
On 5/4/2023 12:17 PM, Edgecombe, Rick P wrote:
> On Thu, 2023-05-04 at 09:20 +0800, Yang, Weijiang wrote:
>> On 5/4/2023 1:07 AM, Edgecombe, Rick P wrote:
>>> On Fri, 2023-04-21 at 09:46 -0400, Yang Weijiang wrote:
>>>> +
>>>> +       incpt = !is_cet_state_supported(vcpu,
>>>> XFEATURE_MASK_CET_KERNEL);
>>>> +       incpt |= !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK);
>>>> +
>>>> +       vmx_set_intercept_for_msr(vcpu, MSR_IA32_INT_SSP_TAB,
>>>> MSR_TYPE_RW, incpt);
>>>> +       vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL0_SSP,
>>>> MSR_TYPE_RW, incpt);
>>>> +       vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL1_SSP,
>>>> MSR_TYPE_RW, incpt);
>>>> +       vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL2_SSP,
>>>> MSR_TYPE_RW, incpt);
>>>>     }
>>> Why is this tied to XFEATURE_MASK_CET_KERNEL? I don't know how the
>>> SVM
>>> side works, but the host kernel doesn't use this xfeature. Just not
>>> clear on what the intention is. Why not use
>>> kvm_cet_kernel_shstk_supported() again?
>> I don't know how SVM supports supervisor SHSTK either, here just
>> follows
>> the spec.
> What aspect of the spec is this?

I assumed the supervisor SHSTK states are backed via XSAVES/SRSTORS with

XFEATURE_MASK_CET_KERNEL set in XSS.  This is arguable since implementation

is not determined, but XSAVES is an efficient way to manage the states 
compared with

manually save/restore the MSRs.

>
>> to add the dependency check. Maybe you're right, I need to use
>> kvm_cet_kernel_shstk_supported()
>>
>> in my patch set and leave the work to SVM enabling patches. I'll
>> change
>> it, thanks!
> Oh, I see the the SVM patch [0] is adding XFEATURE_MASK_CET_KERNEL to
> kvm_caps.supported_xss as long as kvm_cpu_cap_has(X86_FEATURE_SHSTK).
> And it does not look to be checking XSS host support like how
> kvm_caps.supported_xss is set in your patch. It should depend on host
> support, right?

Yes, it should rely on host to back the states as long as the supervisor

SHSTK MSRs are implemented as XSAVES/XRSTORS managed.

> Is that the intent of kvm_caps.supported_xss?

Yes, it's used to indicate all host XSS supported guest features.

>
> Separate from all that, the code above is in VMX, so not sure how it
> affects SVM in any case.

I was confused a bit. Yes, the pass-through check is specific to VMX, 
there could

be other implementation in SVM.

>
> I might be confused here. The code just looked suspicious.
>
> [0]
> https://lore.kernel.org/kvm/20221012203910.204793-8-john.allen@amd.com/

IMO, above patch is not necessary as  kvm_caps.supported_xss is 
initialized in x86 part and

shared by both SVM and VMX.
diff mbox series

Patch

diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index b1658c0de847..019a16b25b88 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -232,4 +232,10 @@  static __always_inline bool guest_pv_has(struct kvm_vcpu *vcpu,
 	return vcpu->arch.pv_cpuid.features & (1u << kvm_feature);
 }
 
+static __always_inline bool kvm_cet_kernel_shstk_supported(void)
+{
+	return !IS_ENABLED(CONFIG_KVM_INTEL) &&
+	       kvm_cpu_cap_has(X86_FEATURE_SHSTK);
+}
+
 #endif
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index bf690827bfee..aaaae92dc9f6 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -670,6 +670,18 @@  static inline bool nested_vmx_prepare_msr_bitmap(struct kvm_vcpu *vcpu,
 	nested_vmx_set_intercept_for_msr(vmx, msr_bitmap_l1, msr_bitmap_l0,
 					 MSR_IA32_PL3_SSP, MSR_TYPE_RW);
 
+	nested_vmx_set_intercept_for_msr(vmx, msr_bitmap_l1, msr_bitmap_l0,
+					 MSR_IA32_PL0_SSP, MSR_TYPE_RW);
+
+	nested_vmx_set_intercept_for_msr(vmx, msr_bitmap_l1, msr_bitmap_l0,
+					 MSR_IA32_PL1_SSP, MSR_TYPE_RW);
+
+	nested_vmx_set_intercept_for_msr(vmx, msr_bitmap_l1, msr_bitmap_l0,
+					 MSR_IA32_PL2_SSP, MSR_TYPE_RW);
+
+	nested_vmx_set_intercept_for_msr(vmx, msr_bitmap_l1, msr_bitmap_l0,
+					 MSR_IA32_INT_SSP_TAB, MSR_TYPE_RW);
+
 	kvm_vcpu_unmap(vcpu, &vmx->nested.msr_bitmap_map, false);
 
 	vmx->nested.force_msr_bitmap_recalc = false;
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 6eab3e452bbb..074b618f1a07 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -713,6 +713,9 @@  static bool is_valid_passthrough_msr(u32 msr)
 	case MSR_IA32_PL3_SSP:
 	case MSR_IA32_S_CET:
 		return true;
+	case MSR_IA32_PL0_SSP ... MSR_IA32_PL2_SSP:
+	case MSR_IA32_INT_SSP_TAB:
+		return true;
 	}
 
 	r = possible_passthrough_msr_slot(msr) != -ENOENT;
@@ -1962,8 +1965,11 @@  static int vmx_get_msr_feature(struct kvm_msr_entry *msr)
 static bool cet_is_msr_accessible(struct kvm_vcpu *vcpu,
 				  struct msr_data *msr)
 {
+	u64 mask;
+
 	if (!kvm_cet_user_supported() &&
-	    !kvm_cpu_cap_has(X86_FEATURE_IBT))
+	    !(kvm_cpu_cap_has(X86_FEATURE_IBT) ||
+	      kvm_cpu_cap_has(X86_FEATURE_SHSTK)))
 		return false;
 
 	if (msr->host_initiated)
@@ -1973,15 +1979,27 @@  static bool cet_is_msr_accessible(struct kvm_vcpu *vcpu,
 	    !guest_cpuid_has(vcpu, X86_FEATURE_IBT))
 		return false;
 
+	if (msr->index == MSR_IA32_U_CET)
+		return true;
+
 	if (msr->index == MSR_IA32_S_CET)
-		return guest_cpuid_has(vcpu, X86_FEATURE_IBT);
+		return guest_cpuid_has(vcpu, X86_FEATURE_IBT) ||
+		       kvm_cet_kernel_shstk_supported();
 
-	if ((msr->index == MSR_IA32_PL3_SSP ||
-	     msr->index == MSR_KVM_GUEST_SSP) &&
+	if (msr->index == MSR_KVM_GUEST_SSP)
+		return guest_cpuid_has(vcpu, X86_FEATURE_SHSTK);
+
+	if (msr->index == MSR_IA32_INT_SSP_TAB)
+		return guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) &&
+		       kvm_cet_kernel_shstk_supported();
+
+	if (msr->index == MSR_IA32_PL3_SSP &&
 	    !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK))
 		return false;
 
-	return true;
+	mask = (msr->index == MSR_IA32_PL3_SSP) ? XFEATURE_MASK_CET_USER :
+						  XFEATURE_MASK_CET_KERNEL;
+	return !!(kvm_caps.supported_xss & mask);
 }
 
 /*
@@ -2135,6 +2153,12 @@  static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		else
 			kvm_get_xsave_msr(msr_info);
 		break;
+	case MSR_IA32_PL0_SSP ... MSR_IA32_PL2_SSP:
+	case MSR_IA32_INT_SSP_TAB:
+		if (!cet_is_msr_accessible(vcpu, msr_info))
+			return 1;
+		kvm_get_xsave_msr(msr_info);
+		break;
 	case MSR_IA32_DEBUGCTLMSR:
 		msr_info->data = vmcs_read64(GUEST_IA32_DEBUGCTL);
 		break;
@@ -2471,6 +2495,12 @@  static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		else
 			kvm_set_xsave_msr(msr_info);
 		break;
+	case MSR_IA32_PL0_SSP ... MSR_IA32_PL2_SSP:
+	case MSR_IA32_INT_SSP_TAB:
+		if (!cet_is_msr_accessible(vcpu, msr_info))
+			return 1;
+		kvm_set_xsave_msr(msr_info);
+		break;
 	case MSR_IA32_PERF_CAPABILITIES:
 		if (data && !vcpu_to_pmu(vcpu)->version)
 			return 1;
@@ -7774,6 +7804,14 @@  static void vmx_update_intercept_for_cet_msr(struct kvm_vcpu *vcpu)
 
 	incpt |= !guest_cpuid_has(vcpu, X86_FEATURE_IBT);
 	vmx_set_intercept_for_msr(vcpu, MSR_IA32_S_CET, MSR_TYPE_RW, incpt);
+
+	incpt = !is_cet_state_supported(vcpu, XFEATURE_MASK_CET_KERNEL);
+	incpt |= !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK);
+
+	vmx_set_intercept_for_msr(vcpu, MSR_IA32_INT_SSP_TAB, MSR_TYPE_RW, incpt);
+	vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL0_SSP, MSR_TYPE_RW, incpt);
+	vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL1_SSP, MSR_TYPE_RW, incpt);
+	vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL2_SSP, MSR_TYPE_RW, incpt);
 }
 
 static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
@@ -7844,7 +7882,8 @@  static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 	/* Refresh #PF interception to account for MAXPHYADDR changes. */
 	vmx_update_exception_bitmap(vcpu);
 
-	if (kvm_cet_user_supported() || kvm_cpu_cap_has(X86_FEATURE_IBT))
+	if (kvm_cet_user_supported() || kvm_cpu_cap_has(X86_FEATURE_IBT) ||
+	    kvm_cpu_cap_has(X86_FEATURE_SHSTK))
 		vmx_update_intercept_for_cet_msr(vcpu);
 }