linux-kernel - Re: [RFC Part2 PATCH v3 14/26] KVM: SVM: VMRUN should use assosiated ASID when SEV is enabled

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:   Wed, 13 Sep 2017 17:37:46 +0200
From:   Borislav Petkov <bp@...e.de>
To:     Brijesh Singh <brijesh.singh@....com>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org, kvm@...r.kernel.org,
        Thomas Gleixner <tglx@...utronix.de>,
        Joerg Roedel <joro@...tes.org>,
        "Michael S . Tsirkin" <mst@...hat.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        \"Radim Krčmář\" <rkrcmar@...hat.com>,
        Tom Lendacky <thomas.lendacky@....com>
Subject: Re: [RFC Part2 PATCH v3 14/26] KVM: SVM: VMRUN should use assosiated
 ASID when SEV is enabled

On Mon, Jul 24, 2017 at 03:02:51PM -0500, Brijesh Singh wrote:
> SEV hardware uses ASIDs to associate memory encryption key with the
> guest VMs. During the guest creation time, we use SEV_CMD_ACTIVATE

"VM"

> command to bind a particular ASID to the guest. Lets make sure that
> VMCB is programmed with the binded ASID before a VMRUN.

"the VMCB"	 	 	binded? you mean "bound"

> 
> Signed-off-by: Brijesh Singh <brijesh.singh@....com>
> ---
>  arch/x86/kvm/svm.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index e99a572..72f7c27 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -213,6 +213,9 @@ struct vcpu_svm {
>  	 */
>  	struct list_head ir_list;
>  	spinlock_t ir_list_lock;
> +
> +	/* which host cpu was used for running this vcpu */

s/cpu/CPU/

> +	unsigned int last_cpuid;

... and since it is a CPU, then "last_cpu" I guess.

>  };
>  
>  /*
> @@ -573,6 +576,8 @@ struct svm_cpu_data {
>  	struct kvm_ldttss_desc *tss_desc;
>  
>  	struct page *save_area;
> +
> +	struct vmcb **sev_vmcbs;  /* index = sev_asid, value = vmcb pointer */

Put that comment above it.

>  static DEFINE_PER_CPU(struct svm_cpu_data *, svm_data);
> @@ -886,6 +891,7 @@ static void svm_cpu_uninit(int cpu)
>  		return;
>  
>  	per_cpu(svm_data, raw_smp_processor_id()) = NULL;
> +	kfree(sd->sev_vmcbs);
>  	__free_page(sd->save_area);
>  	kfree(sd);
>  }
> @@ -904,6 +910,14 @@ static int svm_cpu_init(int cpu)
>  	if (!sd->save_area)
>  		goto err_1;
>  
> +	if (svm_sev_enabled()) {
> +		sd->sev_vmcbs = kmalloc((max_sev_asid + 1) * sizeof(void *),
> +					GFP_KERNEL);

You can let that line stick out.

> +		r = -ENOMEM;

That assignment usually is done before the call. I know, this function
does it differently but you can fix it up while you're touching it.

> +		if (!sd->sev_vmcbs)
> +			goto err_1;
> +	}
> +
>  	per_cpu(svm_data, cpu) = sd;
>  
>  	return 0;
> @@ -4442,12 +4456,40 @@ static void reload_tss(struct kvm_vcpu *vcpu)
>  	load_TR_desc();
>  }
>  
> +static void pre_sev_run(struct vcpu_svm *svm)
> +{
> +	int cpu = raw_smp_processor_id();
> +	int asid = sev_get_asid(svm->vcpu.kvm);
> +	struct svm_cpu_data *sd = per_cpu(svm_data, cpu);
> +
> +	/* Assign the asid allocated with this SEV guest */
> +	svm->vmcb->control.asid = asid;
> +
> +	/*
> +	 * Flush guest TLB:
> +	 *
> +	 * 1) when different VMCB for the same ASID is to be run on the same host CPU.
> +	 * 2) or this VMCB was executed on different host cpu in previous VMRUNs.

s/cpu/CPU/

> +	 */
> +	if (sd->sev_vmcbs[asid] == svm->vmcb &&
> +		svm->last_cpuid == cpu)
> +		return;
> +
> +	svm->last_cpuid = cpu;
> +	sd->sev_vmcbs[asid] = svm->vmcb;
> +	svm->vmcb->control.tlb_ctl = TLB_CONTROL_FLUSH_ASID;
> +	mark_dirty(svm->vmcb, VMCB_ASID);
> +}
> +
>  static void pre_svm_run(struct vcpu_svm *svm)
>  {
>  	int cpu = raw_smp_processor_id();
>  
>  	struct svm_cpu_data *sd = per_cpu(svm_data, cpu);
>  
> +	if (sev_guest(svm->vcpu.kvm))
> +		return pre_sev_run(svm);

Just pass @cpu here so that you don't have to do raw_smp_processor_id()
again above.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--