[PATCH 0/7 v7] KVM: nSVM: Check addresses of MSR bitmap and IO bitmap tables on vmrun of nested guests

[PATCH 0/7 v7] KVM: nSVM: Check addresses of MSR bitmap and IO bitmap tables on vmrun of nested guests

[Krish Sadhukhan]

v6 -> v7:
	1. In patch# 4, the logic in nested_svm_check_bitmap_pa() has been
	   fixed. The size of IOPM passed to nested_svm_check_bitmap_pa() has
	   also been corrected. Indentation has been improved.
	2. In patch# 1, the names of the #defines have been changed.
	3. In patch# 2, a new exit code is defined to differentiate between
	   consistency failures and failures after switching to guest mode,
	   because tests need to know the exact failure instead of
	   SVM_EXIT_ERR. This exit code is similar to what nVMX does and
	   appears to be the best solution to differentiate the above-mentioned
	   error scenarios.
	4. In patch# 3, code that unset bit 11:0 of the MSRPm and IOPM tables,
	   has been removed because hardware doesn't care about the value
	   these bits. Also, tests need to verify hardware behavior. So if
	   these bits are unset, the checks in nested_svm_check_bitmap_pa()
	   do not work as expected.
	5. In patch# 7, additional test cases have been added.


[PATCH 1/7 v7] KVM: SVM: Define actual size of IOPM and MSRPM tables
[PATCH 2/7 v7] KVM: nSVM: Define an exit code to reflect consistency
[PATCH 3/7 v7] KVM: nSVM: No need to set bits 11:0 in MSRPM and IOPM
[PATCH 4/7 v7] nSVM: Check addresses of MSR and IO permission maps
[PATCH 5/7 v7] SVM: Use ALIGN macro when aligning 'io_bitmap_area'
[PATCH 6/7 v7] nSVM: Define an exit code to reflect consistency check
[PATCH 7/7 v7] nSVM: Test addresses of MSR and IO permissions maps

 arch/x86/include/uapi/asm/svm.h |  1 +
 arch/x86/kvm/svm/nested.c       | 29 +++++++++++++++++++++++------
 arch/x86/kvm/svm/svm.c          | 20 ++++++++++----------
 arch/x86/kvm/svm/svm.h          |  3 +++
 4 files changed, 37 insertions(+), 16 deletions(-)

Krish Sadhukhan (4):
      KVM: SVM: Define actual size of IOPM and MSRPM tables
      KVM: nSVM: Define an exit code to reflect consistency check failure
      KVM: nSVM: No need to set bits 11:0 in MSRPM and IOPM bitmaps
      nSVM: Check addresses of MSR and IO permission maps

 x86/svm.c       |  2 +-
 x86/svm.h       |  1 +
 x86/svm_tests.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 79 insertions(+), 2 deletions(-)

Krish Sadhukhan (3):
      SVM: Use ALIGN macro when aligning 'io_bitmap_area'
      nSVM: Define an exit code to reflect consistency check failure
      nSVM: Test addresses of MSR and IO permissions maps

[PATCH 1/7 v7] KVM: SVM: Define actual size of IOPM and MSRPM tables

[Krish Sadhukhan]

Define the actual size of the IOPM and MSRPM tables so that the actual size
can be used when initializing them and when checking the consistency of their
physical address.
These #defines are placed in svm.h so that they can be shared.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 arch/x86/kvm/svm/svm.c | 20 ++++++++++----------
 arch/x86/kvm/svm/svm.h |  3 +++
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 48b396f33bee..0fe60095ab67 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -56,9 +56,6 @@ static const struct x86_cpu_id svm_cpu_id[] = {
 MODULE_DEVICE_TABLE(x86cpu, svm_cpu_id);
 #endif
 
-#define IOPM_ALLOC_ORDER 2
-#define MSRPM_ALLOC_ORDER 1
-
 #define SEG_TYPE_LDT 2
 #define SEG_TYPE_BUSY_TSS16 3
 
@@ -683,14 +680,15 @@ void set_msr_interception(struct kvm_vcpu *vcpu, u32 *msrpm, u32 msr,
 
 u32 *svm_vcpu_alloc_msrpm(void)
 {
-	struct page *pages = alloc_pages(GFP_KERNEL_ACCOUNT, MSRPM_ALLOC_ORDER);
+	unsigned int order = get_order(MSRPM_SIZE);
+	struct page *pages = alloc_pages(GFP_KERNEL_ACCOUNT, order);
 	u32 *msrpm;
 
 	if (!pages)
 		return NULL;
 
 	msrpm = page_address(pages);
-	memset(msrpm, 0xff, PAGE_SIZE * (1 << MSRPM_ALLOC_ORDER));
+	memset(msrpm, 0xff, PAGE_SIZE * (1 << order));
 
 	return msrpm;
 }
@@ -709,7 +707,7 @@ void svm_vcpu_init_msrpm(struct kvm_vcpu *vcpu, u32 *msrpm)
 
 void svm_vcpu_free_msrpm(u32 *msrpm)
 {
-	__free_pages(virt_to_page(msrpm), MSRPM_ALLOC_ORDER);
+	__free_pages(virt_to_page(msrpm), get_order(MSRPM_SIZE));
 }
 
 static void svm_msr_filter_changed(struct kvm_vcpu *vcpu)
@@ -896,7 +894,8 @@ static void svm_hardware_teardown(void)
 	for_each_possible_cpu(cpu)
 		svm_cpu_uninit(cpu);
 
-	__free_pages(pfn_to_page(iopm_base >> PAGE_SHIFT), IOPM_ALLOC_ORDER);
+	__free_pages(pfn_to_page(iopm_base >> PAGE_SHIFT),
+	get_order(IOPM_SIZE));
 	iopm_base = 0;
 }
 
@@ -932,14 +931,15 @@ static __init int svm_hardware_setup(void)
 	struct page *iopm_pages;
 	void *iopm_va;
 	int r;
+	unsigned int order = get_order(IOPM_SIZE);
 
-	iopm_pages = alloc_pages(GFP_KERNEL, IOPM_ALLOC_ORDER);
+	iopm_pages = alloc_pages(GFP_KERNEL, order);
 
 	if (!iopm_pages)
 		return -ENOMEM;
 
 	iopm_va = page_address(iopm_pages);
-	memset(iopm_va, 0xff, PAGE_SIZE * (1 << IOPM_ALLOC_ORDER));
+	memset(iopm_va, 0xff, PAGE_SIZE * (1 << order));
 	iopm_base = page_to_pfn(iopm_pages) << PAGE_SHIFT;
 
 	init_msrpm_offsets();
@@ -1425,7 +1425,7 @@ static void svm_free_vcpu(struct kvm_vcpu *vcpu)
 	sev_free_vcpu(vcpu);
 
 	__free_page(pfn_to_page(__sme_clr(svm->vmcb01.pa) >> PAGE_SHIFT));
-	__free_pages(virt_to_page(svm->msrpm), MSRPM_ALLOC_ORDER);
+	__free_pages(virt_to_page(svm->msrpm), get_order(MSRPM_SIZE));
 }
 
 static void svm_prepare_guest_switch(struct kvm_vcpu *vcpu)
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 02f8ece8c741..b07765c278fa 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -28,6 +28,9 @@ static const u32 host_save_user_msrs[] = {
 };
 #define NR_HOST_SAVE_USER_MSRS ARRAY_SIZE(host_save_user_msrs)
 
+#define	IOPM_SIZE PAGE_SIZE * 3
+#define	MSRPM_SIZE PAGE_SIZE * 2
+
 #define MAX_DIRECT_ACCESS_MSRS	20
 #define MSRPM_OFFSETS	16
 extern u32 msrpm_offsets[MSRPM_OFFSETS] __read_mostly;
-- 
2.27.0

[PATCH 2/7 v7] KVM: nSVM: Define an exit code to reflect consistency check failure

[Krish Sadhukhan]

nested_svm_vmrun() returns SVM_EXIT_ERR both when consistency checks for
MSRPM fail and when merging the MSRPM of vmcb12 with that of KVM fails. These
two failures are different in that the first one happens during consistency
checking while the second happens after consistency checking passes and after
guest mode switch is done. In order to differentiate between the two types of
error conditions, define an exit code that can be used to denote consistency
check failures. This new exit code is similar to what nVMX uses to denote
consistency check failures. For nSVM, we will use the highest bit in the high
part of the EXIT_CODE field.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 arch/x86/include/uapi/asm/svm.h | 1 +
 arch/x86/kvm/svm/nested.c       | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/uapi/asm/svm.h b/arch/x86/include/uapi/asm/svm.h
index 554f75fe013c..b0a6550a23f5 100644
--- a/arch/x86/include/uapi/asm/svm.h
+++ b/arch/x86/include/uapi/asm/svm.h
@@ -111,6 +111,7 @@
 #define SVM_VMGEXIT_UNSUPPORTED_EVENT		0x8000ffff
 
 #define SVM_EXIT_ERR           -1
+#define	SVM_CONSISTENCY_ERR    1 << 31
 
 #define SVM_EXIT_REASONS \
 	{ SVM_EXIT_READ_CR0,    "read_cr0" }, \
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 8453c898b68b..ae53ae46ebca 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -606,7 +606,7 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu)
 	if (!nested_vmcb_valid_sregs(vcpu, &vmcb12->save) ||
 	    !nested_vmcb_check_controls(&svm->nested.ctl)) {
 		vmcb12->control.exit_code    = SVM_EXIT_ERR;
-		vmcb12->control.exit_code_hi = 0;
+		vmcb12->control.exit_code_hi = SVM_CONSISTENCY_ERR;
 		vmcb12->control.exit_info_1  = 0;
 		vmcb12->control.exit_info_2  = 0;
 		goto out;
-- 
2.27.0

Re: [PATCH 2/7 v7] KVM: nSVM: Define an exit code to reflect consistency check failure

[Paolo Bonzini]

On 12/04/21 23:56, Krish Sadhukhan wrote:
> nested_svm_vmrun() returns SVM_EXIT_ERR both when consistency checks for
> MSRPM fail and when merging the MSRPM of vmcb12 with that of KVM fails. These
> two failures are different in that the first one happens during consistency
> checking while the second happens after consistency checking passes and after
> guest mode switch is done. In order to differentiate between the two types of
> error conditions, define an exit code that can be used to denote consistency
> check failures. This new exit code is similar to what nVMX uses to denote
> consistency check failures. For nSVM, we will use the highest bit in the high
> part of the EXIT_CODE field.
> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>

The exit code is defined by AMD, we cannot change it.

Paolo

> ---
>   arch/x86/include/uapi/asm/svm.h | 1 +
>   arch/x86/kvm/svm/nested.c       | 2 +-
>   2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/uapi/asm/svm.h b/arch/x86/include/uapi/asm/svm.h
> index 554f75fe013c..b0a6550a23f5 100644
> --- a/arch/x86/include/uapi/asm/svm.h
> +++ b/arch/x86/include/uapi/asm/svm.h
> @@ -111,6 +111,7 @@
>   #define SVM_VMGEXIT_UNSUPPORTED_EVENT		0x8000ffff
>   
>   #define SVM_EXIT_ERR           -1
> +#define	SVM_CONSISTENCY_ERR    1 << 31
>   
>   #define SVM_EXIT_REASONS \
>   	{ SVM_EXIT_READ_CR0,    "read_cr0" }, \
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 8453c898b68b..ae53ae46ebca 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -606,7 +606,7 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu)
>   	if (!nested_vmcb_valid_sregs(vcpu, &vmcb12->save) ||
>   	    !nested_vmcb_check_controls(&svm->nested.ctl)) {
>   		vmcb12->control.exit_code    = SVM_EXIT_ERR;
> -		vmcb12->control.exit_code_hi = 0;
> +		vmcb12->control.exit_code_hi = SVM_CONSISTENCY_ERR;
>   		vmcb12->control.exit_info_1  = 0;
>   		vmcb12->control.exit_info_2  = 0;
>   		goto out;
> 

Re: [PATCH 2/7 v7] KVM: nSVM: Define an exit code to reflect consistency check failure

[Krish Sadhukhan]

On 4/17/21 7:17 AM, Paolo Bonzini wrote:
> On 12/04/21 23:56, Krish Sadhukhan wrote:
>> nested_svm_vmrun() returns SVM_EXIT_ERR both when consistency checks for
>> MSRPM fail and when merging the MSRPM of vmcb12 with that of KVM 
>> fails. These
>> two failures are different in that the first one happens during 
>> consistency
>> checking while the second happens after consistency checking passes 
>> and after
>> guest mode switch is done. In order to differentiate between the two 
>> types of
>> error conditions, define an exit code that can be used to denote 
>> consistency
>> check failures. This new exit code is similar to what nVMX uses to 
>> denote
>> consistency check failures. For nSVM, we will use the highest bit in 
>> the high
>> part of the EXIT_CODE field.
>>
>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>
> The exit code is defined by AMD, we cannot change it.


The reason why I thought of this is that SVM implementation uses only 
the lower half, as all AMD-defined exit code are handled therein only. 
Is this still going to cause an issue ?

>
> Paolo
>
>> ---
>>   arch/x86/include/uapi/asm/svm.h | 1 +
>>   arch/x86/kvm/svm/nested.c       | 2 +-
>>   2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/include/uapi/asm/svm.h 
>> b/arch/x86/include/uapi/asm/svm.h
>> index 554f75fe013c..b0a6550a23f5 100644
>> --- a/arch/x86/include/uapi/asm/svm.h
>> +++ b/arch/x86/include/uapi/asm/svm.h
>> @@ -111,6 +111,7 @@
>>   #define SVM_VMGEXIT_UNSUPPORTED_EVENT        0x8000ffff
>>     #define SVM_EXIT_ERR           -1
>> +#define    SVM_CONSISTENCY_ERR    1 << 31
>>     #define SVM_EXIT_REASONS \
>>       { SVM_EXIT_READ_CR0,    "read_cr0" }, \
>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>> index 8453c898b68b..ae53ae46ebca 100644
>> --- a/arch/x86/kvm/svm/nested.c
>> +++ b/arch/x86/kvm/svm/nested.c
>> @@ -606,7 +606,7 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu)
>>       if (!nested_vmcb_valid_sregs(vcpu, &vmcb12->save) ||
>>           !nested_vmcb_check_controls(&svm->nested.ctl)) {
>>           vmcb12->control.exit_code    = SVM_EXIT_ERR;
>> -        vmcb12->control.exit_code_hi = 0;
>> +        vmcb12->control.exit_code_hi = SVM_CONSISTENCY_ERR;
>>           vmcb12->control.exit_info_1  = 0;
>>           vmcb12->control.exit_info_2  = 0;
>>           goto out;
>>
>

Re: [PATCH 2/7 v7] KVM: nSVM: Define an exit code to reflect consistency check failure

[Paolo Bonzini]

On 19/04/21 19:57, Krish Sadhukhan wrote:
> The reason why I thought of this is that SVM implementation uses only 
> the lower half, as all AMD-defined exit code are handled therein only. 
> Is this still going to cause an issue ?

I would have to check what happens on bare metal, but VMEXIT_INVALID is 
defined as "-1", not "FFFFFFFFh", so I think it should use the high 32 
bits (in which case KVM is wrong in not storing the high 32 bits).

Paolo

Re: [PATCH 2/7 v7] KVM: nSVM: Define an exit code to reflect consistency check failure

[Jim Mattson]

On Mon, Apr 19, 2021 at 11:28 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 19/04/21 19:57, Krish Sadhukhan wrote:
> > The reason why I thought of this is that SVM implementation uses only
> > the lower half, as all AMD-defined exit code are handled therein only.
> > Is this still going to cause an issue ?
>
> I would have to check what happens on bare metal, but VMEXIT_INVALID is
> defined as "-1", not "FFFFFFFFh", so I think it should use the high 32
> bits (in which case KVM is wrong in not storing the high 32 bits).

And VMEXIT_BUSY is -2.

[PATCH 3/7 v7] KVM: nSVM: No need to set bits 11:0 in MSRPM and IOPM bitmaps

[Krish Sadhukhan]

According to APM vol 2, hardware ignores the low 12 bits in MSRPM and IOPM
bitmaps. Therefore setting/unssetting these bits has no effect as far as
VMRUN is concerned. Also, setting/unsetting these bits prevents tests from
verifying hardware behavior.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 arch/x86/kvm/svm/nested.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index ae53ae46ebca..fd42c8b7f99a 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -287,8 +287,6 @@ static void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
 
 	/* Copy it here because nested_svm_check_controls will check it.  */
 	svm->nested.ctl.asid           = control->asid;
-	svm->nested.ctl.msrpm_base_pa &= ~0x0fffULL;
-	svm->nested.ctl.iopm_base_pa  &= ~0x0fffULL;
 }
 
 /*
-- 
2.27.0

Re: [PATCH 3/7 v7] KVM: nSVM: No need to set bits 11:0 in MSRPM and IOPM bitmaps

[Paolo Bonzini]

On 12/04/21 23:56, Krish Sadhukhan wrote:
> According to APM vol 2, hardware ignores the low 12 bits in MSRPM and IOPM
> bitmaps. Therefore setting/unssetting these bits has no effect as far as
> VMRUN is concerned. Also, setting/unsetting these bits prevents tests from
> verifying hardware behavior.

Tests should only verify KVM behavior, in order to verify hardware 
behavior you have to run them on bare metal.

Still, the patch is okay.

Paolo

> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> ---
>   arch/x86/kvm/svm/nested.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index ae53ae46ebca..fd42c8b7f99a 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -287,8 +287,6 @@ static void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
>   
>   	/* Copy it here because nested_svm_check_controls will check it.  */
>   	svm->nested.ctl.asid           = control->asid;
> -	svm->nested.ctl.msrpm_base_pa &= ~0x0fffULL;
> -	svm->nested.ctl.iopm_base_pa  &= ~0x0fffULL;
>   }
>   
>   /*
> 

Re: [PATCH 3/7 v7] KVM: nSVM: No need to set bits 11:0 in MSRPM and IOPM bitmaps

[Sean Christopherson]

On Mon, Apr 12, 2021, Krish Sadhukhan wrote:
> According to APM vol 2, hardware ignores the low 12 bits in MSRPM and IOPM
> bitmaps. Therefore setting/unssetting these bits has no effect as far as
> VMRUN is concerned. Also, setting/unsetting these bits prevents tests from
> verifying hardware behavior.
> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> ---
>  arch/x86/kvm/svm/nested.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index ae53ae46ebca..fd42c8b7f99a 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -287,8 +287,6 @@ static void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
>  
>  	/* Copy it here because nested_svm_check_controls will check it.  */
>  	svm->nested.ctl.asid           = control->asid;
> -	svm->nested.ctl.msrpm_base_pa &= ~0x0fffULL;
> -	svm->nested.ctl.iopm_base_pa  &= ~0x0fffULL;

This will break nested_svm_vmrun_msrpm() if L1 passes an unaligned address.
The shortlog is also wrong, KVM isn't setting bits, it's clearing bits.

I also don't think svm->nested.ctl.msrpm_base_pa makes its way to hardware; IIUC,
it's a copy of vmcs12->control.msrpm_base_pa.  The bitmap that gets loaded into
the "real" VMCB is vmcb02->control.msrpm_base_pa.

>  }
>  
>  /*
> -- 
> 2.27.0
> 

Re: [PATCH 3/7 v7] KVM: nSVM: No need to set bits 11:0 in MSRPM and IOPM bitmaps

[Krish Sadhukhan]

On 4/20/21 1:00 PM, Sean Christopherson wrote:
> On Mon, Apr 12, 2021, Krish Sadhukhan wrote:
>> According to APM vol 2, hardware ignores the low 12 bits in MSRPM and IOPM
>> bitmaps. Therefore setting/unssetting these bits has no effect as far as
>> VMRUN is concerned. Also, setting/unsetting these bits prevents tests from
>> verifying hardware behavior.
>>
>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>> ---
>>   arch/x86/kvm/svm/nested.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>> index ae53ae46ebca..fd42c8b7f99a 100644
>> --- a/arch/x86/kvm/svm/nested.c
>> +++ b/arch/x86/kvm/svm/nested.c
>> @@ -287,8 +287,6 @@ static void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
>>   
>>   	/* Copy it here because nested_svm_check_controls will check it.  */
>>   	svm->nested.ctl.asid           = control->asid;
>> -	svm->nested.ctl.msrpm_base_pa &= ~0x0fffULL;
>> -	svm->nested.ctl.iopm_base_pa  &= ~0x0fffULL;
> This will break nested_svm_vmrun_msrpm() if L1 passes an unaligned address.
> The shortlog is also wrong, KVM isn't setting bits, it's clearing bits.
>
> I also don't think svm->nested.ctl.msrpm_base_pa makes its way to hardware; IIUC,
> it's a copy of vmcs12->control.msrpm_base_pa.  The bitmap that gets loaded into
> the "real" VMCB is vmcb02->control.msrpm_base_pa.


Not sure if there's a problem with my patch as such, but upon inspecting 
the code, I see something missing:

     In nested_load_control_from_vmcb12(), we are not really loading 
msrpm_base_pa from vmcb12 even     though the name of the function 
suggests so.

     Then nested_vmcb_check_controls() checks msrpm_base_pa from 
'nested.ctl' which doesn't have         the copy from vmcb12.

     Then nested_vmcb02_prepare_control() prepares the vmcb02 copy of 
msrpm_base_pa from vmcb01.ptr->control.msrpm_base_pa.

     Then nested_svm_vmrun_msrpm() uses msrpm_base_pa from 'nested.ctl'.


Aren't we actually using msrpm_base_pa from vmcb01 instead of vmcb02 ?

>>   }
>>   
>>   /*
>> -- 
>> 2.27.0
>>

Re: [PATCH 3/7 v7] KVM: nSVM: No need to set bits 11:0 in MSRPM and IOPM bitmaps

[Krish Sadhukhan]

On 4/22/21 10:50 AM, Krish Sadhukhan wrote:
>
> On 4/20/21 1:00 PM, Sean Christopherson wrote:
>> On Mon, Apr 12, 2021, Krish Sadhukhan wrote:
>>> According to APM vol 2, hardware ignores the low 12 bits in MSRPM 
>>> and IOPM
>>> bitmaps. Therefore setting/unssetting these bits has no effect as 
>>> far as
>>> VMRUN is concerned. Also, setting/unsetting these bits prevents 
>>> tests from
>>> verifying hardware behavior.
>>>
>>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>>> ---
>>>   arch/x86/kvm/svm/nested.c | 2 --
>>>   1 file changed, 2 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>>> index ae53ae46ebca..fd42c8b7f99a 100644
>>> --- a/arch/x86/kvm/svm/nested.c
>>> +++ b/arch/x86/kvm/svm/nested.c
>>> @@ -287,8 +287,6 @@ static void 
>>> nested_load_control_from_vmcb12(struct vcpu_svm *svm,
>>>         /* Copy it here because nested_svm_check_controls will check 
>>> it.  */
>>>       svm->nested.ctl.asid           = control->asid;
>>> -    svm->nested.ctl.msrpm_base_pa &= ~0x0fffULL;
>>> -    svm->nested.ctl.iopm_base_pa  &= ~0x0fffULL;
>> This will break nested_svm_vmrun_msrpm() if L1 passes an unaligned 
>> address.
>> The shortlog is also wrong, KVM isn't setting bits, it's clearing bits.
>>
>> I also don't think svm->nested.ctl.msrpm_base_pa makes its way to 
>> hardware; IIUC,
>> it's a copy of vmcs12->control.msrpm_base_pa.  The bitmap that gets 
>> loaded into
>> the "real" VMCB is vmcb02->control.msrpm_base_pa.
>
>
> Not sure if there's a problem with my patch as such, but upon 
> inspecting the code, I see something missing:
>
>     In nested_load_control_from_vmcb12(), we are not really loading 
> msrpm_base_pa from vmcb12 even     though the name of the function 
> suggests so.
>
>     Then nested_vmcb_check_controls() checks msrpm_base_pa from 
> 'nested.ctl' which doesn't have         the copy from vmcb12.
>
>     Then nested_vmcb02_prepare_control() prepares the vmcb02 copy of 
> msrpm_base_pa from vmcb01.ptr->control.msrpm_base_pa.
>
>     Then nested_svm_vmrun_msrpm() uses msrpm_base_pa from 'nested.ctl'.
>
>
> Aren't we actually using msrpm_base_pa from vmcb01 instead of vmcb02 ?

Sorry, I meant to say:  Aren't we actually using msrpm_base_pa from 
vmcb01 instead of vmcb12 ?
>
>>>   }
>>>     /*
>>> -- 
>>> 2.27.0
>>>

Re: [PATCH 3/7 v7] KVM: nSVM: No need to set bits 11:0 in MSRPM and IOPM bitmaps

[Krish Sadhukhan]

On 4/22/21 10:50 AM, Krish Sadhukhan wrote:
>
> On 4/20/21 1:00 PM, Sean Christopherson wrote:
>> On Mon, Apr 12, 2021, Krish Sadhukhan wrote:
>>> According to APM vol 2, hardware ignores the low 12 bits in MSRPM 
>>> and IOPM
>>> bitmaps. Therefore setting/unssetting these bits has no effect as 
>>> far as
>>> VMRUN is concerned. Also, setting/unsetting these bits prevents 
>>> tests from
>>> verifying hardware behavior.
>>>
>>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>>> ---
>>>   arch/x86/kvm/svm/nested.c | 2 --
>>>   1 file changed, 2 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>>> index ae53ae46ebca..fd42c8b7f99a 100644
>>> --- a/arch/x86/kvm/svm/nested.c
>>> +++ b/arch/x86/kvm/svm/nested.c
>>> @@ -287,8 +287,6 @@ static void 
>>> nested_load_control_from_vmcb12(struct vcpu_svm *svm,
>>>         /* Copy it here because nested_svm_check_controls will check 
>>> it.  */
>>>       svm->nested.ctl.asid           = control->asid;
>>> -    svm->nested.ctl.msrpm_base_pa &= ~0x0fffULL;
>>> -    svm->nested.ctl.iopm_base_pa  &= ~0x0fffULL;
>> This will break nested_svm_vmrun_msrpm() if L1 passes an unaligned 
>> address.
>> The shortlog is also wrong, KVM isn't setting bits, it's clearing bits.
>>
>> I also don't think svm->nested.ctl.msrpm_base_pa makes its way to 
>> hardware; IIUC,
>> it's a copy of vmcs12->control.msrpm_base_pa.  The bitmap that gets 
>> loaded into
>> the "real" VMCB is vmcb02->control.msrpm_base_pa.
>
>
> Not sure if there's a problem with my patch as such, but upon 
> inspecting the code, I see something missing:
>
>     In nested_load_control_from_vmcb12(), we are not really loading 
> msrpm_base_pa from vmcb12 even     though the name of the function 
> suggests so.
>
>     Then nested_vmcb_check_controls() checks msrpm_base_pa from 
> 'nested.ctl' which doesn't have         the copy from vmcb12.
>
>     Then nested_vmcb02_prepare_control() prepares the vmcb02 copy of 
> msrpm_base_pa from vmcb01.ptr->control.msrpm_base_pa.
>
>     Then nested_svm_vmrun_msrpm() uses msrpm_base_pa from 'nested.ctl'.
>
>
> Aren't we actually using msrpm_base_pa from vmcb01 instead of vmcb02 ?


Sorry, I meant to say,  "from vmcb01 instead of vmcb12"

>
>>>   }
>>>     /*
>>> -- 
>>> 2.27.0
>>>

Re: [PATCH 3/7 v7] KVM: nSVM: No need to set bits 11:0 in MSRPM and IOPM bitmaps

[Sean Christopherson]

On Thu, Apr 22, 2021, Krish Sadhukhan wrote:
> 
> On 4/22/21 10:50 AM, Krish Sadhukhan wrote:
> > 
> > On 4/20/21 1:00 PM, Sean Christopherson wrote:
> > > On Mon, Apr 12, 2021, Krish Sadhukhan wrote:
> > > > According to APM vol 2, hardware ignores the low 12 bits in
> > > > MSRPM and IOPM
> > > > bitmaps. Therefore setting/unssetting these bits has no effect
> > > > as far as
> > > > VMRUN is concerned. Also, setting/unsetting these bits prevents
> > > > tests from
> > > > verifying hardware behavior.
> > > > 
> > > > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> > > > ---
> > > >   arch/x86/kvm/svm/nested.c | 2 --
> > > >   1 file changed, 2 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > > > index ae53ae46ebca..fd42c8b7f99a 100644
> > > > --- a/arch/x86/kvm/svm/nested.c
> > > > +++ b/arch/x86/kvm/svm/nested.c
> > > > @@ -287,8 +287,6 @@ static void
> > > > nested_load_control_from_vmcb12(struct vcpu_svm *svm,
> > > >         /* Copy it here because nested_svm_check_controls will
> > > > check it.  */
> > > >       svm->nested.ctl.asid           = control->asid;
> > > > -    svm->nested.ctl.msrpm_base_pa &= ~0x0fffULL;
> > > > -    svm->nested.ctl.iopm_base_pa  &= ~0x0fffULL;
> > > This will break nested_svm_vmrun_msrpm() if L1 passes an unaligned
> > > address.
> > > The shortlog is also wrong, KVM isn't setting bits, it's clearing bits.
> > > 
> > > I also don't think svm->nested.ctl.msrpm_base_pa makes its way to
> > > hardware; IIUC,
> > > it's a copy of vmcs12->control.msrpm_base_pa.  The bitmap that gets
> > > loaded into
> > > the "real" VMCB is vmcb02->control.msrpm_base_pa.
> > 
> > 
> > Not sure if there's a problem with my patch as such, but upon inspecting
> > the code, I see something missing:
> > 
> >     In nested_load_control_from_vmcb12(), we are not really loading
> > msrpm_base_pa from vmcb12 even     though the name of the function
> > suggests so.
> > 
> >     Then nested_vmcb_check_controls() checks msrpm_base_pa from
> > 'nested.ctl' which doesn't have         the copy from vmcb12.
> > 
> >     Then nested_vmcb02_prepare_control() prepares the vmcb02 copy of
> > msrpm_base_pa from vmcb01.ptr->control.msrpm_base_pa.
> > 
> >     Then nested_svm_vmrun_msrpm() uses msrpm_base_pa from 'nested.ctl'.
> > 
> > 
> > Aren't we actually using msrpm_base_pa from vmcb01 instead of vmcb02 ?
> 
> 
> Sorry, I meant to say,  "from vmcb01 instead of vmcb12"

The bitmap that's shoved into hardware comes from vmcb02, the bitmap that KVM
reads to merge into _that_ bitmap comes from vmcb12.

static bool nested_svm_vmrun_msrpm(struct vcpu_svm *svm)
{
	/*
	 * This function merges the msr permission bitmaps of kvm and the
	 * nested vmcb. It is optimized in that it only merges the parts where
	 * the kvm msr permission bitmap may contain zero bits
	 */
	int i;

	if (!(vmcb_is_intercept(&svm->nested.ctl, INTERCEPT_MSR_PROT)))
		return true;

	for (i = 0; i < MSRPM_OFFSETS; i++) {
		u32 value, p;
		u64 offset;

		if (msrpm_offsets[i] == 0xffffffff)
			break;

		p      = msrpm_offsets[i];
		offset = svm->nested.ctl.msrpm_base_pa + (p * 4);

		if (kvm_vcpu_read_guest(&svm->vcpu, offset, &value, 4)) <- This reads vmcb12
			return false;

		svm->nested.msrpm[p] = svm->msrpm[p] | value; <- Merge vmcb12's bitmap to KVM's bitmap for L2
	}

	svm->vmcb->control.msrpm_base_pa = __sme_set(__pa(svm->nested.msrpm)); <- This is vmcb02

	return true;
}

Re: [PATCH 3/7 v7] KVM: nSVM: No need to set bits 11:0 in MSRPM and IOPM bitmaps

[Krish Sadhukhan]

On 4/22/21 11:01 AM, Sean Christopherson wrote:
> On Thu, Apr 22, 2021, Krish Sadhukhan wrote:
>> On 4/22/21 10:50 AM, Krish Sadhukhan wrote:
>>> On 4/20/21 1:00 PM, Sean Christopherson wrote:
>>>> On Mon, Apr 12, 2021, Krish Sadhukhan wrote:
>>>>> According to APM vol 2, hardware ignores the low 12 bits in
>>>>> MSRPM and IOPM
>>>>> bitmaps. Therefore setting/unssetting these bits has no effect
>>>>> as far as
>>>>> VMRUN is concerned. Also, setting/unsetting these bits prevents
>>>>> tests from
>>>>> verifying hardware behavior.
>>>>>
>>>>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>>>>> ---
>>>>>    arch/x86/kvm/svm/nested.c | 2 --
>>>>>    1 file changed, 2 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>>>>> index ae53ae46ebca..fd42c8b7f99a 100644
>>>>> --- a/arch/x86/kvm/svm/nested.c
>>>>> +++ b/arch/x86/kvm/svm/nested.c
>>>>> @@ -287,8 +287,6 @@ static void
>>>>> nested_load_control_from_vmcb12(struct vcpu_svm *svm,
>>>>>          /* Copy it here because nested_svm_check_controls will
>>>>> check it.  */
>>>>>        svm->nested.ctl.asid           = control->asid;
>>>>> -    svm->nested.ctl.msrpm_base_pa &= ~0x0fffULL;
>>>>> -    svm->nested.ctl.iopm_base_pa  &= ~0x0fffULL;
>>>> This will break nested_svm_vmrun_msrpm() if L1 passes an unaligned
>>>> address.
>>>> The shortlog is also wrong, KVM isn't setting bits, it's clearing bits.
>>>>
>>>> I also don't think svm->nested.ctl.msrpm_base_pa makes its way to
>>>> hardware; IIUC,
>>>> it's a copy of vmcs12->control.msrpm_base_pa.  The bitmap that gets
>>>> loaded into
>>>> the "real" VMCB is vmcb02->control.msrpm_base_pa.
>>>
>>> Not sure if there's a problem with my patch as such, but upon inspecting
>>> the code, I see something missing:
>>>
>>>      In nested_load_control_from_vmcb12(), we are not really loading
>>> msrpm_base_pa from vmcb12 even     though the name of the function
>>> suggests so.
>>>
>>>      Then nested_vmcb_check_controls() checks msrpm_base_pa from
>>> 'nested.ctl' which doesn't have         the copy from vmcb12.
>>>
>>>      Then nested_vmcb02_prepare_control() prepares the vmcb02 copy of
>>> msrpm_base_pa from vmcb01.ptr->control.msrpm_base_pa.
>>>
>>>      Then nested_svm_vmrun_msrpm() uses msrpm_base_pa from 'nested.ctl'.
>>>
>>>
>>> Aren't we actually using msrpm_base_pa from vmcb01 instead of vmcb02 ?
>>
>> Sorry, I meant to say,  "from vmcb01 instead of vmcb12"
> The bitmap that's shoved into hardware comes from vmcb02, the bitmap that KVM
> reads to merge into _that_ bitmap comes from vmcb12.
>
> static bool nested_svm_vmrun_msrpm(struct vcpu_svm *svm)
> {
> 	/*
> 	 * This function merges the msr permission bitmaps of kvm and the
> 	 * nested vmcb. It is optimized in that it only merges the parts where
> 	 * the kvm msr permission bitmap may contain zero bits
> 	 */
> 	int i;
>
> 	if (!(vmcb_is_intercept(&svm->nested.ctl, INTERCEPT_MSR_PROT)))
> 		return true;
>
> 	for (i = 0; i < MSRPM_OFFSETS; i++) {
> 		u32 value, p;
> 		u64 offset;
>
> 		if (msrpm_offsets[i] == 0xffffffff)
> 			break;
>
> 		p      = msrpm_offsets[i];
> 		offset = svm->nested.ctl.msrpm_base_pa + (p * 4);
>
> 		if (kvm_vcpu_read_guest(&svm->vcpu, offset, &value, 4)) <- This reads vmcb12
> 			return false;
>
> 		svm->nested.msrpm[p] = svm->msrpm[p] | value; <- Merge vmcb12's bitmap to KVM's bitmap for L2
> 	}
>
> 	svm->vmcb->control.msrpm_base_pa = __sme_set(__pa(svm->nested.msrpm)); <- This is vmcb02
>
> 	return true;
> }

Sorry, I somehow missed the call to copy_vmcb_control_area() in 
nested_load_control_from_vmcb12() where we are actually getting the 
msrpm_base_pa from vmcb12. Thanks for the explanation.

Getting back to your concern that this patch breaks 
nested_svm_vmrun_msrpm().  If L1 passes a valid address in which some 
bits in 11:0 are set, the hardware is anyway going to ignore those bits, 
irrespective of whether we clear them (before my patch) or pass them as 
is (my patch) and therefore what L1 thinks as a valid address will 
effectively be an invalid address to the hardware. The only difference 
my patch makes is it enables tests to verify hardware behavior. Am 
missing something ?

Re: [PATCH 3/7 v7] KVM: nSVM: No need to set bits 11:0 in MSRPM and IOPM bitmaps

[Sean Christopherson]

On Thu, Apr 22, 2021, Krish Sadhukhan wrote:
> On 4/22/21 11:01 AM, Sean Christopherson wrote:
> > 		offset = svm->nested.ctl.msrpm_base_pa + (p * 4);
> > 
> > 		if (kvm_vcpu_read_guest(&svm->vcpu, offset, &value, 4)) <- This reads vmcb12
> > 			return false;
> > 
> > 		svm->nested.msrpm[p] = svm->msrpm[p] | value; <- Merge vmcb12's bitmap to KVM's bitmap for L2

... 
 
> Getting back to your concern that this patch breaks
> nested_svm_vmrun_msrpm().  If L1 passes a valid address in which some bits
> in 11:0 are set, the hardware is anyway going to ignore those bits,
> irrespective of whether we clear them (before my patch) or pass them as is
> (my patch) and therefore what L1 thinks as a valid address will effectively
> be an invalid address to the hardware. The only difference my patch makes is
> it enables tests to verify hardware behavior. Am missing something ?

See the above snippet where KVM reads the effectively vmcb12->msrpm to merge L1's
desires with KVM's desires.  By removing the code that ensures
svm->nested.ctl.msrpm_base_pa is page aligned, the above offset calculation will
be wrong.

Re: [PATCH 3/7 v7] KVM: nSVM: No need to set bits 11:0 in MSRPM and IOPM bitmaps

[Paolo Bonzini]

On 23/04/21 17:56, Sean Christopherson wrote:
> On Thu, Apr 22, 2021, Krish Sadhukhan wrote:
>> On 4/22/21 11:01 AM, Sean Christopherson wrote:
>>> 		offset = svm->nested.ctl.msrpm_base_pa + (p * 4);
>>>
>>> 		if (kvm_vcpu_read_guest(&svm->vcpu, offset, &value, 4)) <- This reads vmcb12
>>> 			return false;
>>>
>>> 		svm->nested.msrpm[p] = svm->msrpm[p] | value; <- Merge vmcb12's bitmap to KVM's bitmap for L2
> 
> ...
>   
>> Getting back to your concern that this patch breaks
>> nested_svm_vmrun_msrpm().  If L1 passes a valid address in which some bits
>> in 11:0 are set, the hardware is anyway going to ignore those bits,
>> irrespective of whether we clear them (before my patch) or pass them as is
>> (my patch) and therefore what L1 thinks as a valid address will effectively
>> be an invalid address to the hardware. The only difference my patch makes is
>> it enables tests to verify hardware behavior. Am missing something ?
> 
> See the above snippet where KVM reads the effectively vmcb12->msrpm to merge L1's
> desires with KVM's desires.  By removing the code that ensures
> svm->nested.ctl.msrpm_base_pa is page aligned, the above offset calculation will
> be wrong.

In fact the kvm-unit-test you sent was also wrong for this same reason 
when it was testing addresses near the end of the physical address space.

Paolo

Re: [PATCH 3/7 v7] KVM: nSVM: No need to set bits 11:0 in MSRPM and IOPM bitmaps

[Krish Sadhukhan]

On 4/23/21 1:31 PM, Paolo Bonzini wrote:
> On 23/04/21 17:56, Sean Christopherson wrote:
>> On Thu, Apr 22, 2021, Krish Sadhukhan wrote:
>>> On 4/22/21 11:01 AM, Sean Christopherson wrote:
>>>>         offset = svm->nested.ctl.msrpm_base_pa + (p * 4);
>>>>
>>>>         if (kvm_vcpu_read_guest(&svm->vcpu, offset, &value, 4)) <- 
>>>> This reads vmcb12
>>>>             return false;
>>>>
>>>>         svm->nested.msrpm[p] = svm->msrpm[p] | value; <- Merge 
>>>> vmcb12's bitmap to KVM's bitmap for L2
>>
>> ...
>>> Getting back to your concern that this patch breaks
>>> nested_svm_vmrun_msrpm().  If L1 passes a valid address in which 
>>> some bits
>>> in 11:0 are set, the hardware is anyway going to ignore those bits,
>>> irrespective of whether we clear them (before my patch) or pass them 
>>> as is
>>> (my patch) and therefore what L1 thinks as a valid address will 
>>> effectively
>>> be an invalid address to the hardware. The only difference my patch 
>>> makes is
>>> it enables tests to verify hardware behavior. Am missing something ?
>>
>> See the above snippet where KVM reads the effectively vmcb12->msrpm 
>> to merge L1's
>> desires with KVM's desires.  By removing the code that ensures
>> svm->nested.ctl.msrpm_base_pa is page aligned, the above offset 
>> calculation will
>> be wrong.
>
> In fact the kvm-unit-test you sent was also wrong for this same reason 
> when it was testing addresses near the end of the physical address space.
>
> Paolo
>
It seems to me that we should clear bits 11:0 in 
nested_svm_vmrun_msrpm() where we are forming  msrpm_base_pa address for 
vmcb02.  nested_svm_check_bitmap_pa() aligns the address passed to it, 
before checking it.

Should I send a patch for this ?

Re: [PATCH 3/7 v7] KVM: nSVM: No need to set bits 11:0 in MSRPM and IOPM bitmaps

[Sean Christopherson]

On Mon, Apr 26, 2021, Krish Sadhukhan wrote:
> 
> On 4/23/21 1:31 PM, Paolo Bonzini wrote:
> > On 23/04/21 17:56, Sean Christopherson wrote:
> > > On Thu, Apr 22, 2021, Krish Sadhukhan wrote:
> > > > On 4/22/21 11:01 AM, Sean Christopherson wrote:
> > > > >         offset = svm->nested.ctl.msrpm_base_pa + (p * 4);
> > > > > 
> > > > >         if (kvm_vcpu_read_guest(&svm->vcpu, offset, &value,
> > > > > 4)) <- This reads vmcb12
> > > > >             return false;
> > > > > 
> > > > >         svm->nested.msrpm[p] = svm->msrpm[p] | value; <-
> > > > > Merge vmcb12's bitmap to KVM's bitmap for L2
> > > 
> > > ...
> > > > Getting back to your concern that this patch breaks
> > > > nested_svm_vmrun_msrpm().  If L1 passes a valid address in which
> > > > some bits
> > > > in 11:0 are set, the hardware is anyway going to ignore those bits,
> > > > irrespective of whether we clear them (before my patch) or pass
> > > > them as is
> > > > (my patch) and therefore what L1 thinks as a valid address will
> > > > effectively
> > > > be an invalid address to the hardware. The only difference my
> > > > patch makes is
> > > > it enables tests to verify hardware behavior. Am missing something ?
> > > 
> > > See the above snippet where KVM reads the effectively vmcb12->msrpm
> > > to merge L1's
> > > desires with KVM's desires.  By removing the code that ensures
> > > svm->nested.ctl.msrpm_base_pa is page aligned, the above offset
> > > calculation will
> > > be wrong.
> > 
> > In fact the kvm-unit-test you sent was also wrong for this same reason
> > when it was testing addresses near the end of the physical address
> > space.
> > 
> > Paolo
> > 
> It seems to me that we should clear bits 11:0 in nested_svm_vmrun_msrpm()
> where we are forming  msrpm_base_pa address for vmcb02. 
> nested_svm_check_bitmap_pa() aligns the address passed to it, before
> checking it.
> 
> Should I send a patch for this ?

I don't see any reason to leave the bits set in svm->nested.ctl.msrpm_base_pa
any longer than they absolutely need to be set, i.e.
nested_load_control_from_vmcb12() feels like the best place to clear the offset.

If we really want to change something, we could WARN in the consistency check on
an unaligned address, e.g.

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 540d43ba2cf4..56e109d2ea7f 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -220,7 +220,7 @@ static bool nested_svm_vmrun_msrpm(struct vcpu_svm *svm)
  */
 static bool nested_svm_check_bitmap_pa(struct kvm_vcpu *vcpu, u64 pa, u32 size)
 {
-       u64 addr = PAGE_ALIGN(pa);
+       WARN_ON_ONCE(!PAGE_ALIGNED(pa));

        return kvm_vcpu_is_legal_gpa(vcpu, addr) &&
            kvm_vcpu_is_legal_gpa(vcpu, addr + size - 1);

[PATCH 4/7 v7] nSVM: Check addresses of MSR and IO permission maps

[Krish Sadhukhan]

According to section "Canonicalization and Consistency Checks" in APM vol 2,
the following guest state is illegal:

    "The MSR or IOIO intercept tables extend to a physical address that
     is greater than or equal to the maximum supported physical address."

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 arch/x86/kvm/svm/nested.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index fd42c8b7f99a..c3e5f4e7c987 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -215,7 +215,19 @@ static bool nested_svm_vmrun_msrpm(struct vcpu_svm *svm)
 	return true;
 }
 
-static bool nested_vmcb_check_controls(struct vmcb_control_area *control)
+/*
+ * Bits 11:0 of bitmap address are ignored by hardware
+ */
+static bool nested_svm_check_bitmap_pa(struct kvm_vcpu *vcpu, u64 pa, u32 size)
+{
+	u64 addr = PAGE_ALIGN(pa);
+
+	return kvm_vcpu_is_legal_gpa(vcpu, addr) &&
+	    kvm_vcpu_is_legal_gpa(vcpu, addr + size - 1);
+}
+
+static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu,
+				       struct vmcb_control_area *control)
 {
 	if (CC(!vmcb_is_intercept(control, INTERCEPT_VMRUN)))
 		return false;
@@ -226,6 +238,13 @@ static bool nested_vmcb_check_controls(struct vmcb_control_area *control)
 	if (CC((control->nested_ctl & SVM_NESTED_CTL_NP_ENABLE) && !npt_enabled))
 		return false;
 
+	if (CC(!nested_svm_check_bitmap_pa(vcpu, control->msrpm_base_pa,
+					   MSRPM_SIZE)))
+		return false;
+	if (CC(!nested_svm_check_bitmap_pa(vcpu, control->iopm_base_pa,
+					   IOPM_SIZE)))
+		return false;
+
 	return true;
 }
 
@@ -602,7 +621,7 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu)
 	nested_load_control_from_vmcb12(svm, &vmcb12->control);
 
 	if (!nested_vmcb_valid_sregs(vcpu, &vmcb12->save) ||
-	    !nested_vmcb_check_controls(&svm->nested.ctl)) {
+	    !nested_vmcb_check_controls(vcpu, &svm->nested.ctl)) {
 		vmcb12->control.exit_code    = SVM_EXIT_ERR;
 		vmcb12->control.exit_code_hi = SVM_CONSISTENCY_ERR;
 		vmcb12->control.exit_info_1  = 0;
@@ -1250,7 +1269,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 		goto out_free;
 
 	ret = -EINVAL;
-	if (!nested_vmcb_check_controls(ctl))
+	if (!nested_vmcb_check_controls(vcpu, ctl))
 		goto out_free;
 
 	/*
-- 
2.27.0

[PATCH 5/7 v7] SVM: Use ALIGN macro when aligning 'io_bitmap_area'

[Krish Sadhukhan]

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 x86/svm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/x86/svm.c b/x86/svm.c
index a1808c7..846cf2a 100644
--- a/x86/svm.c
+++ b/x86/svm.c
@@ -298,7 +298,7 @@ static void setup_svm(void)
 	wrmsr(MSR_EFER, rdmsr(MSR_EFER) | EFER_SVME);
 	wrmsr(MSR_EFER, rdmsr(MSR_EFER) | EFER_NX);
 
-	io_bitmap = (void *) (((ulong)io_bitmap_area + 4095) & ~4095);
+	io_bitmap = (void *) ALIGN((ulong)io_bitmap_area, PAGE_SIZE);
 
 	msr_bitmap = (void *) ALIGN((ulong)msr_bitmap_area, PAGE_SIZE);
 
-- 
2.27.0

[PATCH 6/7 v7] nSVM: Define an exit code to reflect consistency check failure

[Krish Sadhukhan]

Define an exit code that reflects failures in consistency checking during
VMRUN. KVM sets bit 63 in EXIT CODE when it detects invalid guest state
during VMRUN consistency checking.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 x86/svm.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/x86/svm.h b/x86/svm.h
index a0863b8..48a07e7 100644
--- a/x86/svm.h
+++ b/x86/svm.h
@@ -321,6 +321,7 @@ struct __attribute__ ((__packed__)) vmcb {
 #define SVM_EXIT_NPF  		0x400
 
 #define SVM_EXIT_ERR		-1
+#define	SVM_CONSISTENCY_ERR	1 << 31
 
 #define SVM_CR0_SELECTIVE_MASK (X86_CR0_TS | X86_CR0_MP)
 
-- 
2.27.0

[PATCH 7/7 v7] nSVM: Test addresses of MSR and IO permissions maps

[Krish Sadhukhan]

According to section "Canonicalization and Consistency Checks" in APM vol 2,
the following guest state is illegal:

    "The MSR or IOIO intercept tables extend to a physical address that
     is greater than or equal to the maximum supported physical address.
     The VMRUN instruction ignores the lower 12 bits of the address
     specified in the VMCB."

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 x86/svm_tests.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 77 insertions(+), 1 deletion(-)

diff --git a/x86/svm_tests.c b/x86/svm_tests.c
index 29a0b59..15be8f5 100644
--- a/x86/svm_tests.c
+++ b/x86/svm_tests.c
@@ -2304,15 +2304,91 @@ static void test_dr(void)
 	vmcb->save.dr7 = dr_saved;
 }
 
+#define	TEST_BITMAP_ADDR(save_intercept, type, addr, exit_code,		\
+			consistency_fail, msg) {			\
+	u32 exit_code_hi;						\
+	vmcb->control.intercept = saved_intercept | 1ULL << type;	\
+	if (type == INTERCEPT_MSR_PROT)					\
+		vmcb->control.msrpm_base_pa = addr;			\
+	else								\
+		vmcb->control.iopm_base_pa = addr;			\
+	exit_code_hi = consistency_fail	? SVM_CONSISTENCY_ERR : 0;	\
+	report(svm_vmrun() == exit_code &&				\
+	    vmcb->control.exit_code_hi == exit_code_hi,			\
+	    "Test %s address: %lx %x", msg, addr, vmcb->control.exit_code_hi);\
+}
+
+/*
+ * If the MSR or IOIO intercept table extends to a physical address that
+ * is greater than or equal to the maximum supported physical address, the
+ * guest state is illegal.
+ *
+ * The VMRUN instruction ignores the lower 12 bits of the address specified
+ * in the VMCB.
+ *
+ * MSRPM spans 2 contiguous 4KB pages while IOPM spans 2 contiguous 4KB
+ * pages + 1 byte.
+ *
+ * [APM vol 2]
+ *
+ * Note: Unallocated MSRPM addresses conforming to consistency checks, generate
+ * #NPF.
+ */
+static void test_msrpm_iopm_bitmap_addrs(void)
+{
+	u64 saved_intercept = vmcb->control.intercept;
+	u64 addr_beyond_limit = 1ull << cpuid_maxphyaddr();
+	u64 addr = virt_to_phys(msr_bitmap) & (~((1ull << 12) - 1));
+
+	TEST_BITMAP_ADDR(saved_intercept, INTERCEPT_MSR_PROT,
+			addr_beyond_limit - 3 * PAGE_SIZE, SVM_EXIT_ERR, false,
+			"MSRPM");
+	TEST_BITMAP_ADDR(saved_intercept, INTERCEPT_MSR_PROT,
+			addr_beyond_limit - 2 * PAGE_SIZE, SVM_EXIT_ERR, false,
+			"MSRPM");
+	TEST_BITMAP_ADDR(saved_intercept, INTERCEPT_MSR_PROT,
+			addr_beyond_limit - 2 * PAGE_SIZE + 1, SVM_EXIT_ERR,
+			true, "MSRPM");
+	TEST_BITMAP_ADDR(saved_intercept, INTERCEPT_MSR_PROT,
+			addr_beyond_limit - PAGE_SIZE, SVM_EXIT_ERR, true,
+			"MSRPM");
+	TEST_BITMAP_ADDR(saved_intercept, INTERCEPT_MSR_PROT, addr,
+			SVM_EXIT_VMMCALL, false, "MSRPM");
+	addr |= (1ull << 12) - 1;
+	TEST_BITMAP_ADDR(saved_intercept, INTERCEPT_MSR_PROT, addr,
+			SVM_EXIT_VMMCALL, false, "MSRPM");
+
+	TEST_BITMAP_ADDR(saved_intercept, INTERCEPT_IOIO_PROT,
+			addr_beyond_limit - 4 * PAGE_SIZE, SVM_EXIT_VMMCALL,
+			false, "IOPM");
+	TEST_BITMAP_ADDR(saved_intercept, INTERCEPT_IOIO_PROT,
+			addr_beyond_limit - 3 * PAGE_SIZE, SVM_EXIT_VMMCALL,
+			false, "IOPM");
+	TEST_BITMAP_ADDR(saved_intercept, INTERCEPT_IOIO_PROT,
+			addr_beyond_limit - 3 * PAGE_SIZE + 1, SVM_EXIT_ERR,
+			true, "IOPM");
+	TEST_BITMAP_ADDR(saved_intercept, INTERCEPT_IOIO_PROT,
+			addr_beyond_limit - PAGE_SIZE, SVM_EXIT_ERR, true,
+			"IOPM");
+	addr = virt_to_phys(io_bitmap) & (~((1ull << 11) - 1));
+	TEST_BITMAP_ADDR(saved_intercept, INTERCEPT_IOIO_PROT, addr,
+			SVM_EXIT_VMMCALL, false, "IOPM");
+	addr |= (1ull << 12) - 1;
+	TEST_BITMAP_ADDR(saved_intercept, INTERCEPT_IOIO_PROT, addr,
+			SVM_EXIT_VMMCALL, false, "IOPM");
+
+	vmcb->control.intercept = saved_intercept;
+}
+
 static void svm_guest_state_test(void)
 {
 	test_set_guest(basic_guest_main);
-
 	test_efer();
 	test_cr0();
 	test_cr3();
 	test_cr4();
 	test_dr();
+	test_msrpm_iopm_bitmap_addrs();
 }
 
 
-- 
2.27.0

Re: [PATCH 0/7 v7] KVM: nSVM: Check addresses of MSR bitmap and IO bitmap tables on vmrun of nested guests

[Paolo Bonzini]

On 12/04/21 23:56, Krish Sadhukhan wrote:
> v6 -> v7:
> 	1. In patch# 4, the logic in nested_svm_check_bitmap_pa() has been
> 	   fixed. The size of IOPM passed to nested_svm_check_bitmap_pa() has
> 	   also been corrected. Indentation has been improved.
> 	2. In patch# 1, the names of the #defines have been changed.
> 	3. In patch# 2, a new exit code is defined to differentiate between
> 	   consistency failures and failures after switching to guest mode,
> 	   because tests need to know the exact failure instead of
> 	   SVM_EXIT_ERR. This exit code is similar to what nVMX does and
> 	   appears to be the best solution to differentiate the above-mentioned
> 	   error scenarios.
> 	4. In patch# 3, code that unset bit 11:0 of the MSRPm and IOPM tables,
> 	   has been removed because hardware doesn't care about the value
> 	   these bits. Also, tests need to verify hardware behavior. So if
> 	   these bits are unset, the checks in nested_svm_check_bitmap_pa()
> 	   do not work as expected.
> 	5. In patch# 7, additional test cases have been added.
> 
> 
> [PATCH 1/7 v7] KVM: SVM: Define actual size of IOPM and MSRPM tables
> [PATCH 2/7 v7] KVM: nSVM: Define an exit code to reflect consistency
> [PATCH 3/7 v7] KVM: nSVM: No need to set bits 11:0 in MSRPM and IOPM
> [PATCH 4/7 v7] nSVM: Check addresses of MSR and IO permission maps
> [PATCH 5/7 v7] SVM: Use ALIGN macro when aligning 'io_bitmap_area'
> [PATCH 6/7 v7] nSVM: Define an exit code to reflect consistency check
> [PATCH 7/7 v7] nSVM: Test addresses of MSR and IO permissions maps
> 
>   arch/x86/include/uapi/asm/svm.h |  1 +
>   arch/x86/kvm/svm/nested.c       | 29 +++++++++++++++++++++++------
>   arch/x86/kvm/svm/svm.c          | 20 ++++++++++----------
>   arch/x86/kvm/svm/svm.h          |  3 +++
>   4 files changed, 37 insertions(+), 16 deletions(-)
> 
> Krish Sadhukhan (4):
>        KVM: SVM: Define actual size of IOPM and MSRPM tables
>        KVM: nSVM: Define an exit code to reflect consistency check failure
>        KVM: nSVM: No need to set bits 11:0 in MSRPM and IOPM bitmaps
>        nSVM: Check addresses of MSR and IO permission maps
> 
>   x86/svm.c       |  2 +-
>   x86/svm.h       |  1 +
>   x86/svm_tests.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>   3 files changed, 79 insertions(+), 2 deletions(-)
> 
> Krish Sadhukhan (3):
>        SVM: Use ALIGN macro when aligning 'io_bitmap_area'
>        nSVM: Define an exit code to reflect consistency check failure
>        nSVM: Test addresses of MSR and IO permissions maps
> 

Queued except for SVM_CONSISTENCY_ERR.

Paolo