linux-kernel - Re: [PATCH] KVM: nVMX: remove incorrect vpid check in nested invvpid emulation

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: <20151125161834.GA22092@hzzhang-OptiPlex-9020.sh.intel.com>
Date:	Thu, 26 Nov 2015 00:18:34 +0800
From:	Haozhong Zhang <haozhong.zhang@...el.com>
To:	Bandan Das <bsd@...hat.com>
Cc:	<kvm@...r.kernel.org>, Gleb Natapov <gleb@...nel.org>,
	Paolo Bonzini <pbonzini@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>, <x86@...nel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] KVM: nVMX: remove incorrect vpid check in nested invvpid
 emulation

On 11/25/15 10:45, Bandan Das wrote:
> Haozhong Zhang <haozhong.zhang@...el.com> writes:
> 
> > This patch removes the vpid check when emulating nested invvpid
> > instruction of type all-contexts invalidation. The existing code is
> > incorrect because:
> >  (1) According to Intel SDM Vol 3, Section "INVVPID - Invalidate
> >      Translations Based on VPID", invvpid instruction does not check
> >      vpid in the invvpid descriptor when its type is all-contexts
> >      invalidation.
> 
> But iirc isn't vpid=0 reserved for root mode ?
Yes,

> I think we don't want
> L1 hypervisor to be able do a invvpid(0).

but the invvpid emulated here is doing the all-contexts invalidation that
does not use the given vpid and "invalidates all mappings tagged with all
VPIDs except VPID 0000H" (from Intel SDM).

> 
> >  (2) According to the same document, invvpid of type all-contexts
> >      invalidation does not require there is an active VMCS, so/and
> >      get_vmcs12() in the existing code may result in a NULL-pointer
> >      dereference. In practice, it can crash both KVM itself and L1
> >      hypervisors that use invvpid (e.g. Xen).
> 
> If that is the case, then just check if it's null and return without
> doing anything.

(according to Intel SDM) invvpid of type all-contexts invalidation
should not trigger a valid vmx fail if vpid in the current VMCS is 0.
However, this check and its following operation do change this semantics
in nested VMX, so it should be completely removed.

> 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@...el.com>
> > ---
> >  arch/x86/kvm/vmx.c | 5 -----
> >  1 file changed, 5 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index 87acc52..af823a3 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -7394,11 +7394,6 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
> >  
> >  	switch (type) {
> >  	case VMX_VPID_EXTENT_ALL_CONTEXT:
> > -		if (get_vmcs12(vcpu)->virtual_processor_id == 0) {
> > -			nested_vmx_failValid(vcpu,
> > -				VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);
> > -			return 1;
> > -		}
> >  		__vmx_flush_tlb(vcpu, to_vmx(vcpu)->nested.vpid02);
> >  		nested_vmx_succeed(vcpu);
> >  		break;
> 
> I also noticed a BUG() here in the default. It might be a good idea to replace
> it with a WARN.

Or, in nested_vmx_setup_ctls_msrs():

    if (enable_vpid)
-	vmx->nested.nested_vmx_vpid_caps = VMX_VPID_INVVPID_BIT |
-			VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT;
+       vmx->nested.nested_vmx_vpid_caps = VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT;

because the current handle_invvpid() only handles all-contexts invalidation.

Haozhong

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/