linux-kernel - Re: [PATCH v3 07/21] KVM:x86: Refresh CPUID on write to guest MSR_IA32

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: <23169e5f-9793-9486-fd5f-287e3317b837@intel.com>
Date:   Tue, 30 May 2023 11:51:22 +0800
From:   "Yang, Weijiang" <weijiang.yang@...el.com>
To:     Chao Gao <chao.gao@...el.com>
CC:     <seanjc@...gle.com>, <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>,
        Zhang Yi Z <yi.z.zhang@...ux.intel.com>
Subject: Re: [PATCH v3 07/21] KVM:x86: Refresh CPUID on write to guest
 MSR_IA32_XSS


On 5/25/2023 2:10 PM, Chao Gao wrote:
> On Thu, May 11, 2023 at 12:08:43AM -0400, Yang Weijiang wrote:
>> Update CPUID(EAX=0DH,ECX=1) when the guest's XSS is modified.
>> CPUID(EAX=0DH,ECX=1).EBX reports current required storage size for all
>> features enabled via XCR0 | XSS so that guest can allocate correct xsave
>> buffer.
>>
>> Note, KVM does not yet support any XSS based features, i.e. supported_xss
>> is guaranteed to be zero at this time.
>>
>> Co-developed-by: Zhang Yi Z <yi.z.zhang@...ux.intel.com>
>> Signed-off-by: Zhang Yi Z <yi.z.zhang@...ux.intel.com>
>> Signed-off-by: Yang Weijiang <weijiang.yang@...el.com>
>> ---
>> arch/x86/kvm/cpuid.c | 7 +++++--
>> arch/x86/kvm/x86.c   | 6 ++++--
>> 2 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index 123bf8b97a4b..cbb1b8a65502 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -277,8 +277,11 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e
>>
>> 	best = cpuid_entry2_find(entries, nent, 0xD, 1);
>> 	if (best && (cpuid_entry_has(best, X86_FEATURE_XSAVES) ||
>> -		     cpuid_entry_has(best, X86_FEATURE_XSAVEC)))
>> -		best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
>> +		cpuid_entry_has(best, X86_FEATURE_XSAVEC))) {
> Align indentation.

OK. Thanks!

>
>   	if (best && (cpuid_entry_has(best, X86_FEATURE_XSAVES) ||
> 		     cpuid_entry_has(best, X86_FEATURE_XSAVEC))) {
>
>> +		u64 xstate = vcpu->arch.xcr0 | vcpu->arch.ia32_xss;
>> +
>> +		best->ebx = xstate_required_size(xstate, true);
>> +	}
>>
>> 	best = __kvm_find_kvm_cpuid_features(vcpu, entries, nent);
>> 	if (kvm_hlt_in_guest(vcpu->kvm) && best &&
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 33a780fe820b..ab3360a10933 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -3776,8 +3776,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>> 		 */
>> 		if (data & ~kvm_caps.supported_xss)
> Shouldn't we check against the supported value of _this_ guest? similar to
> guest_supported_xcr0.

I don't think it requires an extra variable to serve per guest purpose.

For guest XSS settings, now we don't add extra constraints like XCR0, 
thus all KVM supported

bits can be accessed by guest.  There's already another variable 
vcpu->arch.ia32_xss

to play similar role. In future, if there's requirement to serve per VM 
control purpose,

then will align it to XCR0 settings.


>
>> 			return 1;
>> -		vcpu->arch.ia32_xss = data;
>> -		kvm_update_cpuid_runtime(vcpu);
>> +		if (vcpu->arch.ia32_xss != data) {
>> +			vcpu->arch.ia32_xss = data;
>> +			kvm_update_cpuid_runtime(vcpu);
>> +		}
>> 		break;
>> 	case MSR_SMI_COUNT:
>> 		if (!msr_info->host_initiated)
>> -- 
>> 2.27.0
>>