linux-kernel - Re: [Part2 PATCH v6 25/38] KVM: SVM: Add KVM_SEV

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:   Fri, 27 Oct 2017 22:24:21 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Brijesh Singh <brijesh.singh@....com>
Cc:     kvm@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        Joerg Roedel <joro@...tes.org>,
        Tom Lendacky <thomas.lendacky@....com>, x86@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [Part2 PATCH v6 25/38] KVM: SVM: Add KVM_SEV_INIT command

On Thu, Oct 19, 2017 at 09:34:00PM -0500, Brijesh Singh wrote:
> The command initializes the SEV platform context and allocates a new ASID
> for this guest from the SEV ASID pool. The firmware must be initialized
> before we issue any guest launch commands to create a new memory encryption
> context.
> 
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: "H. Peter Anvin" <hpa@...or.com>
> Cc: Paolo Bonzini <pbonzini@...hat.com>
> Cc: "Radim Krčmář" <rkrcmar@...hat.com>
> Cc: Joerg Roedel <joro@...tes.org>
> Cc: Borislav Petkov <bp@...e.de>
> Cc: Tom Lendacky <thomas.lendacky@....com>
> Cc: x86@...nel.org
> Cc: kvm@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org
> Signed-off-by: Brijesh Singh <brijesh.singh@....com>
> ---
>  arch/x86/include/asm/kvm_host.h |   7 +++
>  arch/x86/kvm/svm.c              | 132 +++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 138 insertions(+), 1 deletion(-)

...

> @@ -1167,10 +1187,18 @@ static __init int svm_hardware_setup(void)
>  	return r;
>  }
>  
> +static __exit void sev_hardware_unsetup(void)
> +{
> +	kfree(sev_asid_bitmap);
> +}
> +
>  static __exit void svm_hardware_unsetup(void)
>  {
>  	int cpu;
>  
> +	if (svm_sev_enabled())
> +		sev_hardware_unsetup();

Move that svm_sev_enabled() check into the function.

> +
>  	for_each_possible_cpu(cpu)
>  		svm_cpu_uninit(cpu);
>  
> @@ -1361,6 +1389,9 @@ static void init_vmcb(struct vcpu_svm *svm)
>  		svm->vmcb->control.int_ctl |= V_GIF_ENABLE_MASK;
>  	}
>  
> +	if (sev_guest(svm->vcpu.kvm))
> +		svm->vmcb->control.nested_ctl |= SVM_NESTED_CTL_SEV_ENABLE;
> +
>  	mark_all_dirty(svm->vmcb);
>  
>  	enable_gif(svm);
> @@ -1443,6 +1474,28 @@ static int avic_init_backing_page(struct kvm_vcpu *vcpu)
>  	return 0;
>  }
>  
> +static void sev_asid_free(struct kvm *kvm)
> +{
> +	struct kvm_sev_info *sev = &kvm->arch.sev_info;
> +	int pos, asid;
> +
> +	if (!svm_sev_enabled())
> +		return;

You're already checking !sev_guest() below - no need to do the
svm_sev_enabled() check again.

> +
> +	asid = sev->asid;
> +	pos = asid - 1;
> +	clear_bit(pos, sev_asid_bitmap);
> +}
> +
> +static void sev_vm_destroy(struct kvm *kvm)
> +{
> +	if (!sev_guest(kvm))
> +		return;
> +
> +	sev_platform_shutdown(NULL);
> +	sev_asid_free(kvm);
> +}
> +
>  static void avic_vm_destroy(struct kvm *kvm)
>  {
>  	unsigned long flags;
> @@ -1461,6 +1514,12 @@ static void avic_vm_destroy(struct kvm *kvm)
>  	spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);

>  }
>  
> +static void svm_vm_destroy(struct kvm *kvm)
> +{
> +	avic_vm_destroy(kvm);
> +	sev_vm_destroy(kvm);
> +}
> +
>  static int avic_vm_init(struct kvm *kvm)
>  {
>  	unsigned long flags;
> @@ -5427,6 +5486,75 @@ static void svm_setup_mce(struct kvm_vcpu *vcpu)
>  	vcpu->arch.mcg_cap &= 0x1ff;
>  }
>  
> +static int sev_asid_new(void)
> +{
> +	int pos;
> +
> +	if (!svm_sev_enabled())
> +		return -EINVAL;

You already checked that in svm_mem_enc_op() - no need to do it again.

> +	pos = find_first_zero_bit(sev_asid_bitmap, max_sev_asid);
> +	if (pos >= max_sev_asid)
> +		return -EBUSY;
> +
> +	set_bit(pos, sev_asid_bitmap);
> +	return pos + 1;
> +}
> +
> +static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
> +{
> +	struct kvm_sev_info *sev = &kvm->arch.sev_info;
> +	int asid, ret;
> +
> +	ret = sev_platform_init(NULL, &argp->error);
> +	if (ret)
> +		return ret;
> +
> +	ret = -EBUSY;
> +	asid = sev_asid_new();

This operation is cheaper so do it first and then sev_platform_init()
so that in case sev_asid_new() fails, you don't "toggle" the PSP
unnecessarily.

> +	if (asid < 0)
> +		goto e_shutdown;
> +
> +	sev->active = true;
> +	sev->asid = asid;
> +
> +	return 0;
> +
> +e_shutdown:
> +	sev_platform_shutdown(&argp->error);
> +	return ret;
> +}
> +

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--