From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH] KVM: nVMX: remove incorrect vpid check in nested invvpid emulation Date: Wed, 25 Nov 2015 17:21:20 +0100 Message-ID: <5655E000.9000400@redhat.com> References: <1448443299-9375-1-git-send-email-haozhong.zhang@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, Gleb Natapov , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, linux-kernel@vger.kernel.org To: Bandan Das , Haozhong Zhang Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On 25/11/2015 16:45, Bandan Das wrote: > Haozhong Zhang 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 ? I think we don't want > L1 hypervisor to be able do a invvpid(0). The instruction simply "invalidates all mappings tagged with all non-zero VPIDs", which in our case is all L0 mappings tagged with vpid02. Paolo >> (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. > >> Signed-off-by: Haozhong Zhang >> --- >> 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. >