linux-kernel - Re: [PATCH v3 09/22] kvm: x86: Enable dynamic XSAVE features at KVM_SET

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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 28 Dec 2021 23:54:02 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Jing Liu <jing2.liu@...el.com>
Cc:     x86@...nel.org, kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-kselftest@...r.kernel.org,
        tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
        dave.hansen@...ux.intel.com, pbonzini@...hat.com, corbet@....net,
        shuah@...nel.org, jun.nakajima@...el.com, kevin.tian@...el.com,
        jing2.liu@...ux.intel.com, guang.zeng@...el.com,
        wei.w.wang@...el.com, yang.zhong@...el.com
Subject: Re: [PATCH v3 09/22] kvm: x86: Enable dynamic XSAVE features at
 KVM_SET_CPUID2

On Wed, Dec 22, 2021, Jing Liu wrote:
> Statically enable all xfeatures allowed by guest perm in

Statically isn't the right word.  It's not dymanic with respect to running the
vCPU, but it's certainly not static.  I think you can just omit "Statically"
entirely.

> KVM_SET_CPUID2, with fpstate buffer sized accordingly. This avoids
> run-time expansion in the emulation and restore path of XCR0 and
> XFD MSR [1].
> 
> Change kvm_vcpu_after_set_cpuid() to return error given fpstate
> reallocation may fail.
> 
> [1] https://lore.kernel.org/all/20211214024948.048572883@linutronix.de/
> 
> Suggested-by: Thomas Gleixner <tglx@...utronix.de>
> Suggested-by: Paolo Bonzini <pbonzini@...hat.com>
> Signed-off-by: Jing Liu <jing2.liu@...el.com>
> ---
>  arch/x86/kvm/cpuid.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index a068373a7fbd..eb5a5070accb 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -204,10 +204,12 @@ void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu)
>  }
>  EXPORT_SYMBOL_GPL(kvm_update_cpuid_runtime);
>  
> -static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> +static int kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_lapic *apic = vcpu->arch.apic;
>  	struct kvm_cpuid_entry2 *best;
> +	u64 xfeatures;
> +	int r;
>  
>  	best = kvm_find_cpuid_entry(vcpu, 1, 0);
>  	if (best && apic) {
> @@ -222,9 +224,17 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  	best = kvm_find_cpuid_entry(vcpu, 0xD, 0);
>  	if (!best)
>  		vcpu->arch.guest_supported_xcr0 = 0;
> -	else
> -		vcpu->arch.guest_supported_xcr0 =
> -			(best->eax | ((u64)best->edx << 32)) & supported_xcr0;
> +	else {
> +		xfeatures = best->eax | ((u64)best->edx << 32);
> +
> +		vcpu->arch.guest_supported_xcr0 = xfeatures & supported_xcr0;
> +
> +		if (xfeatures != vcpu->arch.guest_fpu.xfeatures) {
> +			r = fpu_update_guest_perm_features(&vcpu->arch.guest_fpu);
> +			if (r)
> +				return r;

IMO, this should be done and check before "committing" state, otherwise KVM will
set the vCPU's CPUID info and update a variety of state, but then tell userspace
that it failed.  The -EPERM case in particular falls squarely into the "check"
category.