linux-kernel - Re: [PATCH v3 11/13] KVM: x86: SVM: use smram structs

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: <YwapX2HFpUGbxBZJ@google.com>
Date:   Wed, 24 Aug 2022 22:42:39 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Maxim Levitsky <mlevitsk@...hat.com>
Cc:     kvm@...r.kernel.org, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        linux-kernel@...r.kernel.org, Wanpeng Li <wanpengli@...cent.com>,
        Ingo Molnar <mingo@...hat.com>, x86@...nel.org,
        Jim Mattson <jmattson@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        "H. Peter Anvin" <hpa@...or.com>, Joerg Roedel <joro@...tes.org>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [PATCH v3 11/13] KVM: x86: SVM: use smram structs

On Wed, Aug 03, 2022, Maxim Levitsky wrote:
> @@ -4486,28 +4482,23 @@ static int svm_leave_smm(struct kvm_vcpu *vcpu, const union kvm_smram *smram)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  	struct kvm_host_map map, map_save;
> -	u64 saved_efer, vmcb12_gpa;
>  	struct vmcb *vmcb12;
>  	int ret;
>  
> -	const char *smstate = (const char *)smram;

To shorten line lengths, what about grabbing smram64 here?

	const kvm_smram_state_64 *smram64 = &smram->smram64;

IMO, makes things a little easier to read too.

> -
>  	if (!guest_cpuid_has(vcpu, X86_FEATURE_LM))
>  		return 0;
>  
>  	/* Non-zero if SMI arrived while vCPU was in guest mode. */
> -	if (!GET_SMSTATE(u64, smstate, 0x7ed8))
> +	if (!smram->smram64.svm_guest_flag)
>  		return 0;
>  
>  	if (!guest_cpuid_has(vcpu, X86_FEATURE_SVM))
>  		return 1;
>  
> -	saved_efer = GET_SMSTATE(u64, smstate, 0x7ed0);
> -	if (!(saved_efer & EFER_SVME))
> +	if (!(smram->smram64.efer & EFER_SVME))
>  		return 1;
>  
> -	vmcb12_gpa = GET_SMSTATE(u64, smstate, 0x7ee0);
> -	if (kvm_vcpu_map(vcpu, gpa_to_gfn(vmcb12_gpa), &map) == -EINVAL)
> +	if (kvm_vcpu_map(vcpu, gpa_to_gfn(smram->smram64.svm_guest_vmcb_gpa), &map) == -EINVAL)

Eww, this is horrifically wrong.  KVM will explode if kvm_vcpu_map() returns
-EFAULT, e.g. if guest memory is not backed by struct page and memremap() fails.
Not to mention that it's comically fragile too.

Can you add a patch to this series to drop the "== -EINVAL"?

And there's another one lurking just out of sight in this diff.

>  		return 1;
>  
>  	ret = 1;
> @@ -4533,7 +4524,7 @@ static int svm_leave_smm(struct kvm_vcpu *vcpu, const union kvm_smram *smram)
>  	vmcb12 = map.hva;
>  	nested_copy_vmcb_control_to_cache(svm, &vmcb12->control);
>  	nested_copy_vmcb_save_to_cache(svm, &vmcb12->save);
> -	ret = enter_svm_guest_mode(vcpu, vmcb12_gpa, vmcb12, false);
> +	ret = enter_svm_guest_mode(vcpu, smram->smram64.svm_guest_vmcb_gpa, vmcb12, false);
>  
>  	if (ret)
>  		goto unmap_save;
> -- 
> 2.26.3
>