linux-kernel - Re: [PATCH v5 08/19] KVM:x86: Report KVM supported CET MSRs as to-be-saved

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87788afa-8dec-a54a-c3f9-4b35fbcb03ae@intel.com>
Date:   Sun, 6 Aug 2023 16:54:43 +0800
From:   "Yang, Weijiang" <weijiang.yang@...el.com>
To:     Chao Gao <chao.gao@...el.com>
CC:     <seanjc@...gle.com>, <pbonzini@...hat.com>, <peterz@...radead.org>,
        <john.allen@....com>, <kvm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <rick.p.edgecombe@...el.com>,
        <binbin.wu@...ux.intel.com>
Subject: Re: [PATCH v5 08/19] KVM:x86: Report KVM supported CET MSRs as
 to-be-saved

On 8/4/2023 1:51 PM, Chao Gao wrote:
> On Fri, Aug 04, 2023 at 11:13:36AM +0800, Yang, Weijiang wrote:
>>>> @@ -7214,6 +7217,13 @@ static void kvm_probe_msr_to_save(u32 msr_index)
>>>> 		if (!kvm_caps.supported_xss)
>>>> 			return;
>>>> 		break;
>>>> +	case MSR_IA32_U_CET:
>>>> +	case MSR_IA32_S_CET:
>>>> +	case MSR_KVM_GUEST_SSP:
>>>> +	case MSR_IA32_PL0_SSP ... MSR_IA32_INT_SSP_TAB:
>>>> +		if (!kvm_is_cet_supported())
>>> shall we consider the case where IBT is supported while SS isn't
>>> (e.g., in L1 guest)?
>> Yes, but userspace should be able to access SHSTK MSRs even only IBT is exposed to guest so
>> far as KVM can support SHSTK MSRs.
> Why should userspace be allowed to access SHSTK MSRs in this case? L1 may not
> even enumerate SHSTK (qemu removes -shstk explicitly but keeps IBT), how KVM in
> L1 can allow its userspace to do that?
Hold on until host_initiated access is finalized.
>>>> +static inline bool kvm_is_cet_supported(void)
>>>> +{
>>>> +	return (kvm_caps.supported_xss & CET_XSTATE_MASK) == CET_XSTATE_MASK;
>>> why not just check if SHSTK or IBT is supported explicitly, i.e.,
>>>
>>> 	return kvm_cpu_cap_has(X86_FEATURE_SHSTK) ||
>>> 	       kvm_cpu_cap_has(X86_FEATURE_IBT);
>>>
>>> this is straightforward. And strictly speaking, the support of a feature and
>>> the support of managing a feature's state via XSAVE(S) are two different things.x
>> I think using exiting check implies two things:
>> 1. Platform/KVM can support CET features.
>> 2. CET user mode MSRs are backed by host thus are guaranteed to be valid.
>> i.e., the purpose is to check guest CET dependencies instead of features' availability.
> When KVM claims a feature is supported, it should ensure all its dependencies are
> met. that's, KVM's support of a feature also imples all dependencies are met.
> Function-wise, the two approaches have no difference. I just think checking
> KVM's support of SHSTK/IBT is more clear because the function name is
> kvm_is_cet_supported() rather than e.g., kvm_is_cet_state_managed_by_xsave().
OK, maybe the helper is not necessary anymore, I will remove it, thank you!
>> kvm_cpu_cap_has(X86_FEATURE_SHSTK) || kvm_cpu_cap_has(X86_FEATURE_IBT)
>>
>> only tells at least one of the CET features is supported by KVM.
>>
>>> then patch 16 has no need to do
>>>
>>> +	/*
>>> +	 * If SHSTK and IBT are not available in KVM, clear CET user bit in
>>> +	 * kvm_caps.supported_xss so that kvm_is_cet__supported() returns
>>> +	 * false when called.
>>> +	 */
>>> +	if (!kvm_cpu_cap_has(X86_FEATURE_SHSTK) &&
>>> +	    !kvm_cpu_cap_has(X86_FEATURE_IBT))
>>> +		kvm_caps.supported_xss &= ~CET_XSTATE_MASK;