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]

Date:   Tue, 8 Aug 2023 23:16:36 +0800
From:   "Yang, Weijiang" <weijiang.yang@...el.com>
To:     Sean Christopherson <seanjc@...gle.com>,
        Chao Gao <chao.gao@...el.com>
CC:     <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/5/2023 2:51 AM, Sean Christopherson wrote:
> On Fri, Aug 04, 2023, 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?
> +1.  And specifically, this isn't about SHSTK being exposed to the guest, it's about
> SHSTK being _supported by KVM_.  This is all about KVM telling userspace what MSRs
> are valid and/or need to be saved+restored.  If KVM doesn't support a feature,
> then the MSRs are invalid and there is no reason for userspace to save+restore
> the MSRs on live migration.
OK, will use kvm_cpu_cap_has() to check KVM support before add CET MSRs to the lists.
>>>>> +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().
> +1, one of the big reasons kvm_cpu_cap_has() came about was being KVM had a giant
> mess of one-off helpers.
I see, thanks!