[PATCH 0/4 v2] KVM: nSVM: Add checks for CR3 and CR4 reserved bits to svm_set_nested_state() and test CR3 non-MBZ reserved bits

[PATCH 0/4 v2] KVM: nSVM: Add checks for CR3 and CR4 reserved bits to svm_set_nested_state() and test CR3 non-MBZ reserved bits

[Krish Sadhukhan]

v1 -> v2:
	1. Patch# 2 has added checks for DR6, DR7 and EFER to
	   SVM_SET_NESTED_STATE path.
	2. Patch# 4 is a new addition. It has added missing checks for EFER
	   to nested_vmcb_checks().

[PATCH 1/4 v2] KVM: nSVM: CR3 MBZ bits are only 63:52
[PATCH 2/4 v2] KVM: nSVM: Add check for reserved bits for CR3, CR4, DR6,
[PATCH 3/4 v2] KVM: nSVM: Test non-MBZ reserved bits in CR3 in long mode
[PATCH 4/4 v2] KVM: nSVM: nested_vmcb_checks() needs to check all bits of

 arch/x86/kvm/svm/nested.c | 58 ++++++++++++++++++++++++++++-------------------
 arch/x86/kvm/svm/svm.h    |  2 +-
 2 files changed, 36 insertions(+), 24 deletions(-)

Krish Sadhukhan (3):
      KVM: nSVM: CR3 MBZ bits are only 63:52
      KVM: nSVM: Add check for reserved bits for CR3, CR4, DR6, DR7 and EFER to 
svm_set_nested_state()
      KVM: nSVM: nested_vmcb_checks() needs to check all bits of EFER

 x86/svm.h       |  3 ++-
 x86/svm_tests.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 48 insertions(+), 7 deletions(-)

Krish Sadhukhan (1):
      KVM: nSVM: Test non-MBZ reserved bits in CR3 in long mode

[PATCH 1/4 v2] KVM: nSVM: CR3 MBZ bits are only 63:52

[Krish Sadhukhan]

Commit 761e4169346553c180bbd4a383aedd72f905bc9a created a wrong mask for the
CR3 MBZ bits. According to APM vol 2, only the upper 12 bits are MBZ.

(Fixes 761e4169346553c180bbd4a383aedd72f905bc9a)

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

diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index a798e1731709..c0d75b1e0664 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -345,7 +345,7 @@ static inline bool gif_set(struct vcpu_svm *svm)
 /* svm.c */
 #define MSR_CR3_LEGACY_RESERVED_MASK		0xfe7U
 #define MSR_CR3_LEGACY_PAE_RESERVED_MASK	0x7U
-#define MSR_CR3_LONG_RESERVED_MASK		0xfff0000000000fe7U
+#define MSR_CR3_LONG_MBZ_MASK			0xfff0000000000000U
 #define MSR_INVALID				0xffffffffU
 
 u32 svm_msrpm_offset(u32 msr);
-- 
2.18.4

[PATCH 2/4 v2] KVM: nSVM: Add check for reserved bits for CR3, CR4, DR6, DR7 and EFER to svm_set_nested_state()

[Krish Sadhukhan]

The path for SVM_SET_NESTED_STATE needs to have the same checks for the CPU
registers, as we have in the VMRUN path for a nested guest. This patch adds
those missing checks to svm_set_nested_state().

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

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index e90bc436f584..55c785587bd4 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -215,9 +215,35 @@ static bool nested_vmcb_check_controls(struct vmcb_control_area *control)
 	return true;
 }
 
+static bool nested_vmcb_check_cr3_cr4(struct vcpu_svm *svm,
+				      struct vmcb_save_area *save)
+{
+	bool nested_vmcb_lma =
+	        (save->efer & EFER_LME) &&
+		(save->cr0 & X86_CR0_PG);
+
+	if (!nested_vmcb_lma) {
+		if (save->cr4 & X86_CR4_PAE) {
+			if (save->cr3 & MSR_CR3_LEGACY_PAE_RESERVED_MASK)
+				return false;
+		} else {
+			if (save->cr3 & MSR_CR3_LEGACY_RESERVED_MASK)
+				return false;
+		}
+	} else {
+		if (!(save->cr4 & X86_CR4_PAE) ||
+		    !(save->cr0 & X86_CR0_PE) ||
+		    (save->cr3 & MSR_CR3_LONG_MBZ_MASK))
+			return false;
+	}
+	if (kvm_valid_cr4(&svm->vcpu, save->cr4))
+		return false;
+
+	return true;
+}
+
 static bool nested_vmcb_checks(struct vcpu_svm *svm, struct vmcb *vmcb)
 {
-	bool nested_vmcb_lma;
 	if ((vmcb->save.efer & EFER_SVME) == 0)
 		return false;
 
@@ -228,25 +254,7 @@ static bool nested_vmcb_checks(struct vcpu_svm *svm, struct vmcb *vmcb)
 	if (!kvm_dr6_valid(vmcb->save.dr6) || !kvm_dr7_valid(vmcb->save.dr7))
 		return false;
 
-	nested_vmcb_lma =
-	        (vmcb->save.efer & EFER_LME) &&
-		(vmcb->save.cr0 & X86_CR0_PG);
-
-	if (!nested_vmcb_lma) {
-		if (vmcb->save.cr4 & X86_CR4_PAE) {
-			if (vmcb->save.cr3 & MSR_CR3_LEGACY_PAE_RESERVED_MASK)
-				return false;
-		} else {
-			if (vmcb->save.cr3 & MSR_CR3_LEGACY_RESERVED_MASK)
-				return false;
-		}
-	} else {
-		if (!(vmcb->save.cr4 & X86_CR4_PAE) ||
-		    !(vmcb->save.cr0 & X86_CR0_PE) ||
-		    (vmcb->save.cr3 & MSR_CR3_LONG_RESERVED_MASK))
-			return false;
-	}
-	if (kvm_valid_cr4(&svm->vcpu, vmcb->save.cr4))
+	if (!nested_vmcb_check_cr3_cr4(svm, &(vmcb->save)))
 		return false;
 
 	return nested_vmcb_check_controls(&vmcb->control);
@@ -1116,9 +1124,12 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 	/*
 	 * Validate host state saved from before VMRUN (see
 	 * nested_svm_check_permissions).
-	 * TODO: validate reserved bits for all saved state.
 	 */
-	if (!(save.cr0 & X86_CR0_PG))
+	if (!(save.cr0 & X86_CR0_PG) ||
+	    !nested_vmcb_check_cr3_cr4(svm, &save) ||
+	    !kvm_dr6_valid(save.dr6) ||
+	    !kvm_dr7_valid(save.dr7) ||
+	    !kvm_valid_efer(vcpu, save.efer))
 		return -EINVAL;
 
 	/*
-- 
2.18.4

[PATCH 3/4 v2] KVM: nSVM: Test non-MBZ reserved bits in CR3 in long mode

[Krish Sadhukhan]

According to section "CR3" in APM vol. 2, the non-MBZ reserved bits in CR3
need to be set by software as follows:

	"Reserved Bits. Reserved fields should be cleared to 0 by software
	when writing CR3."

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 x86/svm.h       |  3 ++-
 x86/svm_tests.c | 52 +++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 48 insertions(+), 7 deletions(-)

diff --git a/x86/svm.h b/x86/svm.h
index 15e0f18..465d794 100644
--- a/x86/svm.h
+++ b/x86/svm.h
@@ -325,7 +325,8 @@ struct __attribute__ ((__packed__)) vmcb {
 #define SVM_CR0_SELECTIVE_MASK (X86_CR0_TS | X86_CR0_MP)
 
 #define	SVM_CR0_RESERVED_MASK			0xffffffff00000000U
-#define	SVM_CR3_LONG_RESERVED_MASK		0xfff0000000000000U
+#define	SVM_CR3_LONG_MBZ_MASK			0xfff0000000000000U
+#define	SVM_CR3_LONG_RESERVED_MASK		0x0000000000000fe7U
 #define	SVM_CR4_LEGACY_RESERVED_MASK		0xff88f000U
 #define	SVM_CR4_RESERVED_MASK			0xffffffffff88f000U
 #define	SVM_DR6_RESERVED_MASK			0xffffffffffff1ff0U
diff --git a/x86/svm_tests.c b/x86/svm_tests.c
index 1908c7c..6c97ee3 100644
--- a/x86/svm_tests.c
+++ b/x86/svm_tests.c
@@ -1913,7 +1913,8 @@ static void basic_guest_main(struct svm_test *test)
         }								\
 }
 
-#define SVM_TEST_CR_RESERVED_BITS(start, end, inc, cr, val, resv_mask)	\
+#define SVM_TEST_CR_RESERVED_BITS(start, end, inc, cr, val, resv_mask,	\
+				  exit_code)				\
 {									\
 	u64 tmp, mask;							\
 	int i;								\
@@ -1933,7 +1934,7 @@ static void basic_guest_main(struct svm_test *test)
 		case 4:							\
 			vmcb->save.cr4 = tmp;				\
 		}							\
-		report(svm_vmrun() == SVM_EXIT_ERR, "Test CR%d %d:%d: %lx",\
+		report(svm_vmrun() == exit_code, "Test CR%d %d:%d: %lx",\
 		    cr, end, start, tmp);				\
 	}								\
 }
@@ -2012,9 +2013,48 @@ static void test_cr3(void)
 	u64 cr3_saved = vmcb->save.cr3;
 
 	SVM_TEST_CR_RESERVED_BITS(0, 63, 1, 3, cr3_saved,
-	    SVM_CR3_LONG_RESERVED_MASK);
+	    SVM_CR3_LONG_MBZ_MASK, SVM_EXIT_ERR);
+
+	vmcb->save.cr3 = cr3_saved & ~SVM_CR3_LONG_MBZ_MASK;
+	report(svm_vmrun() == SVM_EXIT_VMMCALL, "Test CR3 63:0: %lx",
+	    vmcb->save.cr3);
+
+	/*
+	 * CR3 non-MBZ reserved bits based on different modes:
+	 *   [11:5] [2:0] - long mode
+	 */
+	u64 cr4_saved = vmcb->save.cr4;
+
+	/*
+	 * Long mode
+	 */
+	if (this_cpu_has(X86_FEATURE_PCID)) {
+		vmcb->save.cr4 = cr4_saved | X86_CR4_PCIDE;
+		SVM_TEST_CR_RESERVED_BITS(0, 11, 1, 3, cr3_saved,
+		    SVM_CR3_LONG_RESERVED_MASK, SVM_EXIT_VMMCALL);
+
+		vmcb->save.cr3 = cr3_saved & ~SVM_CR3_LONG_RESERVED_MASK;
+		report(svm_vmrun() == SVM_EXIT_VMMCALL, "Test CR3 63:0: %lx",
+		    vmcb->save.cr3);
+	} else {
+		u64 *pdpe = npt_get_pml4e();
+
+		vmcb->save.cr4 = cr4_saved & ~X86_CR4_PCIDE;
+
+		/* Clear P (Present) bit in NPT in order to trigger #NPF */
+		pdpe[0] &= ~1ULL;
+
+		SVM_TEST_CR_RESERVED_BITS(0, 11, 1, 3, cr3_saved,
+		    SVM_CR3_LONG_RESERVED_MASK, SVM_EXIT_NPF);
+
+		pdpe[0] |= 1ULL;
+		vmcb->save.cr3 = cr3_saved & ~SVM_CR3_LONG_RESERVED_MASK;
+		report(svm_vmrun() == SVM_EXIT_VMMCALL, "Test CR3 63:0: %lx",
+		    vmcb->save.cr3);
+	}
 
 	vmcb->save.cr3 = cr3_saved;
+	vmcb->save.cr4 = cr4_saved;
 }
 
 static void test_cr4(void)
@@ -2031,14 +2071,14 @@ static void test_cr4(void)
 	efer &= ~EFER_LME;
 	vmcb->save.efer = efer;
 	SVM_TEST_CR_RESERVED_BITS(12, 31, 1, 4, cr4_saved,
-	    SVM_CR4_LEGACY_RESERVED_MASK);
+	    SVM_CR4_LEGACY_RESERVED_MASK, SVM_EXIT_ERR);
 
 	efer |= EFER_LME;
 	vmcb->save.efer = efer;
 	SVM_TEST_CR_RESERVED_BITS(12, 31, 1, 4, cr4_saved,
-	    SVM_CR4_RESERVED_MASK);
+	    SVM_CR4_RESERVED_MASK, SVM_EXIT_ERR);
 	SVM_TEST_CR_RESERVED_BITS(32, 63, 4, 4, cr4_saved,
-	    SVM_CR4_RESERVED_MASK);
+	    SVM_CR4_RESERVED_MASK, SVM_EXIT_ERR);
 
 	vmcb->save.cr4 = cr4_saved;
 	vmcb->save.efer = efer_saved;
-- 
2.18.4

Re: [PATCH 3/4 v2] KVM: nSVM: Test non-MBZ reserved bits in CR3 in long mode

[Sean Christopherson]

On Mon, Sep 28, 2020 at 07:20:42AM +0000, Krish Sadhukhan wrote:
> According to section "CR3" in APM vol. 2, the non-MBZ reserved bits in CR3
> need to be set by software as follows:
> 
> 	"Reserved Bits. Reserved fields should be cleared to 0 by software
> 	when writing CR3."

Nothing in the shortlog or changelog actually states what this patch does.
"Test non-MBZ reserved bits in CR3 in long mode" is rather ambiguous, and
IIUC, the changelog is straight up misleading.

Based on the discussion from v1, I _think_ this test verifies that KVM does
_not_ fail nested VMRUN if non-MBZ bits are set, correct?

If so, then something like:

  KVM: nSVM: Verify non-MBZ CR3 reserved bits can be set in long mode

with further explanation in the changelog would be very helpful.

Re: [PATCH 3/4 v2] KVM: nSVM: Test non-MBZ reserved bits in CR3 in long mode

[Krish Sadhukhan]

On 9/28/20 8:11 PM, Sean Christopherson wrote:
> On Mon, Sep 28, 2020 at 07:20:42AM +0000, Krish Sadhukhan wrote:
>> According to section "CR3" in APM vol. 2, the non-MBZ reserved bits in CR3
>> need to be set by software as follows:
>>
>> 	"Reserved Bits. Reserved fields should be cleared to 0 by software
>> 	when writing CR3."
> Nothing in the shortlog or changelog actually states what this patch does.
> "Test non-MBZ reserved bits in CR3 in long mode" is rather ambiguous, and
> IIUC, the changelog is straight up misleading.
>
> Based on the discussion from v1, I _think_ this test verifies that KVM does
> _not_ fail nested VMRUN if non-MBZ bits are set, correct?

Not KVM, hardware rather.  Hardware doesn't consider it as an invalid 
guest state if non-MBZ reserved bits are set.
>
> If so, then something like:
>
>    KVM: nSVM: Verify non-MBZ CR3 reserved bits can be set in long mode
>
> with further explanation in the changelog would be very helpful.

Even though the non-MBZ reserved bits are ignored by the consistency 
checks in hardware, eventually page-table walks fail. So, I am wondering 
whether it is appropriate to say,

             "Verify non-MBZ CR3 reserved bits can be set in long mode"

because the test is inducing an artificial failure even before any guest 
instruction is executed. We are not entering the guest with these bits set.

I prefer to keep the commit header as is and rather expand the commit 
message to explain what I have described here. How about that ?

Re: [PATCH 3/4 v2] KVM: nSVM: Test non-MBZ reserved bits in CR3 in long mode

[Sean Christopherson]

On Wed, Sep 30, 2020 at 05:29:24PM -0700, Krish Sadhukhan wrote:
> 
> On 9/28/20 8:11 PM, Sean Christopherson wrote:
> >On Mon, Sep 28, 2020 at 07:20:42AM +0000, Krish Sadhukhan wrote:
> >>According to section "CR3" in APM vol. 2, the non-MBZ reserved bits in CR3
> >>need to be set by software as follows:
> >>
> >>	"Reserved Bits. Reserved fields should be cleared to 0 by software
> >>	when writing CR3."
> >Nothing in the shortlog or changelog actually states what this patch does.
> >"Test non-MBZ reserved bits in CR3 in long mode" is rather ambiguous, and
> >IIUC, the changelog is straight up misleading.
> >
> >Based on the discussion from v1, I _think_ this test verifies that KVM does
> >_not_ fail nested VMRUN if non-MBZ bits are set, correct?
> 
> Not KVM, hardware rather.  Hardware doesn't consider it as an invalid guest
> state if non-MBZ reserved bits are set.
> >
> >If so, then something like:
> >
> >   KVM: nSVM: Verify non-MBZ CR3 reserved bits can be set in long mode
> >
> >with further explanation in the changelog would be very helpful.
> 
> Even though the non-MBZ reserved bits are ignored by the consistency checks
> in hardware, eventually page-table walks fail. So, I am wondering whether it

Page table walks fail how?  Are you referring to forcing the #NPF, or does
the CPU puke on the non-MBZ reserved bits at some point?

> is appropriate to say,
> 
>             "Verify non-MBZ CR3 reserved bits can be set in long mode"
> 
> because the test is inducing an artificial failure even before any guest
> instruction is executed. We are not entering the guest with these bits set.

Yes we are, unless I'm misunderstanding how SVM handles VMRUN.  "entering" the
guest does not mean successfully executing guest code, it means loading guest
state and completing the world switch.  I don't think I'm misunderstanding,
because the test explicitly clears the NPT PML4[0]'s present bit to induce a
#NPF.  That means the CPU is fetching instructions, and again unless there's
details about NPT that I'm missing, the fact that the test sees a #NPF means
that the CPU successfully completed the GVA->GPA translation using the "bad"
CR3.

> I prefer to keep the commit header as is and rather expand the commit
> message to explain what I have described here. How about that ?

That's fine, so long as it documents both what the test is actually verifying
and what is/isn't legal.

Re: [PATCH 3/4 v2] KVM: nSVM: Test non-MBZ reserved bits in CR3 in long mode

[Krish Sadhukhan]

On 9/30/20 5:50 PM, Sean Christopherson wrote:
> On Wed, Sep 30, 2020 at 05:29:24PM -0700, Krish Sadhukhan wrote:
>> On 9/28/20 8:11 PM, Sean Christopherson wrote:
>>> On Mon, Sep 28, 2020 at 07:20:42AM +0000, Krish Sadhukhan wrote:
>>>> According to section "CR3" in APM vol. 2, the non-MBZ reserved bits in CR3
>>>> need to be set by software as follows:
>>>>
>>>> 	"Reserved Bits. Reserved fields should be cleared to 0 by software
>>>> 	when writing CR3."
>>> Nothing in the shortlog or changelog actually states what this patch does.
>>> "Test non-MBZ reserved bits in CR3 in long mode" is rather ambiguous, and
>>> IIUC, the changelog is straight up misleading.
>>>
>>> Based on the discussion from v1, I _think_ this test verifies that KVM does
>>> _not_ fail nested VMRUN if non-MBZ bits are set, correct?
>> Not KVM, hardware rather.  Hardware doesn't consider it as an invalid guest
>> state if non-MBZ reserved bits are set.
>>> If so, then something like:
>>>
>>>    KVM: nSVM: Verify non-MBZ CR3 reserved bits can be set in long mode
>>>
>>> with further explanation in the changelog would be very helpful.
>> Even though the non-MBZ reserved bits are ignored by the consistency checks
>> in hardware, eventually page-table walks fail. So, I am wondering whether it
> Page table walks fail how?  Are you referring to forcing the #NPF, or does
> the CPU puke on the non-MBZ reserved bits at some point?


I notice the following in my experiments when I don't clear the P bit in 
NPT PML4[0] to induce an
#NPF:

     1. In long mode and in legacy-PAE mode, guest VMMCALL is 
successfully executed and kvm_x86_ops.handle_exit() receives VMEXIT_VMMCALL.

     2. In legacy-non-PAE mode, kvm_x86_ops.handle_exit(), receives 
VMEXIT_NPF.

>
>> is appropriate to say,
>>
>>              "Verify non-MBZ CR3 reserved bits can be set in long mode"
>>
>> because the test is inducing an artificial failure even before any guest
>> instruction is executed. We are not entering the guest with these bits set.
> Yes we are, unless I'm misunderstanding how SVM handles VMRUN.  "entering" the
> guest does not mean successfully executing guest code, it means loading guest
> state and completing the world switch.  I don't think I'm misunderstanding,
> because the test explicitly clears the NPT PML4[0]'s present bit to induce a
> #NPF.  That means the CPU is fetching instructions, and again unless there's
> details about NPT that I'm missing, the fact that the test sees a #NPF means
> that the CPU successfully completed the GVA->GPA translation using the "bad"
> CR3.


You are right. According to APM, the hardware loads the guest state 
first and then does the consistency checking. So, yes, world switch 
happens before consistency checking begins.

>
>> I prefer to keep the commit header as is and rather expand the commit
>> message to explain what I have described here. How about that ?
> That's fine, so long as it documents both what the test is actually verifying
> and what is/isn't legal.

[PATCH 4/4 v2] KVM: nSVM: nested_vmcb_checks() needs to check all bits of EFER

[Krish Sadhukhan]

Current implementation of nested_vmcb_checks() checks only the SVME bit in
EFER. We need to check all other bits of EFER including the reserved bits.
This patch enhances nested_vmcb_checks() by calling kvm_valid_efer() which
checks all bits of EFER.

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

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 55c785587bd4..cf4048401737 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -244,7 +244,8 @@ static bool nested_vmcb_check_cr3_cr4(struct vcpu_svm *svm,
 
 static bool nested_vmcb_checks(struct vcpu_svm *svm, struct vmcb *vmcb)
 {
-	if ((vmcb->save.efer & EFER_SVME) == 0)
+	if (((vmcb->save.efer & EFER_SVME) == 0) ||
+	     !kvm_valid_efer(&(svm->vcpu), vmcb->save.efer))
 		return false;
 
 	if (((vmcb->save.cr0 & X86_CR0_CD) == 0) &&
-- 
2.18.4