| 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
| ||
|
Message-ID: <140483ed-f1f9-067e-6b60-eccd7115bb0d@redhat.com>
Date: Tue, 31 Mar 2020 17:03:06 +0200
From: Paolo Bonzini <pbonzini@...hat.com>
To: Sean Christopherson <sean.j.christopherson@...el.com>
Cc: Vitaly Kuznetsov <vkuznets@...hat.com>,
Wanpeng Li <wanpengli@...cent.com>,
Jim Mattson <jmattson@...gle.com>,
Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: VMX: Add a trampoline to fix VMREAD error handling
On 26/03/20 17:07, Sean Christopherson wrote:
> Add a hand coded assembly trampoline to preserve volatile registers
> across vmread_error(), and to handle the calling convention differences
> between 64-bit and 32-bit due to asmlinkage on vmread_error(). Pass
> @field and @fault on the stack when invoking the trampoline to avoid
> clobbering volatile registers in the context of the inline assembly.
>
> Calling vmread_error() directly from inline assembly is partially broken
> on 64-bit, and completely broken on 32-bit. On 64-bit, it will clobber
> %rdi and %rsi (used to pass @field and @fault) and any volatile regs
> written by vmread_error(). On 32-bit, asmlinkage means vmread_error()
> expects the parameters to be passed on the stack, not via regs.
>
> Opportunistically zero out the result in the trampoline to save a few
> bytes of code for every VMREAD. A happy side effect of the trampoline
> is that the inline code footprint is reduced by three bytes on 64-bit
> due to PUSH/POP being more efficent (in terms of opcode bytes) than MOV.
>
> Fixes: 6e2020977e3e6 ("KVM: VMX: Add error handling to VMREAD helper")
> Cc: stable@...r.kernel.org
> Signed-off-by: Sean Christopherson <sean.j.christopherson@...el.com>
> ---
>
> Becuase there just isn't enough custom assembly in VMX :-)
>
> Simply reverting isn't a great option because we'd lose error reporting
> for VMREAD failure, i.e. it'd return garbage with no other indication that
> something went awry.
>
> Tested all paths (fail, fault w/o rebooting, fault w/ rebooting) on both
> 64-bit and 32-bit.
>
> arch/x86/kvm/vmx/ops.h | 28 +++++++++++++-----
> arch/x86/kvm/vmx/vmenter.S | 58 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 79 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/ops.h b/arch/x86/kvm/vmx/ops.h
> index 45eaedee2ac0..09b0937d56b1 100644
> --- a/arch/x86/kvm/vmx/ops.h
> +++ b/arch/x86/kvm/vmx/ops.h
> @@ -12,7 +12,8 @@
>
> #define __ex(x) __kvm_handle_fault_on_reboot(x)
>
> -asmlinkage void vmread_error(unsigned long field, bool fault);
> +__attribute__((regparm(0))) void vmread_error_trampoline(unsigned long field,
> + bool fault);
> void vmwrite_error(unsigned long field, unsigned long value);
> void vmclear_error(struct vmcs *vmcs, u64 phys_addr);
> void vmptrld_error(struct vmcs *vmcs, u64 phys_addr);
> @@ -70,15 +71,28 @@ static __always_inline unsigned long __vmcs_readl(unsigned long field)
> asm volatile("1: vmread %2, %1\n\t"
> ".byte 0x3e\n\t" /* branch taken hint */
> "ja 3f\n\t"
> - "mov %2, %%" _ASM_ARG1 "\n\t"
> - "xor %%" _ASM_ARG2 ", %%" _ASM_ARG2 "\n\t"
> - "2: call vmread_error\n\t"
> - "xor %k1, %k1\n\t"
> +
> + /*
> + * VMREAD failed. Push '0' for @fault, push the failing
> + * @field, and bounce through the trampoline to preserve
> + * volatile registers.
> + */
> + "push $0\n\t"
> + "push %2\n\t"
> + "2:call vmread_error_trampoline\n\t"
> +
> + /*
> + * Unwind the stack. Note, the trampoline zeros out the
> + * memory for @fault so that the result is '0' on error.
> + */
> + "pop %2\n\t"
> + "pop %1\n\t"
> "3:\n\t"
>
> + /* VMREAD faulted. As above, except push '1' for @fault. */
> ".pushsection .fixup, \"ax\"\n\t"
> - "4: mov %2, %%" _ASM_ARG1 "\n\t"
> - "mov $1, %%" _ASM_ARG2 "\n\t"
> + "4: push $1\n\t"
> + "push %2\n\t"
> "jmp 2b\n\t"
> ".popsection\n\t"
> _ASM_EXTABLE(1b, 4b)
> diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
> index 81ada2ce99e7..861ae40e7144 100644
> --- a/arch/x86/kvm/vmx/vmenter.S
> +++ b/arch/x86/kvm/vmx/vmenter.S
> @@ -234,3 +234,61 @@ SYM_FUNC_START(__vmx_vcpu_run)
> 2: mov $1, %eax
> jmp 1b
> SYM_FUNC_END(__vmx_vcpu_run)
> +
> +/**
> + * vmread_error_trampoline - Trampoline from inline asm to vmread_error()
> + * @field: VMCS field encoding that failed
> + * @fault: %true if the VMREAD faulted, %false if it failed
> +
> + * Save and restore volatile registers across a call to vmread_error(). Note,
> + * all parameters are passed on the stack.
> + */
> +SYM_FUNC_START(vmread_error_trampoline)
> + push %_ASM_BP
> + mov %_ASM_SP, %_ASM_BP
> +
> + push %_ASM_AX
> + push %_ASM_CX
> + push %_ASM_DX
> +#ifdef CONFIG_X86_64
> + push %rdi
> + push %rsi
> + push %r8
> + push %r9
> + push %r10
> + push %r11
> +#endif
> +#ifdef CONFIG_X86_64
> + /* Load @field and @fault to arg1 and arg2 respectively. */
> + mov 3*WORD_SIZE(%rbp), %_ASM_ARG2
> + mov 2*WORD_SIZE(%rbp), %_ASM_ARG1
> +#else
> + /* Parameters are passed on the stack for 32-bit (see asmlinkage). */
> + push 3*WORD_SIZE(%ebp)
> + push 2*WORD_SIZE(%ebp)
> +#endif
> +
> + call vmread_error
> +
> +#ifndef CONFIG_X86_64
> + add $8, %esp
> +#endif
> +
> + /* Zero out @fault, which will be popped into the result register. */
> + _ASM_MOV $0, 3*WORD_SIZE(%_ASM_BP)
> +
> +#ifdef CONFIG_X86_64
> + pop %r11
> + pop %r10
> + pop %r9
> + pop %r8
> + pop %rsi
> + pop %rdi
> +#endif
> + pop %_ASM_DX
> + pop %_ASM_CX
> + pop %_ASM_AX
> + pop %_ASM_BP
> +
> + ret
> +SYM_FUNC_END(vmread_error_trampoline)
>
Queued, thanks.
Paolo
Powered by blists - more mailing lists