Re: [PATCH v3 13/21] KVM:VMX: Emulate reads and writes to CET MSRs
From: Yang, Weijiang
Date: Mon Jun 26 2023 - 23:40:12 EST
On 6/27/2023 5:15 AM, Sean Christopherson wrote:
On Mon, Jun 26, 2023, Weijiang Yang wrote:
On 6/24/2023 7:53 AM, Sean Christopherson wrote:
On Thu, May 11, 2023, Yang Weijiang wrote:
Side topic, what on earth does the SDM mean by this?!?
The linear address written must be aligned to 8 bytes and bits 2:0 must be 0
(hardware requires bits 1:0 to be 0).
I know Intel retroactively changed the alignment requirements, but the above
is nonsensical. If ucode prevents writing bits 2:0, who cares what hardware
requires?
So do I ;-/
Can you follow-up with someone to get clarification? If writing bit 2 with '1'
does not #GP despite the statement that it "must be aligned", then KVM shouldn't
injected a #GP on that case.
OK, will consult someone and get back to this thread.
+ return 1;
+ kvm_set_xsave_msr(msr_info);
+ break;
case MSR_IA32_PERF_CAPABILITIES:
if (data && !vcpu_to_pmu(vcpu)->version)
return 1;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b6eec9143129..2e3a39c9297c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -13630,6 +13630,26 @@ int kvm_sev_es_string_io(struct kvm_vcpu *vcpu, unsigned int size,
}
EXPORT_SYMBOL_GPL(kvm_sev_es_string_io);
+bool kvm_cet_is_msr_accessible(struct kvm_vcpu *vcpu, struct msr_data *msr)
+{
+ if (!kvm_cet_user_supported())
This feels wrong. KVM should differentiate between SHSTK and IBT in the host.
E.g. if running in a VM with SHSTK but not IBT, or vice versa, KVM should allow
writes to non-existent MSRs.
I don't follow you, in this case, which part KVM is on behalf of? guest or
user space?
Sorry, typo. KVM *shouldn't* allow writes to non-existent MSRs.
I.e. this looks wrong:
/*
* If SHSTK and IBT are available in KVM, clear CET user bit in
* kvm_caps.supported_xss so that kvm_cet_user_supported() returns
* false when called.
*/
if (!kvm_cpu_cap_has(X86_FEATURE_SHSTK) &&
!kvm_cpu_cap_has(X86_FEATURE_IBT))
kvm_caps.supported_xss &= ~XFEATURE_MASK_CET_USER;
The comment is wrong, it should be "are not available in KVM". My intent is,�
if both features are not available in KVM, then clear the precondition bit so
that all dependent checks will fail quickly.
Checking kvm_caps.supported_xss.CET_USER is worthless in 99% of the cases though.
Unless I'm missing something, the only time it's useful is for CR4.CET, which
doesn't differentiate between SHSTK and IBT. For everything else that KVM cares
about, at some point KVM needs to precisely check for SHSTK and IBT support
anyways
I will tweak the patches and do precise checks based on the available
features to guest.
and by extension, all dependent code is also wrong. IIRC, there's a virtualization
hole, but I don't see any reason why KVM has to make the hole even bigger.
Do you mean the issue that both SHSTK and IBT share one control MSR? i.e.,
U_CET/S_CET?
I mean that passing through PLx_SSP if the host has IBT but *not* SHSTK is wrong.
Understood.
+ return false;
+
+ if (msr->host_initiated)
+ return true;
+
+ if (!guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) &&
+ !guest_cpuid_has(vcpu, X86_FEATURE_IBT))
+ return false;
+
+ if (msr->index == MSR_IA32_PL3_SSP &&
+ !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK))
I probably asked this long ago, but if I did I since forgot. Is it really just
PL3_SSP that depends on SHSTK? I would expect all shadow stack MSRs to depend
on SHSTK.
All PL{0,1,2,3}_SSP plus INT_SSP_TAB msr depend on SHSTK. In patch 21, I
added more MSRs in this helper.
Sure, except that patch 21 never adds handling for PL{0,1,2}_SSP. I see:
if (!kvm_cet_user_supported() &&
!(kvm_cpu_cap_has(X86_FEATURE_IBT) ||
kvm_cpu_cap_has(X86_FEATURE_SHSTK)))
return false;
if (msr->host_initiated)
return true;
if (!guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) &&
!guest_cpuid_has(vcpu, X86_FEATURE_IBT))
return false;
/* The synthetic MSR is for userspace access only. */
if (msr->index == MSR_KVM_GUEST_SSP)
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) ||
kvm_cet_kernel_shstk_supported();
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;
mask = (msr->index == MSR_IA32_PL3_SSP) ? XFEATURE_MASK_CET_USER :
XFEATURE_MASK_CET_KERNEL;
return !!(kvm_caps.supported_xss & mask);
Which means that KVM will allow guest accesses to PL{0,1,2}_SSP regardless of
whether or not X86_FEATURE_SHSTK is enumerated to the guest.
Hmm, the check of X86_FEATURE_SHSTK is missing in this case.
And the above is also wrong for host_initiated writes to SHSTK MSRs. E.g. if KVM
is running on a CPU that has IBT but not SHSTK, then userspace can write to MSRs
that do not exist.
Maybe this confusion is just a symptom of the series not providing proper
Supervisor Shadow Stack support, but that's still a poor excuse for posting
broken code.
I suspect you tried to get too fancy. I don't see any reason to ever care about
kvm_caps.supported_xss beyond emulating writes to XSS itself. Just require that
both CET_USER and CET_KERNEL are supported in XSS to allow IBT or SHSTK, i.e. let
X86_FEATURE_IBT and X86_FEATURE_SHSTK speak for themselves. That way, this can
simply be:
You're right, kvm_cet_user_supported() is overused.
Let me recap to see if I understand correctly:
1. Check both CET_USER and CET_KERNEL are supported in XSS before
advertise SHSTK is supported
in KVM and expose it to guest, the reason is once SHSTK is exposed to
guest, KVM should support both
modes to honor arch integrity.
2. Check CET_USER is supported before advertise IBT is supported in KVM
and expose IBT, the reason is,
user IBT(MSR_U_CET) depends on CET_USER bit while kernel IBT(MSR_S_CET)
doesn't.
bool kvm_cet_is_msr_accessible(struct kvm_vcpu *vcpu, struct msr_data *msr)
{
if (is_shadow_stack_msr(...))
if (!kvm_cpu_cap_has(X86_FEATURE_SHSTK))
return false;
return msr->host_initiated ||
guest_cpuid_has(vcpu, X86_FEATURE_SHSTK);
}
if (!kvm_cpu_cap_has(X86_FEATURE_IBT) &&
!kvm_cpu_cap_has(X86_FEATURE_SHSTK))
return false;
Move above checks to the beginning?
return msr->host_initiated ||
guest_cpuid_has(vcpu, X86_FEATURE_IBT) ||
guest_cpuid_has(vcpu, X86_FEATURE_SHSTK);
}
+ * and reload the guest fpu states before read/write xsaves-managed MSRs.
+ */
+static inline void kvm_get_xsave_msr(struct msr_data *msr_info)
+{
+ fpregs_lock_and_load();
KVM already has helpers that do exactly this, and they have far better names for
KVM: kvm_fpu_get() and kvm_fpu_put(). Can you convert kvm_fpu_get() to
fpregs_lock_and_load() and use those isntead? And if the extra consistency checks
in fpregs_lock_and_load() fire, we definitely want to know, as it means we probably
have bugs in KVM.
Do you want me to do some experiments to make sure the WARN()� in
fpregs_lock_and load() would be triggered or not?
Yes, though I shouldn't have to clarify that. The well-documented (as of now)
expectation is that any code that someone posts is tested, unless explicitly
stated otherwise. I.e. you should not have to ask if you should verify the WARN
doesn't trigger, because you should be doing that for all code you post.
Surely I will do tests based on the change.
If no WARN() trigger, then replace fpregs_lock_and_load()/fpregs_unlock()
with kvm_fpu_get()/
kvm_fpu_put()?
Yes.