Re: [PATCH v3 16/21] KVM:x86: Save/Restore GUEST_SSP to/from SMM state save area
From: Yang, Weijiang
Date: Mon Jun 26 2023 - 23:51:20 EST
On 6/27/2023 5:20 AM, Sean Christopherson wrote:
On Mon, Jun 26, 2023, Weijiang Yang wrote:
On 6/24/2023 6:30 AM, Sean Christopherson wrote:
On Thu, May 11, 2023, Yang Weijiang wrote:
Save GUEST_SSP to SMM state save area when guest exits to SMM
due to SMI and restore it VMCS field when guest exits SMM.
This fails to answer "Why does KVM need to do this?"
How about this:
Guest SMM mode execution is out of guest kernel, to avoid GUEST_SSP
corruption,
KVM needs to save current normal mode GUEST_SSP to SMRAM area so that it can
restore original GUEST_SSP at the end of SMM.
The key point I am looking for is a call out that KVM is emulating architectural
behavior, i.e. that smram->ssp is defined in the SDM and that the documented
behavior of Intel CPUs is that the CPU's current SSP is saved on SMI and loaded
on RSM. And I specifically say "loaded" and not "restored", because the field
is writable.
OK, will enclose these ideas.
Signed-off-by: Yang Weijiang <weijiang.yang@xxxxxxxxx>
---
arch/x86/kvm/smm.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/arch/x86/kvm/smm.c b/arch/x86/kvm/smm.c
index b42111a24cc2..c54d3eb2b7e4 100644
--- a/arch/x86/kvm/smm.c
+++ b/arch/x86/kvm/smm.c
@@ -275,6 +275,16 @@ static void enter_smm_save_state_64(struct kvm_vcpu *vcpu,
enter_smm_save_seg_64(vcpu, &smram->gs, VCPU_SREG_GS);
smram->int_shadow = static_call(kvm_x86_get_interrupt_shadow)(vcpu);
+
+ if (kvm_cet_user_supported()) {
This is wrong, KVM should not save/restore state that doesn't exist from the guest's
perspective, i.e. this needs to check guest_cpuid_has().
Yes, the check missed the case that user space disables SHSTK. Will change
it, thanks!
On a related topic, I would love feedback on my series that adds a framework for
features like this, where KVM needs to check guest CPUID as well as host support.
https://lore.kernel.org/all/20230217231022.816138-1-seanjc@xxxxxxxxxx
The framework looks good, will it be merged in kvm_x86?
Yes, I would like to merge it at some point.
@@ -565,6 +575,16 @@ static int rsm_load_state_64(struct x86_emulate_ctxt *ctxt,
static_call(kvm_x86_set_interrupt_shadow)(vcpu, 0);
ctxt->interruptibility = (u8)smstate->int_shadow;
+ if (kvm_cet_user_supported()) {
+ struct msr_data msr;
+
+ msr.index = MSR_KVM_GUEST_SSP;
+ msr.host_initiated = true;
+ msr.data = smstate->ssp;
+ /* Mimic host_initiated access to bypass ssp access check. */
No, masquerading as a host access is all kinds of wrong. I have no idea what
check you're trying to bypass, but whatever it is, it's wrong. Per the SDM, the
SSP field in SMRAM is writable, which means that KVM needs to correctly handle
the scenario where SSP holds garbage, e.g. a non-canonical address.
MSR_KVM_GUEST_SSP is only accessible to user space, e.g., during LM, it's not
accessible to VM itself. So in kvm_cet_is_msr_accessible(), I added a check to
tell whether the access is initiated from user space or not, I tried to bypass
that check. Yes, I will add necessary checks here.
Why can't this use kvm_get_msr() and kvm_set_msr()?
If my above assumption is correct, these helpers are passed by
host_initiated=false and cannot meet the requirments.
Sorry, I don't follow. These writes are NOT initiated from the host, i.e.
kvm_get_msr() and kvm_set_msr() do the right thing, unless I'm missing something.
In this series, in patch 14, I added below check:
+/* The synthetic MSR is for userspace access only. */
+if (msr->index == MSR_KVM_GUEST_SSP)
+return false;
If kvm_get_msr() or kvm_set_msr() is used(host_initiated=false),
it'll hit this check and fail to write the MSR.
But there's anther check at the beginning of kvm_cet_is_msr_accessible():
+if (msr->host_initiated)
+return true;
I thought to use the host_initiated = true to bypass the former check.
Now the helper is going to be overhauled then this is not an issue.