Re: [PATCH v3 13/21] KVM:VMX: Emulate reads and writes to CET MSRs

Yang, Weijiang
2023-06-28T01:42:43Z

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);
>>> }