Messages in this thread | | | Date | Wed, 28 Jun 2023 09:42:08 +0800 | Subject | Re: [PATCH v3 13/21] KVM:VMX: Emulate reads and writes to CET MSRs | From | "Yang, Weijiang" <> |
| |
On 6/27/2023 10:55 PM, Sean Christopherson wrote: > On Tue, Jun 27, 2023, Weijiang Yang wrote: >> On 6/27/2023 5:15 AM, Sean Christopherson wrote: >>> 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. > IBT can also used by the kernel... > > Just require that both CET_USER and CET_KERNEL are supported to advertise IBT > or SHSTK. I don't see why this is needs to be any more complex than that.
The arch control for user/kernel mode CET is the big source of complexity of the helpers.
Currently, CET_USER bit manages IA32_U_CET and IA32_PL3_SSP.
And CET_KERNEL bit manages PL{0,1,2}_SSP,
but architectural control/enable of IBT(user or kernel) is through IA32_{U,S}_CET, the former is
XSAVE-managed, but the latter is not.
Checking both before enable the features would make things much easier, but looks like
CET_KERNEL check for kernel IBT is excessive, just want to get your opinion on this. Thanks!
> >>> 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? > Why? The is_shadow_stack_msr() would still have to recheck X86_FEATURE_SHSTK, > so hoisting the checks to the top would be doing unnecessary work.
Yeah, just considered from readability perspective for the change, but it does introduce
unnecessary check. Will follow it.
> >>> return msr->host_initiated || >>> guest_cpuid_has(vcpu, X86_FEATURE_IBT) || >>> guest_cpuid_has(vcpu, X86_FEATURE_SHSTK); >>> }
| |