kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field

According to the SDM, a VMWRITE in VMX non-root operation with an invalid VMCS-link pointer results in VMfailInvalid before the validity of the VMCS field in the secondary source operand is checked.

Fixes 6d894f498f5d1 ("KVM: nVMX: vmread/vmwrite: Use shadow vmcs12 if running L2")

Signed-off-by Jim Mattson <[email protected]>

Cc Liran Alon <[email protected]>

---
 arch/x86/kvm/vmx/nested.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

arch/x86/kvm/vmx/nested.c +0 -0

index 4aea7d304beb..146e1b40c69f 100644
@@ -4864,6 +4864,25 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
 	if (vmx->nested.current_vmptr == -1ull)
 		return nested_vmx_failInvalid(vcpu);

arch/x86/kvm/vmx/nested.c +10 -19

+	} else {
+		/*
+		 * When vmcs->vmcs_link_pointer is -1ull, any VMWRITE
+		 * to shadowed-field sets the ALU flags for VMfailInvalid.
+		 */
+		if (get_vmcs12(vcpu)->vmcs_link_pointer == -1ull)
+			return nested_vmx_failInvalid(vcpu);
+		vmcs12 = get_shadow_vmcs12(vcpu);
+	}
+
 	if (vmx_instruction_info & (1u << 10))
 		field_value = kvm_register_readl(vcpu,
 			(((vmx_instruction_info) >> 3) & 0xf));

@@ -4889,25 +4908,6 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
 		return nested_vmx_failValid(vcpu,
 			VMXERR_VMWRITE_READ_ONLY_VMCS_COMPONENT);
 
-	if (!is_guest_mode(vcpu)) {
-		vmcs12 = get_vmcs12(vcpu);
-
-		/*
-		 * Ensure vmcs12 is up-to-date before any VMWRITE that dirties
-		 * vmcs12, else we may crush a field or consume a stale value.
-		 */
-		if (!is_shadow_field_rw(field))
-			copy_vmcs02_to_vmcs12_rare(vcpu, vmcs12);
-	} else {
-		/*
-		 * When vmcs->vmcs_link_pointer is -1ull, any VMWRITE
-		 * to shadowed-field sets the ALU flags for VMfailInvalid.
-		 */
-		if (get_vmcs12(vcpu)->vmcs_link_pointer == -1ull)
-			return nested_vmx_failInvalid(vcpu);
-		vmcs12 = get_shadow_vmcs12(vcpu);
-	}
-
 	offset = vmcs_field_to_offset(field);
 	if (offset < 0)
 		return nested_vmx_failValid(vcpu,

According to the SDM, a VMWRITE in VMX non-root operation with an invalid VMCS-link pointer results in VMfailInvalid before the validity of the VMCS field in the secondary source operand is checked.

Fixes 6d894f498f5d1 ("KVM: nVMX: vmread/vmwrite: Use shadow vmcs12 if running L2")

Signed-off-by Jim Mattson <[email protected]>

Cc Liran Alon <[email protected]>

---
 arch/x86/kvm/vmx/nested.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

Quoted 4 lines · Jim Mattson

According to the SDM, a VMWRITE in VMX non-root operation with an invalid VMCS-link pointer results in VMfailInvalid before the validity of the VMCS field in the secondary source operand is checked.

Quoted 6 lines

Fixes 6d894f498f5d1 ("KVM: nVMX: vmread/vmwrite: Use shadow vmcs12 if running L2")

Signed-off-by Jim Mattson <[email protected]>

Cc Liran Alon <[email protected]>

---
 arch/x86/kvm/vmx/nested.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

As Vitaly pointed out, the test must be split in two, like this:

---------------- 8< -----------------------
From 3b9d87060e800ffae2bd19da94ede05018066c87 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <[email protected]>
Date: Thu, 5 Dec 2019 12:39:07 +0100
Subject: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field

According to the SDM, a VMWRITE in VMX non-root operation with an invalid VMCS-link pointer results in VMfailInvalid before the validity of the VMCS field in the secondary source operand is checked.

While cleaning up handle_vmwrite, make the code of handle_vmread look the same, too.

Fixes 6d894f498f5d1 ("KVM: nVMX: vmread/vmwrite: Use shadow vmcs12 if running L2")

Signed-off-by Jim Mattson <[email protected]>

Cc Liran Alon <[email protected]>

arch/x86/kvm/vmx/nested.c +20 -22

index 4aea7d304beb..c080a879b95d 100644
@@ -4767,14 +4767,13 @@ static int handle_vmread(struct kvm_vcpu *vcpu)
        if (to_vmx(vcpu)->nested.current_vmptr == -1ull)
                return nested_vmx_failInvalid(vcpu);
 
-       if (!is_guest_mode(vcpu))
-               vmcs12 = get_vmcs12(vcpu);
-       else {
+       vmcs12 = get_vmcs12(vcpu);
+       if (is_guest_mode(vcpu)) {
                /*
                 * When vmcs->vmcs_link_pointer is -1ull, any VMREAD
                 * to shadowed-field sets the ALU flags for VMfailInvalid.
                 */
-               if (get_vmcs12(vcpu)->vmcs_link_pointer == -1ull)
+               if (vmcs12->vmcs_link_pointer == -1ull)
                        return nested_vmx_failInvalid(vcpu);
                vmcs12 = get_shadow_vmcs12(vcpu);
        }

@@ -4878,8 +4877,19 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
                }
        }
 
+       vmcs12 = get_vmcs12(vcpu);
+       if (is_guest_mode(vcpu)) {
+               /*
+                * When vmcs->vmcs_link_pointer is -1ull, any VMWRITE
+                * to shadowed-field sets the ALU flags for VMfailInvalid.
+                */
+               if (vmcs12->vmcs_link_pointer == -1ull)
+                       return nested_vmx_failInvalid(vcpu);
+               vmcs12 = get_shadow_vmcs12(vcpu);
+       }
 
        field = kvm_register_readl(vcpu, (((vmx_instruction_info) >> 28) & 0xf));
+
        /*
         * If the vCPU supports "VMWRITE to any supported field in the
         * VMCS," then the "read-only" fields are actually read/write.

@@ -4889,24 +4899,12 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
                return nested_vmx_failValid(vcpu,
                        VMXERR_VMWRITE_READ_ONLY_VMCS_COMPONENT);
 
-       if (!is_guest_mode(vcpu)) {
-               vmcs12 = get_vmcs12(vcpu);
-
-               /*
-                * Ensure vmcs12 is up-to-date before any VMWRITE that dirties
-                * vmcs12, else we may crush a field or consume a stale value.
-                */
-               if (!is_shadow_field_rw(field))
-                       copy_vmcs02_to_vmcs12_rare(vcpu, vmcs12);
-       } else {
-               /*
-                * When vmcs->vmcs_link_pointer is -1ull, any VMWRITE
-                * to shadowed-field sets the ALU flags for VMfailInvalid.
-                */
-               if (get_vmcs12(vcpu)->vmcs_link_pointer == -1ull)
-                       return nested_vmx_failInvalid(vcpu);
-               vmcs12 = get_shadow_vmcs12(vcpu);
-       }
+       /*
+        * Ensure vmcs12 is up-to-date before any VMWRITE that dirties
+        * vmcs12, else we may crush a field or consume a stale value.
+        */
+       if (!is_guest_mode(vcpu) && !is_shadow_field_rw(field))
+               copy_vmcs02_to_vmcs12_rare(vcpu, vmcs12);
 
        offset = vmcs_field_to_offset(field);
        if (offset < 0)

... and also, do you have a matching kvm-unit-tests patch?

Thanks,

Paolo

Quoted 6 lines · Paolo Bonzini

Quoted 4 lines · Jim Mattson

According to the SDM, a VMWRITE in VMX non-root operation with an invalid VMCS-link pointer results in VMfailInvalid before the validity of the VMCS field in the secondary source operand is checked.

Quoted 8 lines

Quoted 6 lines

Fixes 6d894f498f5d1 ("KVM: nVMX: vmread/vmwrite: Use shadow vmcs12 if running L2")

Signed-off-by Jim Mattson <[email protected]>

Cc Liran Alon <[email protected]>

---
 arch/x86/kvm/vmx/nested.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

As Vitaly pointed out, the test must be split in two, like this:

Right. Odd that no kvm-unit-tests noticed.

Quoted 6 lines

---------------- 8< -----------------------
From 3b9d87060e800ffae2bd19da94ede05018066c87 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <[email protected]>
Date: Thu, 5 Dec 2019 12:39:07 +0100
Subject: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field

Quoted 6 lines

According to the SDM, a VMWRITE in VMX non-root operation with an invalid VMCS-link pointer results in VMfailInvalid before the validity of the VMCS field in the secondary source operand is checked.

While cleaning up handle_vmwrite, make the code of handle_vmread look the same, too.

Okay.

Quoted 75 lines

Fixes 6d894f498f5d1 ("KVM: nVMX: vmread/vmwrite: Use shadow vmcs12 if running L2")

Signed-off-by Jim Mattson <[email protected]>

Cc Liran Alon <[email protected]>

arch/x86/kvm/vmx/nested.c +20 -22

index 4aea7d304beb..c080a879b95d 100644
@@ -4767,14 +4767,13 @@ static int handle_vmread(struct kvm_vcpu *vcpu)
        if (to_vmx(vcpu)->nested.current_vmptr == -1ull)
                return nested_vmx_failInvalid(vcpu);
 
-       if (!is_guest_mode(vcpu))
-               vmcs12 = get_vmcs12(vcpu);
-       else {
+       vmcs12 = get_vmcs12(vcpu);
+       if (is_guest_mode(vcpu)) {
                /*
                 * When vmcs->vmcs_link_pointer is -1ull, any VMREAD
                 * to shadowed-field sets the ALU flags for VMfailInvalid.
                 */
-               if (get_vmcs12(vcpu)->vmcs_link_pointer == -1ull)
+               if (vmcs12->vmcs_link_pointer == -1ull)
                        return nested_vmx_failInvalid(vcpu);
                vmcs12 = get_shadow_vmcs12(vcpu);
        }

@@ -4878,8 +4877,19 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
                }
        }
 
+       vmcs12 = get_vmcs12(vcpu);
+       if (is_guest_mode(vcpu)) {
+               /*
+                * When vmcs->vmcs_link_pointer is -1ull, any VMWRITE
+                * to shadowed-field sets the ALU flags for VMfailInvalid.
+                */
+               if (vmcs12->vmcs_link_pointer == -1ull)
+                       return nested_vmx_failInvalid(vcpu);
+               vmcs12 = get_shadow_vmcs12(vcpu);
+       }
 
        field = kvm_register_readl(vcpu, (((vmx_instruction_info) >> 28) & 0xf));
+
        /*
         * If the vCPU supports "VMWRITE to any supported field in the
         * VMCS," then the "read-only" fields are actually read/write.

@@ -4889,24 +4899,12 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
                return nested_vmx_failValid(vcpu,
                        VMXERR_VMWRITE_READ_ONLY_VMCS_COMPONENT);
 
-       if (!is_guest_mode(vcpu)) {
-               vmcs12 = get_vmcs12(vcpu);
-
-               /*
-                * Ensure vmcs12 is up-to-date before any VMWRITE that dirties
-                * vmcs12, else we may crush a field or consume a stale value.
-                */
-               if (!is_shadow_field_rw(field))
-                       copy_vmcs02_to_vmcs12_rare(vcpu, vmcs12);
-       } else {
-               /*
-                * When vmcs->vmcs_link_pointer is -1ull, any VMWRITE
-                * to shadowed-field sets the ALU flags for VMfailInvalid.
-                */
-               if (get_vmcs12(vcpu)->vmcs_link_pointer == -1ull)
-                       return nested_vmx_failInvalid(vcpu);
-               vmcs12 = get_shadow_vmcs12(vcpu);
-       }
+       /*
+        * Ensure vmcs12 is up-to-date before any VMWRITE that dirties
+        * vmcs12, else we may crush a field or consume a stale value.
+        */
+       if (!is_guest_mode(vcpu) && !is_shadow_field_rw(field))
+               copy_vmcs02_to_vmcs12_rare(vcpu, vmcs12);
 

Quoted 5 lines

arch/x86/kvm/vmx/nested.c +0 -0

        offset = vmcs_field_to_offset(field);
        if (offset < 0)

... and also, do you have a matching kvm-unit-tests patch?

I'll put one together, along with a test that shows the current priority inversion between read-only and unsupported VMCS fields.

Quoted 4 lines

Thanks,

Paolo

Quoted 113 lines · Jim Mattson

Quoted 6 lines · Paolo Bonzini

Quoted 4 lines · Jim Mattson

According to the SDM, a VMWRITE in VMX non-root operation with an invalid VMCS-link pointer results in VMfailInvalid before the validity of the VMCS field in the secondary source operand is checked.

Quoted 8 lines

Quoted 6 lines

Fixes 6d894f498f5d1 ("KVM: nVMX: vmread/vmwrite: Use shadow vmcs12 if running L2")

Signed-off-by Jim Mattson <[email protected]>

Cc Liran Alon <[email protected]>

---
arch/x86/kvm/vmx/nested.c | 38 +++++++++++++++++++-------------------

1 file changed, 19 insertions(+), 19 deletions(-)

As Vitaly pointed out, the test must be split in two, like this:

Right. Odd that no kvm-unit-tests noticed.

Quoted 6 lines

---------------- 8< -----------------------
From 3b9d87060e800ffae2bd19da94ede05018066c87 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <[email protected]>
Date: Thu, 5 Dec 2019 12:39:07 +0100
Subject: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field

Quoted 6 lines

According to the SDM, a VMWRITE in VMX non-root operation with an invalid VMCS-link pointer results in VMfailInvalid before the validity of the VMCS field in the secondary source operand is checked.

While cleaning up handle_vmwrite, make the code of handle_vmread look the same, too.

Okay.

Quoted 79 lines

Fixes 6d894f498f5d1 ("KVM: nVMX: vmread/vmwrite: Use shadow vmcs12 if running L2")

Signed-off-by Jim Mattson <[email protected]>

Cc Liran Alon <[email protected]>

arch/x86/kvm/vmx/nested.c +20 -22

index 4aea7d304beb..c080a879b95d 100644
@@ -4767,14 +4767,13 @@ static int handle_vmread(struct kvm_vcpu *vcpu)
       if (to_vmx(vcpu)->nested.current_vmptr == -1ull)
               return nested_vmx_failInvalid(vcpu);
 
-       if (!is_guest_mode(vcpu))
-               vmcs12 = get_vmcs12(vcpu);
-       else {
+       vmcs12 = get_vmcs12(vcpu);
+       if (is_guest_mode(vcpu)) {
               /*
                * When vmcs->vmcs_link_pointer is -1ull, any VMREAD
                * to shadowed-field sets the ALU flags for VMfailInvalid.
                */
-               if (get_vmcs12(vcpu)->vmcs_link_pointer == -1ull)
+               if (vmcs12->vmcs_link_pointer == -1ull)
                       return nested_vmx_failInvalid(vcpu);
               vmcs12 = get_shadow_vmcs12(vcpu);
       }

@@ -4878,8 +4877,19 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
               }
       }
 
+       vmcs12 = get_vmcs12(vcpu);
+       if (is_guest_mode(vcpu)) {
+               /*
+                * When vmcs->vmcs_link_pointer is -1ull, any VMWRITE
+                * to shadowed-field sets the ALU flags for VMfailInvalid.
+                */
+               if (vmcs12->vmcs_link_pointer == -1ull)
+                       return nested_vmx_failInvalid(vcpu);
+               vmcs12 = get_shadow_vmcs12(vcpu);
+       }
 
       field = kvm_register_readl(vcpu, (((vmx_instruction_info) >> 28) & 0xf));
+
       /*
        * If the vCPU supports "VMWRITE to any supported field in the
        * VMCS," then the "read-only" fields are actually read/write.

@@ -4889,24 +4899,12 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
               return nested_vmx_failValid(vcpu,
                       VMXERR_VMWRITE_READ_ONLY_VMCS_COMPONENT);
 
-       if (!is_guest_mode(vcpu)) {
-               vmcs12 = get_vmcs12(vcpu);
-
-               /*
-                * Ensure vmcs12 is up-to-date before any VMWRITE that dirties
-                * vmcs12, else we may crush a field or consume a stale value.
-                */
-               if (!is_shadow_field_rw(field))
-                       copy_vmcs02_to_vmcs12_rare(vcpu, vmcs12);
-       } else {
-               /*
-                * When vmcs->vmcs_link_pointer is -1ull, any VMWRITE
-                * to shadowed-field sets the ALU flags for VMfailInvalid.
-                */
-               if (get_vmcs12(vcpu)->vmcs_link_pointer == -1ull)
-                       return nested_vmx_failInvalid(vcpu);
-               vmcs12 = get_shadow_vmcs12(vcpu);
-       }
+       /*
+        * Ensure vmcs12 is up-to-date before any VMWRITE that dirties
+        * vmcs12, else we may crush a field or consume a stale value.
+        */
+       if (!is_guest_mode(vcpu) && !is_shadow_field_rw(field))
+               copy_vmcs02_to_vmcs12_rare(vcpu, vmcs12);
 
       offset = vmcs_field_to_offset(field);
       if (offset < 0)

Quoted 4 lines

Quoted 1 line

... and also, do you have a matching kvm-unit-tests patch?

I'll put one together, along with a test that shows the current priority inversion between read-only and unsupported VMCS fields.

I can't figure out how to clear IA32_VMX_MISC[bit 29] in qemu, so I'm going to add the test to tools/testing/selftests/kvm instead.

Quoted 119 lines · Jim Mattson

Quoted 113 lines · Jim Mattson

Quoted 6 lines · Paolo Bonzini

Quoted 4 lines · Jim Mattson

According to the SDM, a VMWRITE in VMX non-root operation with an invalid VMCS-link pointer results in VMfailInvalid before the validity of the VMCS field in the secondary source operand is checked.

Quoted 8 lines

Quoted 6 lines

Fixes 6d894f498f5d1 ("KVM: nVMX: vmread/vmwrite: Use shadow vmcs12 if running L2")

Signed-off-by Jim Mattson <[email protected]>

Cc Liran Alon <[email protected]>

---
arch/x86/kvm/vmx/nested.c | 38 +++++++++++++++++++-------------------

1 file changed, 19 insertions(+), 19 deletions(-)

As Vitaly pointed out, the test must be split in two, like this:

Right. Odd that no kvm-unit-tests noticed.

Quoted 6 lines

---------------- 8< -----------------------
From 3b9d87060e800ffae2bd19da94ede05018066c87 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <[email protected]>
Date: Thu, 5 Dec 2019 12:39:07 +0100
Subject: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field

Quoted 6 lines

According to the SDM, a VMWRITE in VMX non-root operation with an invalid VMCS-link pointer results in VMfailInvalid before the validity of the VMCS field in the secondary source operand is checked.

While cleaning up handle_vmwrite, make the code of handle_vmread look the same, too.

Okay.

Quoted 79 lines

Fixes 6d894f498f5d1 ("KVM: nVMX: vmread/vmwrite: Use shadow vmcs12 if running L2")

Signed-off-by Jim Mattson <[email protected]>

Cc Liran Alon <[email protected]>

arch/x86/kvm/vmx/nested.c +20 -22

index 4aea7d304beb..c080a879b95d 100644
@@ -4767,14 +4767,13 @@ static int handle_vmread(struct kvm_vcpu *vcpu)
       if (to_vmx(vcpu)->nested.current_vmptr == -1ull)
               return nested_vmx_failInvalid(vcpu);
 
-       if (!is_guest_mode(vcpu))
-               vmcs12 = get_vmcs12(vcpu);
-       else {
+       vmcs12 = get_vmcs12(vcpu);
+       if (is_guest_mode(vcpu)) {
               /*
                * When vmcs->vmcs_link_pointer is -1ull, any VMREAD
                * to shadowed-field sets the ALU flags for VMfailInvalid.
                */
-               if (get_vmcs12(vcpu)->vmcs_link_pointer == -1ull)
+               if (vmcs12->vmcs_link_pointer == -1ull)
                       return nested_vmx_failInvalid(vcpu);
               vmcs12 = get_shadow_vmcs12(vcpu);
       }

@@ -4878,8 +4877,19 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
               }
       }
 
+       vmcs12 = get_vmcs12(vcpu);
+       if (is_guest_mode(vcpu)) {
+               /*
+                * When vmcs->vmcs_link_pointer is -1ull, any VMWRITE
+                * to shadowed-field sets the ALU flags for VMfailInvalid.
+                */
+               if (vmcs12->vmcs_link_pointer == -1ull)
+                       return nested_vmx_failInvalid(vcpu);
+               vmcs12 = get_shadow_vmcs12(vcpu);
+       }
 
       field = kvm_register_readl(vcpu, (((vmx_instruction_info) >> 28) & 0xf));
+
       /*
        * If the vCPU supports "VMWRITE to any supported field in the
        * VMCS," then the "read-only" fields are actually read/write.

@@ -4889,24 +4899,12 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
               return nested_vmx_failValid(vcpu,
                       VMXERR_VMWRITE_READ_ONLY_VMCS_COMPONENT);
 
-       if (!is_guest_mode(vcpu)) {
-               vmcs12 = get_vmcs12(vcpu);
-
-               /*
-                * Ensure vmcs12 is up-to-date before any VMWRITE that dirties
-                * vmcs12, else we may crush a field or consume a stale value.
-                */
-               if (!is_shadow_field_rw(field))
-                       copy_vmcs02_to_vmcs12_rare(vcpu, vmcs12);
-       } else {
-               /*
-                * When vmcs->vmcs_link_pointer is -1ull, any VMWRITE
-                * to shadowed-field sets the ALU flags for VMfailInvalid.
-                */
-               if (get_vmcs12(vcpu)->vmcs_link_pointer == -1ull)
-                       return nested_vmx_failInvalid(vcpu);
-               vmcs12 = get_shadow_vmcs12(vcpu);
-       }
+       /*
+        * Ensure vmcs12 is up-to-date before any VMWRITE that dirties
+        * vmcs12, else we may crush a field or consume a stale value.
+        */
+       if (!is_guest_mode(vcpu) && !is_shadow_field_rw(field))
+               copy_vmcs02_to_vmcs12_rare(vcpu, vmcs12);
 
       offset = vmcs_field_to_offset(field);
       if (offset < 0)

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

... and also, do you have a matching kvm-unit-tests patch?

I'll put one together, along with a test that shows the current priority inversion between read-only and unsupported VMCS fields.

I can't figure out how to clear IA32_VMX_MISC[bit 29] in qemu, so I'm going to add the test to tools/testing/selftests/kvm instead.

Please don’t.

I wish that we keep clear separation between kvm-unit-tests and self-tests. In the sense that kvm-unit-tests tests for correct CPU behaviour semantics and self-tests tests for correctness of KVM userspace API.

In the future, I wish to change kvm-unit-tests to cpu-unit-tests. As there is no real connection to KVM. It’s a bunch of tests that can be run on top of any CPU Implementation (weather vCPU by some hypervisor or bare-metal CPU) and
test for it’s semantics.
I have already used this to find semantic issues on Hyper-V vCPU implementation for example.

Regarding your question on how to disable IA32_VMX_MISC in QEMU:
Paolo have recently created a patch-series for QEMU that can be used to do this. (Aimed for QEMU nVMX Live-Migration support)
See: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00711.html
(You should search for final patch-series version…)

-Liran

Quoted 117 lines · Jim Mattson

Quoted 111 lines · Jim Mattson

Quoted 5 lines · Paolo Bonzini

Quoted 4 lines · Jim Mattson

According to the SDM, a VMWRITE in VMX non-root operation with an invalid VMCS-link pointer results in VMfailInvalid before the validity of the VMCS field in the secondary source operand is checked.

Quoted 8 lines

Quoted 6 lines

Fixes 6d894f498f5d1 ("KVM: nVMX: vmread/vmwrite: Use shadow vmcs12 if running L2")

Signed-off-by Jim Mattson <[email protected]>

Cc Liran Alon <[email protected]>

---
arch/x86/kvm/vmx/nested.c | 38 +++++++++++++++++++-------------------

1 file changed, 19 insertions(+), 19 deletions(-)

As Vitaly pointed out, the test must be split in two, like this:

Right. Odd that no kvm-unit-tests noticed.

Quoted 6 lines

---------------- 8< -----------------------
From 3b9d87060e800ffae2bd19da94ede05018066c87 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <[email protected]>
Date: Thu, 5 Dec 2019 12:39:07 +0100
Subject: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field

Quoted 6 lines

According to the SDM, a VMWRITE in VMX non-root operation with an invalid VMCS-link pointer results in VMfailInvalid before the validity of the VMCS field in the secondary source operand is checked.

While cleaning up handle_vmwrite, make the code of handle_vmread look the same, too.

Okay.

Quoted 79 lines

Fixes 6d894f498f5d1 ("KVM: nVMX: vmread/vmwrite: Use shadow vmcs12 if running L2")

Signed-off-by Jim Mattson <[email protected]>

Cc Liran Alon <[email protected]>

arch/x86/kvm/vmx/nested.c +20 -22

index 4aea7d304beb..c080a879b95d 100644
@@ -4767,14 +4767,13 @@ static int handle_vmread(struct kvm_vcpu *vcpu)
      if (to_vmx(vcpu)->nested.current_vmptr == -1ull)
              return nested_vmx_failInvalid(vcpu);
 
-       if (!is_guest_mode(vcpu))
-               vmcs12 = get_vmcs12(vcpu);
-       else {
+       vmcs12 = get_vmcs12(vcpu);
+       if (is_guest_mode(vcpu)) {
              /*
               * When vmcs->vmcs_link_pointer is -1ull, any VMREAD
               * to shadowed-field sets the ALU flags for VMfailInvalid.
               */
-               if (get_vmcs12(vcpu)->vmcs_link_pointer == -1ull)
+               if (vmcs12->vmcs_link_pointer == -1ull)
                      return nested_vmx_failInvalid(vcpu);
              vmcs12 = get_shadow_vmcs12(vcpu);
      }

@@ -4878,8 +4877,19 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
              }
      }
 
+       vmcs12 = get_vmcs12(vcpu);
+       if (is_guest_mode(vcpu)) {
+               /*
+                * When vmcs->vmcs_link_pointer is -1ull, any VMWRITE
+                * to shadowed-field sets the ALU flags for VMfailInvalid.
+                */
+               if (vmcs12->vmcs_link_pointer == -1ull)
+                       return nested_vmx_failInvalid(vcpu);
+               vmcs12 = get_shadow_vmcs12(vcpu);
+       }
 
      field = kvm_register_readl(vcpu, (((vmx_instruction_info) >> 28) & 0xf));
+
      /*
       * If the vCPU supports "VMWRITE to any supported field in the
       * VMCS," then the "read-only" fields are actually read/write.

@@ -4889,24 +4899,12 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
              return nested_vmx_failValid(vcpu,
                      VMXERR_VMWRITE_READ_ONLY_VMCS_COMPONENT);
 
-       if (!is_guest_mode(vcpu)) {
-               vmcs12 = get_vmcs12(vcpu);
-
-               /*
-                * Ensure vmcs12 is up-to-date before any VMWRITE that dirties
-                * vmcs12, else we may crush a field or consume a stale value.
-                */
-               if (!is_shadow_field_rw(field))
-                       copy_vmcs02_to_vmcs12_rare(vcpu, vmcs12);
-       } else {
-               /*
-                * When vmcs->vmcs_link_pointer is -1ull, any VMWRITE
-                * to shadowed-field sets the ALU flags for VMfailInvalid.
-                */
-               if (get_vmcs12(vcpu)->vmcs_link_pointer == -1ull)
-                       return nested_vmx_failInvalid(vcpu);
-               vmcs12 = get_shadow_vmcs12(vcpu);
-       }
+       /*
+        * Ensure vmcs12 is up-to-date before any VMWRITE that dirties
+        * vmcs12, else we may crush a field or consume a stale value.
+        */
+       if (!is_guest_mode(vcpu) && !is_shadow_field_rw(field))
+               copy_vmcs02_to_vmcs12_rare(vcpu, vmcs12);
 
      offset = vmcs_field_to_offset(field);
      if (offset < 0)

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

... and also, do you have a matching kvm-unit-tests patch?

I'll put one together, along with a test that shows the current priority inversion between read-only and unsupported VMCS fields.

I can't figure out how to clear IA32_VMX_MISC[bit 29] in qemu, so I'm going to add the test to tools/testing/selftests/kvm instead.

Please don’t.

I wish that we keep clear separation between kvm-unit-tests and self-tests. In the sense that kvm-unit-tests tests for correct CPU behaviour semantics and self-tests tests for correctness of KVM userspace API.

In the future, I wish to change kvm-unit-tests to cpu-unit-tests. As there is no real connection to KVM. It’s a bunch of tests that can be run on top of any CPU Implementation (weather vCPU by some hypervisor or bare-metal CPU) and
test for it’s semantics.
I have already used this to find semantic issues on Hyper-V vCPU implementation for example.

Quoted 133 lines · Liran Alon

Quoted 117 lines · Jim Mattson

Quoted 111 lines · Jim Mattson

Quoted 5 lines · Paolo Bonzini

Quoted 4 lines · Jim Mattson

According to the SDM, a VMWRITE in VMX non-root operation with an invalid VMCS-link pointer results in VMfailInvalid before the validity of the VMCS field in the secondary source operand is checked.

Quoted 8 lines

Quoted 6 lines

Fixes 6d894f498f5d1 ("KVM: nVMX: vmread/vmwrite: Use shadow vmcs12 if running L2")

Signed-off-by Jim Mattson <[email protected]>

Cc Liran Alon <[email protected]>

---
arch/x86/kvm/vmx/nested.c | 38 +++++++++++++++++++-------------------

1 file changed, 19 insertions(+), 19 deletions(-)

As Vitaly pointed out, the test must be split in two, like this:

Right. Odd that no kvm-unit-tests noticed.

Quoted 6 lines

---------------- 8< -----------------------
From 3b9d87060e800ffae2bd19da94ede05018066c87 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <[email protected]>
Date: Thu, 5 Dec 2019 12:39:07 +0100
Subject: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field

Quoted 6 lines

According to the SDM, a VMWRITE in VMX non-root operation with an invalid VMCS-link pointer results in VMfailInvalid before the validity of the VMCS field in the secondary source operand is checked.

While cleaning up handle_vmwrite, make the code of handle_vmread look the same, too.

Okay.

Quoted 79 lines

Fixes 6d894f498f5d1 ("KVM: nVMX: vmread/vmwrite: Use shadow vmcs12 if running L2")

Signed-off-by Jim Mattson <[email protected]>

Cc Liran Alon <[email protected]>

arch/x86/kvm/vmx/nested.c +20 -22

index 4aea7d304beb..c080a879b95d 100644
@@ -4767,14 +4767,13 @@ static int handle_vmread(struct kvm_vcpu *vcpu)
     if (to_vmx(vcpu)->nested.current_vmptr == -1ull)
             return nested_vmx_failInvalid(vcpu);
 
-       if (!is_guest_mode(vcpu))
-               vmcs12 = get_vmcs12(vcpu);
-       else {
+       vmcs12 = get_vmcs12(vcpu);
+       if (is_guest_mode(vcpu)) {
             /*
              * When vmcs->vmcs_link_pointer is -1ull, any VMREAD
              * to shadowed-field sets the ALU flags for VMfailInvalid.
              */
-               if (get_vmcs12(vcpu)->vmcs_link_pointer == -1ull)
+               if (vmcs12->vmcs_link_pointer == -1ull)
                     return nested_vmx_failInvalid(vcpu);
             vmcs12 = get_shadow_vmcs12(vcpu);
     }

@@ -4878,8 +4877,19 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
             }
     }
 
+       vmcs12 = get_vmcs12(vcpu);
+       if (is_guest_mode(vcpu)) {
+               /*
+                * When vmcs->vmcs_link_pointer is -1ull, any VMWRITE
+                * to shadowed-field sets the ALU flags for VMfailInvalid.
+                */
+               if (vmcs12->vmcs_link_pointer == -1ull)
+                       return nested_vmx_failInvalid(vcpu);
+               vmcs12 = get_shadow_vmcs12(vcpu);
+       }
 
     field = kvm_register_readl(vcpu, (((vmx_instruction_info) >> 28) & 0xf));
+
     /*
      * If the vCPU supports "VMWRITE to any supported field in the
      * VMCS," then the "read-only" fields are actually read/write.

@@ -4889,24 +4899,12 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
             return nested_vmx_failValid(vcpu,
                     VMXERR_VMWRITE_READ_ONLY_VMCS_COMPONENT);
 
-       if (!is_guest_mode(vcpu)) {
-               vmcs12 = get_vmcs12(vcpu);
-
-               /*
-                * Ensure vmcs12 is up-to-date before any VMWRITE that dirties
-                * vmcs12, else we may crush a field or consume a stale value.
-                */
-               if (!is_shadow_field_rw(field))
-                       copy_vmcs02_to_vmcs12_rare(vcpu, vmcs12);
-       } else {
-               /*
-                * When vmcs->vmcs_link_pointer is -1ull, any VMWRITE
-                * to shadowed-field sets the ALU flags for VMfailInvalid.
-                */
-               if (get_vmcs12(vcpu)->vmcs_link_pointer == -1ull)
-                       return nested_vmx_failInvalid(vcpu);
-               vmcs12 = get_shadow_vmcs12(vcpu);
-       }
+       /*
+        * Ensure vmcs12 is up-to-date before any VMWRITE that dirties
+        * vmcs12, else we may crush a field or consume a stale value.
+        */
+       if (!is_guest_mode(vcpu) && !is_shadow_field_rw(field))
+               copy_vmcs02_to_vmcs12_rare(vcpu, vmcs12);
 
     offset = vmcs_field_to_offset(field);
     if (offset < 0)

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

... and also, do you have a matching kvm-unit-tests patch?

I'll put one together, along with a test that shows the current priority inversion between read-only and unsupported VMCS fields.

I can't figure out how to clear IA32_VMX_MISC[bit 29] in qemu, so I'm going to add the test to tools/testing/selftests/kvm instead.

Please don’t.

I wish that we keep clear separation between kvm-unit-tests and self-tests. In the sense that kvm-unit-tests tests for correct CPU behaviour semantics and self-tests tests for correctness of KVM userspace API.

Quoted 5 lines

In the future, I wish to change kvm-unit-tests to cpu-unit-tests. As there is no real connection to KVM. It’s a bunch of tests that can be run on top of any CPU Implementation (weather vCPU by some hypervisor or bare-metal CPU) and
test for it’s semantics.
I have already used this to find semantic issues on Hyper-V vCPU implementation for example.

Did you use for the matter the “infrastructure” that I added?

Quoted 143 lines · Nadav Amit

Quoted 138 lines · Liran Alon

Quoted 117 lines · Jim Mattson

Quoted 111 lines · Jim Mattson

Quoted 5 lines · Paolo Bonzini

Quoted 4 lines · Jim Mattson

According to the SDM, a VMWRITE in VMX non-root operation with an invalid VMCS-link pointer results in VMfailInvalid before the validity of the VMCS field in the secondary source operand is checked.

Quoted 8 lines

Quoted 6 lines

Fixes 6d894f498f5d1 ("KVM: nVMX: vmread/vmwrite: Use shadow vmcs12 if running L2")

Signed-off-by Jim Mattson <[email protected]>

Cc Liran Alon <[email protected]>

---
arch/x86/kvm/vmx/nested.c | 38 +++++++++++++++++++-------------------

1 file changed, 19 insertions(+), 19 deletions(-)

As Vitaly pointed out, the test must be split in two, like this:

Right. Odd that no kvm-unit-tests noticed.

Quoted 6 lines

---------------- 8< -----------------------
From 3b9d87060e800ffae2bd19da94ede05018066c87 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <[email protected]>
Date: Thu, 5 Dec 2019 12:39:07 +0100
Subject: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field

Quoted 6 lines

According to the SDM, a VMWRITE in VMX non-root operation with an invalid VMCS-link pointer results in VMfailInvalid before the validity of the VMCS field in the secondary source operand is checked.

While cleaning up handle_vmwrite, make the code of handle_vmread look the same, too.

Okay.

Quoted 79 lines

Fixes 6d894f498f5d1 ("KVM: nVMX: vmread/vmwrite: Use shadow vmcs12 if running L2")

Signed-off-by Jim Mattson <[email protected]>

Cc Liran Alon <[email protected]>

arch/x86/kvm/vmx/nested.c +20 -22

index 4aea7d304beb..c080a879b95d 100644
@@ -4767,14 +4767,13 @@ static int handle_vmread(struct kvm_vcpu *vcpu)
    if (to_vmx(vcpu)->nested.current_vmptr == -1ull)
            return nested_vmx_failInvalid(vcpu);
 
-       if (!is_guest_mode(vcpu))
-               vmcs12 = get_vmcs12(vcpu);
-       else {
+       vmcs12 = get_vmcs12(vcpu);
+       if (is_guest_mode(vcpu)) {
            /*
             * When vmcs->vmcs_link_pointer is -1ull, any VMREAD
             * to shadowed-field sets the ALU flags for VMfailInvalid.
             */
-               if (get_vmcs12(vcpu)->vmcs_link_pointer == -1ull)
+               if (vmcs12->vmcs_link_pointer == -1ull)
                    return nested_vmx_failInvalid(vcpu);
            vmcs12 = get_shadow_vmcs12(vcpu);
    }

@@ -4878,8 +4877,19 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
            }
    }
 
+       vmcs12 = get_vmcs12(vcpu);
+       if (is_guest_mode(vcpu)) {
+               /*
+                * When vmcs->vmcs_link_pointer is -1ull, any VMWRITE
+                * to shadowed-field sets the ALU flags for VMfailInvalid.
+                */
+               if (vmcs12->vmcs_link_pointer == -1ull)
+                       return nested_vmx_failInvalid(vcpu);
+               vmcs12 = get_shadow_vmcs12(vcpu);
+       }
 
    field = kvm_register_readl(vcpu, (((vmx_instruction_info) >> 28) & 0xf));
+
    /*
     * If the vCPU supports "VMWRITE to any supported field in the
     * VMCS," then the "read-only" fields are actually read/write.

@@ -4889,24 +4899,12 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
            return nested_vmx_failValid(vcpu,
                    VMXERR_VMWRITE_READ_ONLY_VMCS_COMPONENT);
 
-       if (!is_guest_mode(vcpu)) {
-               vmcs12 = get_vmcs12(vcpu);
-
-               /*
-                * Ensure vmcs12 is up-to-date before any VMWRITE that dirties
-                * vmcs12, else we may crush a field or consume a stale value.
-                */
-               if (!is_shadow_field_rw(field))
-                       copy_vmcs02_to_vmcs12_rare(vcpu, vmcs12);
-       } else {
-               /*
-                * When vmcs->vmcs_link_pointer is -1ull, any VMWRITE
-                * to shadowed-field sets the ALU flags for VMfailInvalid.
-                */
-               if (get_vmcs12(vcpu)->vmcs_link_pointer == -1ull)
-                       return nested_vmx_failInvalid(vcpu);
-               vmcs12 = get_shadow_vmcs12(vcpu);
-       }
+       /*
+        * Ensure vmcs12 is up-to-date before any VMWRITE that dirties
+        * vmcs12, else we may crush a field or consume a stale value.
+        */
+       if (!is_guest_mode(vcpu) && !is_shadow_field_rw(field))
+               copy_vmcs02_to_vmcs12_rare(vcpu, vmcs12);
 
    offset = vmcs_field_to_offset(field);
    if (offset < 0)

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

... and also, do you have a matching kvm-unit-tests patch?

I'll put one together, along with a test that shows the current priority inversion between read-only and unsupported VMCS fields.

I can't figure out how to clear IA32_VMX_MISC[bit 29] in qemu, so I'm going to add the test to tools/testing/selftests/kvm instead.

Please don’t.

I wish that we keep clear separation between kvm-unit-tests and self-tests. In the sense that kvm-unit-tests tests for correct CPU behaviour semantics and self-tests tests for correctness of KVM userspace API.

In the future, I wish to change kvm-unit-tests to cpu-unit-tests. As there is no real connection to KVM. It’s a bunch of tests that can be run on top of any CPU Implementation (weather vCPU by some hypervisor or bare-metal CPU) and
test for it’s semantics.
I have already used this to find semantic issues on Hyper-V vCPU implementation for example.

Quoted 1 line

Did you use for the matter the “infrastructure” that I added?

No. It’s possible to just change QEMU to run with WHPX.
But it’s true that the “infra” you added for running on Bare-Metal should work as-well. This is why I wish to change kvm-unit-tests to cpu-unit-tests. :)

Quoted 2 lines

I'll put one together, along with a test that shows the current priority inversion between read-only and unsupported VMCS fields.

I can't figure out how to clear IA32_VMX_MISC[bit 29] in qemu, so I'm going to add the test to tools/testing/selftests/kvm instead.

Quoted 2 lines · Jim Mattson

Quoted 2 lines

I'll put one together, along with a test that shows the current priority inversion between read-only and unsupported VMCS fields.

Quoted 3 lines

I can't figure out how to clear IA32_VMX_MISC[bit 29] in qemu, so I'm going to add the test to tools/testing/selftests/kvm instead.

With the next version of QEMU it will be "-cpu
host,-vmx-vmwrite-vmexit-fields".

Paolo

Quoted 4 lines · Paolo Bonzini

Quoted 2 lines · Jim Mattson

Quoted 2 lines

I'll put one together, along with a test that shows the current priority inversion between read-only and unsupported VMCS fields.

Quoted 6 lines

Quoted 3 lines

I can't figure out how to clear IA32_VMX_MISC[bit 29] in qemu, so I'm going to add the test to tools/testing/selftests/kvm instead.

With the next version of QEMU it will be "-cpu
host,-vmx-vmwrite-vmexit-fields".

Or, presumably, -cpu Westmere?