Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753038AbbKYQV2 (ORCPT ); Wed, 25 Nov 2015 11:21:28 -0500 Received: from mx1.redhat.com ([209.132.183.28]:54725 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752923AbbKYQV0 (ORCPT ); Wed, 25 Nov 2015 11:21:26 -0500 Subject: Re: [PATCH] KVM: nVMX: remove incorrect vpid check in nested invvpid emulation To: Bandan Das , Haozhong Zhang References: <1448443299-9375-1-git-send-email-haozhong.zhang@intel.com> Cc: kvm@vger.kernel.org, Gleb Natapov , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, linux-kernel@vger.kernel.org From: Paolo Bonzini Message-ID: <5655E000.9000400@redhat.com> Date: Wed, 25 Nov 2015 17:21:20 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2220 Lines: 60 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. > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/