On Mon, Oct 29, 2018 at 04:40:43PM +0100, 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 <jstec...@amazon.de>
> Reviewed-by: Jan H. Schönherr <jscho...@amazon.de>
> Reviewed-by: Konrad Jan Miller <k...@amazon.de>
> Reviewed-by: Jim Mattson <jmatt...@google.com>
> ---
> arch/x86/kvm/vmx.c | 45 ++++++++++++++++++++++++++++-----------------
> 1 file changed, 28 insertions(+), 17 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index a6e5a5c..29a2ee7 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -11281,24 +11281,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"
> - /*
> - * Clear host registers marked as clobbered to prevent
> - * speculative use.
> - */
> - "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"
> @@ -11336,6 +11319,34 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu
> *vcpu)
> );
>
> /*
> + * Don't let guest register values survive. Registers that cannot
> + * contain guest values anymore are not touched.
I think it's a good idea to explicitly call out that clearing the GPRs
is done to prevent speculative use. Simply stating that we don't want
to let guest register values survive doesn't explain *why*.
What about:
/*
* Explicitly clear (in addition to marking them as clobbered) all GPRs
* that have not been loaded with host state to prevent speculatively
* using the guest's values.
*/
> + */
> + asm volatile (
> + "xor %%eax, %%eax \n\t"
> + "xor %%ebx, %%ebx \n\t"
> + "xor %%esi, %%esi \n\t"
> + "xor %%edi, %%edi \n\t"
> +#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
> + ::: "cc"
> +#ifdef CONFIG_X86_64
> + , "rax", "rbx", "rsi", "rdi"
> + , "r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15"
> +#else
> + , "eax", "ebx", "esi", "edi"
> +#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
> * turn it off. This is much more efficient than blindly adding
> --
> 2.7.4
>
