| Message ID | 20230511040857.6094-17-weijiang.yang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable CET Virtualization | expand |
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?" > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > --- > 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(). 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@google.com > + 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. > + 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. Why can't this use kvm_get_msr() and kvm_set_msr()?
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@intel.com> >> --- >> 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@google.com 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.
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. > > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > > > --- > > > 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@google.com > > 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.
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@intel.com> >>>> --- >>>> 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@google.com >> 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.
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()) { + struct msr_data msr; + + msr.index = MSR_KVM_GUEST_SSP; + msr.host_initiated = true; + /* GUEST_SSP is stored in VMCS at vm-exit. */ + 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. */ + static_call(kvm_x86_set_msr)(vcpu, &msr); + } + return X86EMUL_CONTINUE; } #endif
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. Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> --- arch/x86/kvm/smm.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)