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.
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?
+ struct msr_data msr;
+
+ msr.index = MSR_KVM_GUEST_SSP;
+ msr.host_initiated = true;
Huh?
+ /* GUEST_SSP is stored in VMCS at vm-exit. */
(a) this is not VMX code, i.e. referencing the VMCS is wrong, and (b) how the
guest's SSP is managed is irrelevant, all that matters is that KVM can get the
current guest value.
Sorry the comment is incorrect, my original intent is: it's stored in
VM control structure field, will change it.
+ static_call(kvm_x86_get_msr)(vcpu, &msr);
+ smram->ssp = msr.data;
+ }
}
#endif
@@ -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.
