linux-kernel - Re: [PATCH] KVM: x86: Improve exception safe wrappers in emulate.c

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]

Message-ID: <YWcyeGk7vOSoQWW4@google.com>
Date:   Wed, 13 Oct 2021 19:24:40 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Uros Bizjak <ubizjak@...il.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [PATCH] KVM: x86: Improve exception safe wrappers in emulate.c

On Fri, Sep 17, 2021, Uros Bizjak wrote:
> Improve exception safe wrappers in emulate.c by converting them to
> ASM GOTO (and ASM GOTO OUTPUT when supported) statements.  Also, convert
> wrappers to inline functions to avoid statement as expression
> GNU extension and to remove weird requirement where user must know
> where the asm argument is being expanded.
> 
> Cc: Paolo Bonzini <pbonzini@...hat.com>
> Cc: Sean Christopherson  <seanjc@...gle.com>
> Signed-off-by: Uros Bizjak <ubizjak@...il.com>
> ---
>  arch/x86/kvm/emulate.c | 80 ++++++++++++++++++++++++++++++------------
>  1 file changed, 57 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 2837110e66ed..2197a3ecc55b 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -464,25 +464,59 @@ FOP_FUNC(salc)
>  FOP_RET(salc)
>  FOP_END;
>  
> -/*
> - * XXX: inoutclob user must know where the argument is being expanded.

I 100% agree that this is a weird requirement, but I actually like the side
effect of forcing the caller to define a name for the input/output.

> - *      Relying on CONFIG_CC_HAS_ASM_GOTO would allow us to remove _fault.
> - */
> -#define asm_safe(insn, inoutclob...) \
> -({ \
> -	int _fault = 0; \
> - \
> -	asm volatile("1:" insn "\n" \
> -	             "2:\n" \
> -	             ".pushsection .fixup, \"ax\"\n" \
> -	             "3: movl $1, %[_fault]\n" \
> -	             "   jmp  2b\n" \
> -	             ".popsection\n" \
> -	             _ASM_EXTABLE(1b, 3b) \
> -	             : [_fault] "+qm"(_fault) inoutclob ); \
> - \
> -	_fault ? X86EMUL_UNHANDLEABLE : X86EMUL_CONTINUE; \
> -})
> +static __always_inline int safe_fwait(void)
> +{
> +	asm_volatile_goto("1: fwait\n\t"
> +			  _ASM_EXTABLE(1b, %l[fault])
> +			  : : : : fault);
> +	return X86EMUL_CONTINUE;
> + fault:
> +	return X86EMUL_UNHANDLEABLE;
> +}

Rather than defining a bunch of safe_() variants, what about providing a generic
helper/macro similar to the existing asm_safe()?  Not just for KVM, but for the
kernel at large.  Asm with output is problematic due to the CONFIG_CC_HAS_ASM_GOTO
dependency, but it wouldn't be the end of the world to state that simply isn't
supported until the min compiler version is raised.

__wrmsr(), native_write_msr_safe(), cpu_vmxoff(), kvm_cpu_vmxon(), and probably
others could use a generic generic asm_safe().  I wouldn't be surprised if there
are other places in the kernel that could take advantage of such a helper, e.g.
kvm_load_ldt() could use a "safe" variant instead of crashing if the sel is bad.