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

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:   Wed, 28 Jun 2023 09:42:08 +0800
From:   "Yang, Weijiang" <weijiang.yang@...el.com>
To:     Sean Christopherson <seanjc@...gle.com>
CC:     <pbonzini@...hat.com>, <kvm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <peterz@...radead.org>,
        <rppt@...nel.org>, <binbin.wu@...ux.intel.com>,
        <rick.p.edgecombe@...el.com>, <john.allen@....com>,
        Sean Christopherson <sean.j.christopherson@...el.com>
Subject: Re: [PATCH v3 13/21] KVM:VMX: Emulate reads and writes to CET MSRs


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