linux-kernel - Re: [PATCH AUTOSEL for 4.14 006/100] KVM: nVMX/nSVM: Don't intercept #UD when running L2

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-next>] [day] [month] [year] [list]

Message-ID: <4693d8ab-1c86-44b6-b24f-009ee8766c1e@default>
Date:   Thu, 25 Jan 2018 01:25:18 -0800 (PST)
From:   Liran Alon <liran.alon@...cle.com>
To:     <Alexander.Levin@...rosoft.com>
Cc:     <konrad.wilk@...cle.com>, <stable@...r.kernel.org>,
        <rkrcmar@...hat.com>, <pbonzini@...hat.com>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH AUTOSEL for 4.14 006/100] KVM: nVMX/nSVM: Don't intercept
 #UD when running L2


----- Alexander.Levin@...rosoft.com wrote:

> From: Liran Alon <liran.alon@...cle.com>
> 
> [ Upstream commit ac9b305caa0df6f5b75d294e4b86c1027648991e ]
> 
> When running L2, #UD should be intercepted by L1 or just forwarded
> directly to L2. It should not reach L0 x86 emulator.
> Therefore, set intercept for #UD only based on L1 exception-bitmap.
> 
> Also add WARN_ON_ONCE() on L0 #UD intercept handlers to make sure
> it is never reached while running L2.
> 
> This improves commit ae1f57670703 ("KVM: nVMX: Do not emulate #UD
> while
> in guest mode") by removing an unnecessary exit from L2 to L0 on #UD
> when L1 doesn't intercept it.
> 
> In addition, SVM L0 #UD intercept handler doesn't handle correctly
> the
> case it is raised from L2. In this case, it should forward the #UD to
> guest instead of x86 emulator. As done in VMX #UD intercept handler.
> This commit fixes this issue as-well.
> 
> Signed-off-by: Liran Alon <liran.alon@...cle.com>
> Reviewed-by: Nikita Leshenko <nikita.leshchenko@...cle.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
> Reviewed-by: Paolo Bonzini <pbonzini@...hat.com>
> Reviewed-by: Wanpeng Li <wanpeng.li@...mail.com>
> Signed-off-by: Radim Krčmář <rkrcmar@...hat.com>
> Signed-off-by: Sasha Levin <alexander.levin@...rosoft.com>
> ---
>  arch/x86/kvm/svm.c | 9 ++++++++-
>  arch/x86/kvm/vmx.c | 9 ++++-----
>  2 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 6a8284f72328..c8be4e6d365b 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -362,6 +362,7 @@ static void recalc_intercepts(struct vcpu_svm
> *svm)
>  {
>  	struct vmcb_control_area *c, *h;
>  	struct nested_state *g;
> +	u32 h_intercept_exceptions;
>  
>  	mark_dirty(svm->vmcb, VMCB_INTERCEPTS);
>  
> @@ -372,9 +373,14 @@ static void recalc_intercepts(struct vcpu_svm
> *svm)
>  	h = &svm->nested.hsave->control;
>  	g = &svm->nested;
>  
> +	/* No need to intercept #UD if L1 doesn't intercept it */
> +	h_intercept_exceptions =
> +		h->intercept_exceptions & ~(1U << UD_VECTOR);
> +
>  	c->intercept_cr = h->intercept_cr | g->intercept_cr;
>  	c->intercept_dr = h->intercept_dr | g->intercept_dr;
> -	c->intercept_exceptions = h->intercept_exceptions |
> g->intercept_exceptions;
> +	c->intercept_exceptions =
> +		h_intercept_exceptions | g->intercept_exceptions;
>  	c->intercept = h->intercept | g->intercept;
>  }
>  
> @@ -2189,6 +2195,7 @@ static int ud_interception(struct vcpu_svm
> *svm)
>  {
>  	int er;
>  
> +	WARN_ON_ONCE(is_guest_mode(&svm->vcpu));
>  	er = emulate_instruction(&svm->vcpu, EMULTYPE_TRAP_UD);
>  	if (er == EMULATE_USER_EXIT)
>  		return 0;
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index ef16cf0f7cfd..36628ed362d8 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -1891,7 +1891,7 @@ static void update_exception_bitmap(struct
> kvm_vcpu *vcpu)
>  {
>  	u32 eb;
>  
> -	eb = (1u << PF_VECTOR) | (1u << UD_VECTOR) | (1u << MC_VECTOR) |
> +	eb = (1u << PF_VECTOR) | (1u << MC_VECTOR) |
>  	     (1u << DB_VECTOR) | (1u << AC_VECTOR);
>  	if ((vcpu->guest_debug &
>  	     (KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP)) ==
> @@ -1909,6 +1909,8 @@ static void update_exception_bitmap(struct
> kvm_vcpu *vcpu)
>  	 */
>  	if (is_guest_mode(vcpu))
>  		eb |= get_vmcs12(vcpu)->exception_bitmap;
> +	else
> +		eb |= 1u << UD_VECTOR;
>  
>  	vmcs_write32(EXCEPTION_BITMAP, eb);
>  }
> @@ -5919,10 +5921,7 @@ static int handle_exception(struct kvm_vcpu
> *vcpu)
>  		return 1;  /* already handled by vmx_vcpu_run() */
>  
>  	if (is_invalid_opcode(intr_info)) {
> -		if (is_guest_mode(vcpu)) {
> -			kvm_queue_exception(vcpu, UD_VECTOR);
> -			return 1;
> -		}
> +		WARN_ON_ONCE(is_guest_mode(vcpu));
>  		er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
>  		if (er == EMULATE_USER_EXIT)
>  			return 0;
> -- 
> 2.11.0

Just wanted stable maintainers to note that Jim, Paolo & myself decided eventually to revert this commit along with commit ae1f57670703 on upstream KVM. However, it is true that this commit makes commit ae1f57670703 more complete. Therefore we have 2 options here:
1) Apply this backport and sometime in the future also apply the reverts of both these commits with Paolo's commit which reverts them.
2) Don't apply this backport but do revert commit ae1f57670703.

-Liran