linux-kernel - Re: [PATCH v4 09/20] KVM:x86: Add common code of CET MSR access

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, 26 Jul 2023 16:26:06 +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 v4 09/20] KVM:x86: Add common code of CET MSR access


On 7/26/2023 3:33 PM, Chao Gao wrote:
> On Thu, Jul 20, 2023 at 11:03:41PM -0400, Yang Weijiang wrote:
>> +static inline bool is_shadow_stack_msr(struct kvm_vcpu *vcpu,
> remove @vcpu since it isn't used. And I think it is better to accept
> an MSR index than struct msr_data because whether a MSR is a shadow
> stack MSR is entirely decided by the MSR index; other fields in the
> struct msr_data are irrelevant.

Yes, I should have removed it, thanks!

>
>> +				       struct msr_data *msr)
>> +{
>> +	return msr->index == MSR_IA32_PL0_SSP ||
>> +		msr->index == MSR_IA32_PL1_SSP ||
>> +		msr->index == MSR_IA32_PL2_SSP ||
>> +		msr->index == MSR_IA32_PL3_SSP ||
>> +		msr->index == MSR_IA32_INT_SSP_TAB ||
>> +		msr->index == MSR_KVM_GUEST_SSP;
>> +}
>> +
>> +static bool kvm_cet_is_msr_accessible(struct kvm_vcpu *vcpu,
>> +				      struct msr_data *msr)
>> +{
>> +
>> +	/*
>> +	 * This function cannot work without later CET MSR read/write
>> +	 * emulation patch.
> Probably you should consider merging the "later" patch into this one.
> Then you can get rid of this comment and make this patch easier for
> review ...

Which later patch you mean? If you mean [13/20] KVM:VMX: Emulate read 
and write to CET MSRs,

then I intentionally separate these two, this one is for CET MSR common 
checks and operations,

the latter is specific to VMX, and add the above comments in case 
someone is bisecting

the patches and happens to split at this patch, then it would faulted 
and take some actions.

>> int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>> {
>> 	u32 msr = msr_info->index;
>> @@ -3982,6 +4023,35 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>> 		vcpu->arch.guest_fpu.xfd_err = data;
>> 		break;
>> #endif
>> +#define CET_IBT_MASK_BITS	GENMASK_ULL(63, 2)
> bit9:6 are reserved even if IBT is supported.

Yes, as IBT is only available on Intel platforms, I move the handling of 
bit 9:6 to VMX  related patch.

Here's the common check in case IBT is not available.

>
>> @@ -12131,6 +12217,7 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>>
>> 	vcpu->arch.cr3 = 0;
>> 	kvm_register_mark_dirty(vcpu, VCPU_EXREG_CR3);
>> +	memset(vcpu->arch.cet_s_ssp, 0, sizeof(vcpu->arch.cet_s_ssp));
> ... this begs the question: where other MSRs are reset. I suppose
> U_CET/PL3_SSP are handled when resetting guest FPU. But how about S_CET
> and INT_SSP_TAB? there is no answer in this patch.

I think the related guest VMCS fields(S_CET/INT_SSP_TAB/SSP) should be 
reset to 0 in vmx_vcpu_reset(),

do you think so?