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

[Bandan Das]

Haozhong Zhang <haozhong.zhang@xxxxxxxxx> writes:

> On 11/25/15 10:45, Bandan Das wrote:
>> Haozhong Zhang <haozhong.zhang@xxxxxxxxx> 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).

Actually, vpid_sync_context will always try single invalidation first and
I was concerned that we will end up calling vpid_sync_context(0). But that
will not happen since nested.vpid02 is always allocated a vpid.
So... we are good :)

>> 
>> >  (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.

No, I meant just skip instruction and return but I doubt if there's
any overhead of flushing mappings that don't exist in the first place.
Anyway, better to do as the spec says.

> 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@xxxxxxxxx>
>> > ---
>> >  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 kvm" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/