linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Julian Stecklina <jsteckli@amazon.de>
Cc: kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
	js@alien8.de, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/4] kvm, vmx: move register clearing out of assembly path
Date: Fri, 26 Oct 2018 08:46:17 -0700	[thread overview]
Message-ID: <20181026154617.GA23663@linux.intel.com> (raw)
In-Reply-To: <558fea0b4df498eefcaea5ae07a089ad9706c1a2.1540369608.git.jsteckli@amazon.de>

On Wed, Oct 24, 2018 at 10:28:57AM +0200, Julian Stecklina wrote:
> Split the security related register clearing out of the large inline
> assembly VM entry path. This results in two slightly less complicated
> inline assembly statements, where it is clearer what each one does.
> 
> Signed-off-by: Julian Stecklina <jsteckli@amazon.de>
> Reviewed-by: Jan H. Schönherr <jschoenh@amazon.de>
> Reviewed-by: Konrad Jan Miller <kjm@amazon.de>
> ---
>  arch/x86/kvm/vmx.c | 33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 93562d5..9225099 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -10797,20 +10797,7 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
>  		"mov %%r13, %c[r13](%0) \n\t"
>  		"mov %%r14, %c[r14](%0) \n\t"
>  		"mov %%r15, %c[r15](%0) \n\t"
> -		"xor %%r8d,  %%r8d \n\t"
> -		"xor %%r9d,  %%r9d \n\t"
> -		"xor %%r10d, %%r10d \n\t"
> -		"xor %%r11d, %%r11d \n\t"
> -		"xor %%r12d, %%r12d \n\t"
> -		"xor %%r13d, %%r13d \n\t"
> -		"xor %%r14d, %%r14d \n\t"
> -		"xor %%r15d, %%r15d \n\t"
>  #endif
> -
> -		"xor %%eax, %%eax \n\t"
> -		"xor %%ebx, %%ebx \n\t"
> -		"xor %%esi, %%esi \n\t"
> -		"xor %%edi, %%edi \n\t"
>  		"pop  %%" _ASM_BP "; pop  %%" _ASM_DX " \n\t"
>  		".pushsection .rodata \n\t"
>  		".global vmx_return \n\t"
> @@ -10847,6 +10834,26 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
>  #endif
>  	      );
>  
> +	/* Don't let guest register values survive. */
> +	asm volatile (
> +		""
> +#ifdef CONFIG_X86_64
> +		"xor %%r8d,  %%r8d \n\t"
> +		"xor %%r9d,  %%r9d \n\t"
> +		"xor %%r10d, %%r10d \n\t"
> +		"xor %%r11d, %%r11d \n\t"
> +		"xor %%r12d, %%r12d \n\t"
> +		"xor %%r13d, %%r13d \n\t"
> +		"xor %%r14d, %%r14d \n\t"
> +		"xor %%r15d, %%r15d \n\t"
> +#endif
> +		:: "a" (0), "b" (0), "S" (0), "D" (0)

Since clearing the GPRs exists to mitigate speculation junk, I think
we should keep the explicit XOR zeroing instead of deferring to the
compiler.  Explicit XORs will ensure the resulting assembly is the
same regardless of compiler, version, target arch, etc..., whereas the
compiler could theoretically use different zeroing methods[1], e.g. on
my system it generates "mov r32,r32" for EBX, ESI and EDI (loading
from EAX after EAX is zeroed).

And FWIW, I find the original code to be more readable since all GRPs
are zeroed with the same method.


[1] As an aside, I was expecting gcc to generate "xor r32,r32" with
    -mtune=sandybridge as sandybridge can do mov elimination on xors
    that explicitly zero a register but not on generic reg-to-reg mov,
    but I was unable to coerce gcc into using xor.

> +		: "cc"
> +#ifdef CONFIG_X86_64
> +		  , "r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15"
> +#endif
> +		);
> +
>  	/*
>  	 * We do not use IBRS in the kernel. If this vCPU has used the
>  	 * SPEC_CTRL MSR it may have left it on; save the value and
> -- 
> 2.7.4
> 

  parent reply	other threads:[~2018-10-26 15:46 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-24  8:28 [PATCH 1/4] kvm, vmx: move CR2 context switch " Julian Stecklina
2018-10-24  8:28 ` [PATCH 2/4] kvm, vmx: move register clearing " Julian Stecklina
2018-10-25 16:55   ` Jim Mattson
2018-10-26 10:31     ` Stecklina, Julian
2018-10-26 15:46   ` Sean Christopherson [this message]
2018-10-26 16:30     ` Jim Mattson
2018-10-29 13:47       ` Stecklina, Julian
2018-10-24  8:28 ` [PATCH 3/4] kvm, vmx: fix __invvpid style Julian Stecklina
2018-10-24  8:28 ` [PATCH 4/4] kvm, vmx: remove manually coded vmx instructions Julian Stecklina
2018-10-24 17:44   ` Eric Northup
2018-10-26 10:46     ` Stecklina, Julian
2018-10-26 14:30       ` Sean Christopherson
2018-10-25 17:02 ` [PATCH 1/4] kvm, vmx: move CR2 context switch out of assembly path Jim Mattson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181026154617.GA23663@linux.intel.com \
    --to=sean.j.christopherson@intel.com \
    --cc=js@alien8.de \
    --cc=jsteckli@amazon.de \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --subject='Re: [PATCH 2/4] kvm, vmx: move register clearing out of assembly path' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).