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

On Tue, Jun 21, 2022, Maxim Levitsky wrote:
> This removes the last user of put_smstate/GET_SMSTATE so
> remove these functions as well.
> 
> Also add a sanity check that we don't attempt to enter the SMM
> on non long mode capable guest CPU with a running nested guest.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@...hat.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  6 ------
>  arch/x86/kvm/svm/svm.c          | 28 +++++++++++++++++-----------
>  2 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 1038ccb7056a39..9e8467be96b4e6 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -2057,12 +2057,6 @@ static inline int kvm_cpu_get_apicid(int mps_cpu)
>  #endif
>  }
>  
> -#define put_smstate(type, buf, offset, val)                      \
> -	*(type *)((buf) + (offset) - 0x7e00) = val
> -
> -#define GET_SMSTATE(type, buf, offset)		\
> -	(*(type *)((buf) + (offset) - 0x7e00))
> -
>  int kvm_cpu_dirty_log_size(void);
>  
>  int memslot_rmap_alloc(struct kvm_memory_slot *slot, unsigned long npages);
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 136298cfb3fb57..8dcbbe839bef36 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4399,6 +4399,7 @@ static int svm_smi_allowed(struct kvm_vcpu *vcpu, bool for_injection)
>  
>  static int svm_enter_smm(struct kvm_vcpu *vcpu, char *smstate)
>  {
> +	struct kvm_smram_state_64 *smram = (struct kvm_smram_state_64 *)smstate;
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  	struct kvm_host_map map_save;
>  	int ret;
> @@ -4406,10 +4407,17 @@ static int svm_enter_smm(struct kvm_vcpu *vcpu, char *smstate)
>  	if (!is_guest_mode(vcpu))
>  		return 0;
>  
> -	/* FED8h - SVM Guest */
> -	put_smstate(u64, smstate, 0x7ed8, 1);
> -	/* FEE0h - SVM Guest VMCB Physical Address */
> -	put_smstate(u64, smstate, 0x7ee0, svm->nested.vmcb12_gpa);
> +	/*
> +	 * 32 bit SMRAM format doesn't preserve EFER and SVM state.
> +	 * SVM should not be enabled by the userspace without marking
> +	 * the CPU as at least long mode capable.
> +	 */
> +
> +	if (!guest_cpuid_has(vcpu, X86_FEATURE_SVM))
> +		return 1;

Isn't this is an independent bug fix?  I.e. should be in its own patch?

> +
> +	smram->svm_guest_flag = 1;
> +	smram->svm_guest_vmcb_gpa = svm->nested.vmcb12_gpa;
>  
>  	svm->vmcb->save.rax = vcpu->arch.regs[VCPU_REGS_RAX];
>  	svm->vmcb->save.rsp = vcpu->arch.regs[VCPU_REGS_RSP];