Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <558fea0b4df498eefcaea5ae07a089ad9706c1a2.1540369608.git.jsteckli@amazon.de>
References: <09986c98c9655f1542768ecfda644ac821e67a57.1540369608.git.jsteckli@amazon.de>
 <558fea0b4df498eefcaea5ae07a089ad9706c1a2.1540369608.git.jsteckli@amazon.de>
From:   Jim Mattson <jmattson@google.com>
Date:   Thu, 25 Oct 2018 09:55:16 -0700
Message-ID: <CALMp9eQBhz1fzN4z=oMy5X9OKN3JxNyiD4GBYbH+fhen_aP6Ww@mail.gmail.com>
Subject: Re: [PATCH 2/4] kvm, vmx: move register clearing out of assembly path
To:     Julian Stecklina <jsteckli@amazon.de>
Cc:     kvm list <kvm@vger.kernel.org>,
        Paolo Bonzini <pbonzini@redhat.com>, js@alien8.de,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Oct 24, 2018 at 1:28 AM, Julian Stecklina <jsteckli@amazon.de> wrot=
e:
> 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=C3=B6nherr <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_vc=
pu *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_vc=
pu *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)
> +               : "cc"
> +#ifdef CONFIG_X86_64
> +                 , "r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15"
> +#endif
> +               );
> +

Looking at the second asm statement and the comment that precedes it,
my first question would be, "What about the registers not covered
here?" I'm also not convinced that the register-clearing asm statement
is actually "clearer" with some registers cleared as input arguments
and others cleared explicitly, but otherwise, the change looks fine to
me.

Reviewed-by: Jim Mattson <jmattson@google.com>