linux-kernel - Re: [RESEND PATCH 04/13] KVM: x86: Drop EMULTYPE_NO_UD_ON

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]
Date:   Fri, 23 Aug 2019 16:21:24 +0300
From:   Liran Alon <liran.alon@...cle.com>
To:     Sean Christopherson <sean.j.christopherson@...el.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        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: [RESEND PATCH 04/13] KVM: x86: Drop EMULTYPE_NO_UD_ON_FAIL as a
 standalone type



> On 23 Aug 2019, at 4:07, Sean Christopherson <sean.j.christopherson@...el.com> wrote:
> 
> The "no #UD on fail" is used only in the VMWare case, and for the VMWare
> scenario it really means "#GP instead of #UD on fail".  Remove the flag
> in preparation for moving all fault injection into the emulation flow
> itself, which in turn will allow eliminating EMULATE_DONE and company.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@...el.com>

When I created the commit which introduced this
e23661712005 ("KVM: x86: Add emulation_type to not raise #UD on emulation failure")
I intentionally introduced a new flag to emulation_type instead of using EMULTYPE_VMWARE
as I thought it’s weird to couple this behaviour specifically with VMware emulation.
As it made sense to me that there could be more scenarios in which some VMExit handler
would like to use the x86 emulator but in case of failure want to decide what would be
the failure handling from the outside. I also didn’t want the x86 emulator to be aware
of VMware interception internals.

Having said that, one could argue that the x86 emulator already knows about the VMware
interception internals because of how x86_emulate_instruction() use is_vmware_backdoor_opcode()
and from the mere existence of EMULTYPE_VMWARE. So I think it’s legit to decide
that we will just move all the VMware interception logic into the x86 emulator. Including
handling emulation failures. But then, I would make this patch of yours to also
modify handle_emulation_failure() to queue #GP to guest directly instead of #GP intercept
in VMX/SVM to do so.
I see you do it in a later patch "KVM: x86: Move #GP injection for VMware into x86_emulate_instruction()"
but I think this should just be squashed with this patch to make sense.

To sum-up, I agree with your approach but I recommend you squash this patch and patch 6 of the series to one
and change commit message to explain that you just move entire handling of VMware interception into
the x86 emulator. Instead of providing explanations such as VMware emulation is the only one that use
“no #UD on fail”.

The diff itself looks fine to me, therefore:
Reviewed-by: Liran Alon <liran.alon@...cle.com>

-Liran


> ---
> arch/x86/include/asm/kvm_host.h | 1 -
> arch/x86/kvm/svm.c              | 3 +--
> arch/x86/kvm/vmx/vmx.c          | 3 +--
> arch/x86/kvm/x86.c              | 2 +-
> 4 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 44a5ce57a905..dd6bd9ed0839 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1318,7 +1318,6 @@ enum emulation_result {
> #define EMULTYPE_TRAP_UD	    (1 << 1)
> #define EMULTYPE_SKIP		    (1 << 2)
> #define EMULTYPE_ALLOW_RETRY	    (1 << 3)
> -#define EMULTYPE_NO_UD_ON_FAIL	    (1 << 4)
> #define EMULTYPE_VMWARE		    (1 << 5)
> int kvm_emulate_instruction(struct kvm_vcpu *vcpu, int emulation_type);
> int kvm_emulate_instruction_from_buffer(struct kvm_vcpu *vcpu,
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 1f220a85514f..5a42f9c70014 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2772,8 +2772,7 @@ static int gp_interception(struct vcpu_svm *svm)
> 
> 	WARN_ON_ONCE(!enable_vmware_backdoor);
> 
> -	er = kvm_emulate_instruction(vcpu,
> -		EMULTYPE_VMWARE | EMULTYPE_NO_UD_ON_FAIL);
> +	er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
> 	if (er == EMULATE_USER_EXIT)
> 		return 0;
> 	else if (er != EMULATE_DONE)
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 18286e5b5983..6ecf773825e2 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4509,8 +4509,7 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
> 
> 	if (!vmx->rmode.vm86_active && is_gp_fault(intr_info)) {
> 		WARN_ON_ONCE(!enable_vmware_backdoor);
> -		er = kvm_emulate_instruction(vcpu,
> -			EMULTYPE_VMWARE | EMULTYPE_NO_UD_ON_FAIL);
> +		er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
> 		if (er == EMULATE_USER_EXIT)
> 			return 0;
> 		else if (er != EMULATE_DONE)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index fe847f8eb947..e0f0e14d8fac 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6210,7 +6210,7 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
> 	++vcpu->stat.insn_emulation_fail;
> 	trace_kvm_emulate_insn_failed(vcpu);
> 
> -	if (emulation_type & EMULTYPE_NO_UD_ON_FAIL)
> +	if (emulation_type & EMULTYPE_VMWARE)
> 		return EMULATE_FAIL;
> 
> 	kvm_queue_exception(vcpu, UD_VECTOR);
> -- 
> 2.22.0
>