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.
(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).
Signed-off-by: Haozhong Zhang <[email protected]>
---
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;
--
2.4.8
Haozhong Zhang <[email protected]> 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).
> (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 <[email protected]>
> ---
> 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.
On 11/25/15 10:45, Bandan Das wrote:
> Haozhong Zhang <[email protected]> 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 <[email protected]>
> > ---
> > 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
On 25/11/2015 16:45, Bandan Das wrote:
> Haozhong Zhang <[email protected]> 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 <[email protected]>
>> ---
>> 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.
>
Haozhong Zhang <[email protected]> writes:
> On 11/25/15 10:45, Bandan Das wrote:
>> Haozhong Zhang <[email protected]> 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 <[email protected]>
>> > ---
>> > 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html