KVM: nVMX: Verify eVMCS revision id match supported eVMCS version on eVMCS VMPTRLD

Commit Message

Liran Alon Oct. 31, 2018, 3:06 p.m. UTC

According to TLFS section 16.11.2 Enlightened VMCS, the first u32
field of eVMCS should specify eVMCS VersionNumber.

This version should be in the range of supported eVMCS versions exposed
to guest via CPUID.0x4000000A.EAX[0:15].
The range which KVM expose to guest in this CPUID field should be the
same as the value returned in vmcs_version by nested_enable_evmcs().

According to the above, eVMCS VMPTRLD should verify that version specified
in given eVMCS is in the supported range. However, current code
mistakenly verfies this field against VMCS12_REVISION.

One can also see that when KVM use eVMCS, it makes sure that
alloc_vmcs_cpu() sets allocated eVMCS revision_id to KVM_EVMCS_VERSION.

Reviewed-by: Nikita Leshenko <[e-mail]>
Reviewed-by: Mark Kanda <[e-mail]>
Signed-off-by: Liran Alon <[e-mail]>
---
 arch/x86/kvm/vmx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Vitaly Kuznetsov Oct. 31, 2018, 4:12 p.m. UTC | #1

Liran Alon <[e-mail]> writes:

> According to TLFS section 16.11.2 Enlightened VMCS, the first u32
> field of eVMCS should specify eVMCS VersionNumber.
>
> This version should be in the range of supported eVMCS versions exposed
> to guest via CPUID.0x4000000A.EAX[0:15].
> The range which KVM expose to guest in this CPUID field should be the
> same as the value returned in vmcs_version by nested_enable_evmcs().
>
> According to the above, eVMCS VMPTRLD should verify that version specified
> in given eVMCS is in the supported range. However, current code
> mistakenly verfies this field against VMCS12_REVISION.
>
> One can also see that when KVM use eVMCS, it makes sure that
> alloc_vmcs_cpu() sets allocated eVMCS revision_id to KVM_EVMCS_VERSION.
>
> Reviewed-by: Nikita Leshenko <[e-mail]>
> Reviewed-by: Mark Kanda <[e-mail]>
> Signed-off-by: Liran Alon <[e-mail]>
> ---
>  arch/x86/kvm/vmx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 4555077d69ce..36b7b6c64547 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -9369,7 +9369,7 @@ static int nested_vmx_handle_enlightened_vmptrld(struct kvm_vcpu *vcpu,
>  
>  		vmx->nested.hv_evmcs = kmap(vmx->nested.hv_evmcs_page);
>  
> -		if (vmx->nested.hv_evmcs->revision_id != VMCS12_REVISION) {
> +		if (vmx->nested.hv_evmcs->revision_id != KVM_EVMCS_VERSION) {
>  			nested_release_evmcs(vcpu);
>  			return 0;
>  		}

The patch made me wonder how it all worked before, I decided to give it
a try with Hyper-V on KVM and it fails to boot - and I think that was
the reason why VMCS12_REVISION is here. evmcs selftest also seems to
fail post-patch (no surprises).

I, however, see the reason behind your patch. Let me go back and refresh
my memory on what's going on.

Thanks,

Vitaly Kuznetsov Oct. 31, 2018, 5:41 p.m. UTC | #2

Vitaly Kuznetsov <[e-mail]> writes:

> Liran Alon <[e-mail]> writes:
>
>> According to TLFS section 16.11.2 Enlightened VMCS, the first u32
>> field of eVMCS should specify eVMCS VersionNumber.
>>
>> This version should be in the range of supported eVMCS versions exposed
>> to guest via CPUID.0x4000000A.EAX[0:15].
>> The range which KVM expose to guest in this CPUID field should be the
>> same as the value returned in vmcs_version by nested_enable_evmcs().
>>
>> According to the above, eVMCS VMPTRLD should verify that version specified
>> in given eVMCS is in the supported range. However, current code
>> mistakenly verfies this field against VMCS12_REVISION.
>>
>> One can also see that when KVM use eVMCS, it makes sure that
>> alloc_vmcs_cpu() sets allocated eVMCS revision_id to KVM_EVMCS_VERSION.
>>
>> Reviewed-by: Nikita Leshenko <[e-mail]>
>> Reviewed-by: Mark Kanda <[e-mail]>
>> Signed-off-by: Liran Alon <[e-mail]>
>> ---
>>  arch/x86/kvm/vmx.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 4555077d69ce..36b7b6c64547 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -9369,7 +9369,7 @@ static int nested_vmx_handle_enlightened_vmptrld(struct kvm_vcpu *vcpu,
>>  
>>  		vmx->nested.hv_evmcs = kmap(vmx->nested.hv_evmcs_page);
>>  
>> -		if (vmx->nested.hv_evmcs->revision_id != VMCS12_REVISION) {
>> +		if (vmx->nested.hv_evmcs->revision_id != KVM_EVMCS_VERSION) {
>>  			nested_release_evmcs(vcpu);
>>  			return 0;
>>  		}
>
> The patch made me wonder how it all worked before, I decided to give it
> a try with Hyper-V on KVM and it fails to boot - and I think that was
> the reason why VMCS12_REVISION is here. evmcs selftest also seems to
> fail post-patch (no surprises).
>
> I, however, see the reason behind your patch. Let me go back and refresh
> my memory on what's going on.

It seems genuine Hyper-V always put VMCS revision from
IA32_VMX_BASIC_MSR(0x480) to eVMCS regardless of what's in
CPUID.4000000A. This all works just because when we run on genuine
Hyper-V we see revision '1' in both. I don't think we would want to
adjust IA32_VMX_BASIC_MSR in KVM to be '1' - as using Enlightened VMCS
is just an option, we have no idea if the guest is going to use it.

So I think the patch needs to change the other side of KVM to make its
behavior match Hyper-V's. Sigh.

Liran Alon Oct. 31, 2018, 6:58 p.m. UTC | #3

> On 31 Oct 2018, at 19:41, Vitaly Kuznetsov <[e-mail]> wrote:
> 
> Vitaly Kuznetsov <[e-mail]> writes:
> 
>> Liran Alon <[e-mail]> writes:
>> 
>>> According to TLFS section 16.11.2 Enlightened VMCS, the first u32
>>> field of eVMCS should specify eVMCS VersionNumber.
>>> 
>>> This version should be in the range of supported eVMCS versions exposed
>>> to guest via CPUID.0x4000000A.EAX[0:15].
>>> The range which KVM expose to guest in this CPUID field should be the
>>> same as the value returned in vmcs_version by nested_enable_evmcs().
>>> 
>>> According to the above, eVMCS VMPTRLD should verify that version specified
>>> in given eVMCS is in the supported range. However, current code
>>> mistakenly verfies this field against VMCS12_REVISION.
>>> 
>>> One can also see that when KVM use eVMCS, it makes sure that
>>> alloc_vmcs_cpu() sets allocated eVMCS revision_id to KVM_EVMCS_VERSION.
>>> 
>>> Reviewed-by: Nikita Leshenko <[e-mail]>
>>> Reviewed-by: Mark Kanda <[e-mail]>
>>> Signed-off-by: Liran Alon <[e-mail]>
>>> ---
>>> arch/x86/kvm/vmx.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>> index 4555077d69ce..36b7b6c64547 100644
>>> --- a/arch/x86/kvm/vmx.c
>>> +++ b/arch/x86/kvm/vmx.c
>>> @@ -9369,7 +9369,7 @@ static int nested_vmx_handle_enlightened_vmptrld(struct kvm_vcpu *vcpu,
>>> 
>>> 		vmx->nested.hv_evmcs = kmap(vmx->nested.hv_evmcs_page);
>>> 
>>> -		if (vmx->nested.hv_evmcs->revision_id != VMCS12_REVISION) {
>>> +		if (vmx->nested.hv_evmcs->revision_id != KVM_EVMCS_VERSION) {
>>> 			nested_release_evmcs(vcpu);
>>> 			return 0;
>>> 		}
>> 
>> The patch made me wonder how it all worked before, I decided to give it
>> a try with Hyper-V on KVM and it fails to boot - and I think that was
>> the reason why VMCS12_REVISION is here. evmcs selftest also seems to
>> fail post-patch (no surprises).
>> 
>> I, however, see the reason behind your patch. Let me go back and refresh
>> my memory on what's going on.
> 
> It seems genuine Hyper-V always put VMCS revision from
> IA32_VMX_BASIC_MSR(0x480) to eVMCS regardless of what's in
> CPUID.4000000A. This all works just because when we run on genuine
> Hyper-V we see revision '1' in both. I don't think we would want to
> adjust IA32_VMX_BASIC_MSR in KVM to be '1' - as using Enlightened VMCS
> is just an option, we have no idea if the guest is going to use it.
> 
> So I think the patch needs to change the other side of KVM to make its
> behavior match Hyper-V's. Sigh.
> 
> -- 
> Vitaly

Hmm… Vitaly, maybe it is a good idea that you will Cc some Microsoft guys you know on this thread?

It seems to me from what you describe that Hyper-V has a bug of using revision_id specified in IA32_VMX_BASIC_MSR
instead of using one of the eVMCS supported versions reported in CPUID.4000000A.

I’m also not sure I understand what you suggest as “change the other side of KVM to make it’s behaviour match Hyper-V”.
Do you mean that we will change KVM usage of eVMCS such that alloc_vmcs_cpu() will always set vmcs->hdr.revision_id to vmcs_config.revision_id?

-Liran

Vitaly Kuznetsov Oct. 31, 2018, 9:59 p.m. UTC | #4

Liran Alon <[e-mail]> writes:

>> On 31 Oct 2018, at 19:41, Vitaly Kuznetsov <[e-mail]> wrote:
>> 
>> Vitaly Kuznetsov <[e-mail]> writes:
>> 
>>> Liran Alon <[e-mail]> writes:
>>> 
>>>> According to TLFS section 16.11.2 Enlightened VMCS, the first u32
>>>> field of eVMCS should specify eVMCS VersionNumber.
>>>> 
>>>> This version should be in the range of supported eVMCS versions exposed
>>>> to guest via CPUID.0x4000000A.EAX[0:15].
>>>> The range which KVM expose to guest in this CPUID field should be the
>>>> same as the value returned in vmcs_version by nested_enable_evmcs().
>>>> 
>>>> According to the above, eVMCS VMPTRLD should verify that version specified
>>>> in given eVMCS is in the supported range. However, current code
>>>> mistakenly verfies this field against VMCS12_REVISION.
>>>> 
>>>> One can also see that when KVM use eVMCS, it makes sure that
>>>> alloc_vmcs_cpu() sets allocated eVMCS revision_id to KVM_EVMCS_VERSION.
>>>> 
>>>> Reviewed-by: Nikita Leshenko <[e-mail]>
>>>> Reviewed-by: Mark Kanda <[e-mail]>
>>>> Signed-off-by: Liran Alon <[e-mail]>
>>>> ---
>>>> arch/x86/kvm/vmx.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>> 
>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>> index 4555077d69ce..36b7b6c64547 100644
>>>> --- a/arch/x86/kvm/vmx.c
>>>> +++ b/arch/x86/kvm/vmx.c
>>>> @@ -9369,7 +9369,7 @@ static int nested_vmx_handle_enlightened_vmptrld(struct kvm_vcpu *vcpu,
>>>> 
>>>> 		vmx->nested.hv_evmcs = kmap(vmx->nested.hv_evmcs_page);
>>>> 
>>>> -		if (vmx->nested.hv_evmcs->revision_id != VMCS12_REVISION) {
>>>> +		if (vmx->nested.hv_evmcs->revision_id != KVM_EVMCS_VERSION) {
>>>> 			nested_release_evmcs(vcpu);
>>>> 			return 0;
>>>> 		}
>>> 
>>> The patch made me wonder how it all worked before, I decided to give it
>>> a try with Hyper-V on KVM and it fails to boot - and I think that was
>>> the reason why VMCS12_REVISION is here. evmcs selftest also seems to
>>> fail post-patch (no surprises).
>>> 
>>> I, however, see the reason behind your patch. Let me go back and refresh
>>> my memory on what's going on.
>> 
>> It seems genuine Hyper-V always put VMCS revision from
>> IA32_VMX_BASIC_MSR(0x480) to eVMCS regardless of what's in
>> CPUID.4000000A. This all works just because when we run on genuine
>> Hyper-V we see revision '1' in both. I don't think we would want to
>> adjust IA32_VMX_BASIC_MSR in KVM to be '1' - as using Enlightened VMCS
>> is just an option, we have no idea if the guest is going to use it.
>> 
>> So I think the patch needs to change the other side of KVM to make its
>> behavior match Hyper-V's. Sigh.
>> 
>> -- 
>> Vitaly
>
> Hmm… Vitaly, maybe it is a good idea that you will Cc some Microsoft
> guys you know on this thread?

Good idea. We, however, need their Hyper-V host team and not Linux
folks.

>
> It seems to me from what you describe that Hyper-V has a bug of using revision_id specified in IA32_VMX_BASIC_MSR
> instead of using one of the eVMCS supported versions reported in CPUID.4000000A.
>
> I’m also not sure I understand what you suggest as “change the other side of KVM to make it’s behaviour match Hyper-V”.
> Do you mean that we will change KVM usage of eVMCS such that
> alloc_vmcs_cpu() will always set vmcs->hdr.revision_id to
> vmcs_config.revision_id?

Yes. My current understanding (correct me if I'm wrong!) is that both
KVM-on-Hyper-V and Hyper-V-on-KVM work well with Enlightened
VMCS. Surprisingly, KVM-on-KVM doesn't - and this is because of the
issue you're trying to address by the patch.

Liran Alon Oct. 31, 2018, 11:36 p.m. UTC | #5

>Vitaly Kuznetsov <[e-mail]> writes:

>>Liran Alon <[e-mail]> writes:

>> It seems to me from what you describe that Hyper-V has a bug of using revision_id specified in IA32_VMX_BASIC_MSR
>> instead of using one of the eVMCS supported versions reported in CPUID.4000000A.
>>
>> I’m also not sure I understand what you suggest as “change the other side of KVM to make it’s behaviour match Hyper-V”.
>> Do you mean that we will change KVM usage of eVMCS such that
>> alloc_vmcs_cpu() will always set vmcs->hdr.revision_id to
>> vmcs_config.revision_id?

> Yes. My current understanding (correct me if I'm wrong!) is that both
> KVM-on-Hyper-V and Hyper-V-on-KVM work well with Enlightened
> VMCS. Surprisingly, KVM-on-KVM doesn't - and this is because of the
> issue you're trying to address by the patch.

Yes you understand correctly.

Well, I can change alloc_vmcs_cpu() to always set vmcs->hdr.revison_id to vmcs_config.revision_id even when eVMCS is enabled
(which is a deliberate wrong behaviour of KVM eVMCS client code) and also not apply this patch (which remains a deliberate wrong behaviour of KVM eVMCS server code).
All of this to overcome Hyper-V wrong behaviour :)

Paolo, do you think we should insert this wrong behaviour to KVM to support running on top of buggy Hyper-V? I unfortunately believe we should.
If so, should I insert this weird behaviour behind a kvm-intel module parameter? E.g. compact_evmcs_version?

-Liran

Vitaly Kuznetsov Nov. 1, 2018, 7:30 a.m. UTC | #6

Liran Alon <[e-mail]> writes:

>>Vitaly Kuznetsov <[e-mail]> writes:
>
>>>Liran Alon <[e-mail]> writes:
>
>>> It seems to me from what you describe that Hyper-V has a bug of using revision_id specified in IA32_VMX_BASIC_MSR
>>> instead of using one of the eVMCS supported versions reported in CPUID.4000000A.
>>>
>>> I’m also not sure I understand what you suggest as “change the other side of KVM to make it’s behaviour match Hyper-V”.
>>> Do you mean that we will change KVM usage of eVMCS such that
>>> alloc_vmcs_cpu() will always set vmcs->hdr.revision_id to
>>> vmcs_config.revision_id?
>
>> Yes. My current understanding (correct me if I'm wrong!) is that both
>> KVM-on-Hyper-V and Hyper-V-on-KVM work well with Enlightened
>> VMCS. Surprisingly, KVM-on-KVM doesn't - and this is because of the
>> issue you're trying to address by the patch.
>
> Yes you understand correctly.
>
> Well, I can change alloc_vmcs_cpu() to always set vmcs->hdr.revison_id to vmcs_config.revision_id even when eVMCS is enabled
> (which is a deliberate wrong behaviour of KVM eVMCS client code) and also not apply this patch (which remains a deliberate wrong behaviour of KVM eVMCS server code).
> All of this to overcome Hyper-V wrong behaviour :)

Alternatively we can just drop the check in
nested_vmx_handle_enlightened_vmptrld() for now: The only supported
eVMCS version - both by Hyper-V and KVM is '1' and we explicitly
advertise that in  CPUID.4000000A so even when there's a new Hyper-V
version supporting e.g. eVMCS2 it won't be used. Microsoft will also
have to fix the issue when this happens - or how else would they know
which eVMCS version is in use by the guest if they always copy
IA32_VMX_BASIC_MSR?

>
> Paolo, do you think we should insert this wrong behaviour to KVM to support running on top of buggy Hyper-V? I unfortunately believe we should.
> If so, should I insert this weird behaviour behind a kvm-intel module
> parameter? E.g. compact_evmcs_version?

Not Paolo but I'd say this is an overkill. And, even if we decide to go
down this road, I think that the default KVM configuration should
support real world Hyper-V so the module parameter should rather be
'correct_evmcs_versioning' :-)

Liran Alon Nov. 1, 2018, 8:28 a.m. UTC | #7

> On 1 Nov 2018, at 9:30, Vitaly Kuznetsov <[e-mail]> wrote:
> 
> Liran Alon <[e-mail]> writes:
> 
>>> Vitaly Kuznetsov <[e-mail]> writes:
>> 
>>>> Liran Alon <[e-mail]> writes:
>> 
>>>> It seems to me from what you describe that Hyper-V has a bug of using revision_id specified in IA32_VMX_BASIC_MSR
>>>> instead of using one of the eVMCS supported versions reported in CPUID.4000000A.
>>>> 
>>>> I’m also not sure I understand what you suggest as “change the other side of KVM to make it’s behaviour match Hyper-V”.
>>>> Do you mean that we will change KVM usage of eVMCS such that
>>>> alloc_vmcs_cpu() will always set vmcs->hdr.revision_id to
>>>> vmcs_config.revision_id?
>> 
>>> Yes. My current understanding (correct me if I'm wrong!) is that both
>>> KVM-on-Hyper-V and Hyper-V-on-KVM work well with Enlightened
>>> VMCS. Surprisingly, KVM-on-KVM doesn't - and this is because of the
>>> issue you're trying to address by the patch.
>> 
>> Yes you understand correctly.
>> 
>> Well, I can change alloc_vmcs_cpu() to always set vmcs->hdr.revison_id to vmcs_config.revision_id even when eVMCS is enabled
>> (which is a deliberate wrong behaviour of KVM eVMCS client code) and also not apply this patch (which remains a deliberate wrong behaviour of KVM eVMCS server code).
>> All of this to overcome Hyper-V wrong behaviour :)
> 
> Alternatively we can just drop the check in
> nested_vmx_handle_enlightened_vmptrld() for now: The only supported
> eVMCS version - both by Hyper-V and KVM is '1' and we explicitly
> advertise that in  CPUID.4000000A so even when there's a new Hyper-V
> version supporting e.g. eVMCS2 it won't be used. Microsoft will also
> have to fix the issue when this happens - or how else would they know
> which eVMCS version is in use by the guest if they always copy
> IA32_VMX_BASIC_MSR?

Agree.
I will submit a patch that just checks if given hv_evmcs->version_number is either KVM_EVMCS_VERSION or VMCS12_REVISION
and will document properly why we do that… I think it’s better than just dropping the check.

> 
>> 
>> Paolo, do you think we should insert this wrong behaviour to KVM to support running on top of buggy Hyper-V? I unfortunately believe we should.
>> If so, should I insert this weird behaviour behind a kvm-intel module
>> parameter? E.g. compact_evmcs_version?
> 
> Not Paolo but I'd say this is an overkill. And, even if we decide to go
> down this road, I think that the default KVM configuration should
> support real world Hyper-V so the module parameter should rather be
> 'correct_evmcs_versioning' :-)

I agree this seems unnecessary given your suggestion.

Thanks!
-Liran

> 
> -- 
> Vitaly

Patch

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 4555077d69ce..36b7b6c64547 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -9369,7 +9369,7 @@  static int nested_vmx_handle_enlightened_vmptrld(struct kvm_vcpu *vcpu,
 
 		vmx->nested.hv_evmcs = kmap(vmx->nested.hv_evmcs_page);
 
-		if (vmx->nested.hv_evmcs->revision_id != VMCS12_REVISION) {
+		if (vmx->nested.hv_evmcs->revision_id != KVM_EVMCS_VERSION) {
 			nested_release_evmcs(vcpu);
 			return 0;
 		}