* [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field
@ 2019-12-04 21:40 Jim Mattson
2019-12-05 11:28 ` Vitaly Kuznetsov
2019-12-05 11:45 ` Paolo Bonzini
0 siblings, 2 replies; 13+ messages in thread
From: Jim Mattson @ 2019-12-04 21:40 UTC (permalink / raw)
To: kvm, Paolo Bonzini; +Cc: Jim Mattson, Liran Alon
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 <jmattson@google.com>
Cc: Liran Alon <liran.alon@oracle.com>
---
arch/x86/kvm/vmx/nested.c | 38 +++++++++++++++++++-------------------
1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 4aea7d304beb..146e1b40c69f 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -4864,6 +4864,25 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
if (vmx->nested.current_vmptr == -1ull)
return nested_vmx_failInvalid(vcpu);
+ 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);
+ }
+
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,
--
2.24.0.393.g34dc348eaf-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field
2019-12-04 21:40 [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field Jim Mattson
@ 2019-12-05 11:28 ` Vitaly Kuznetsov
2019-12-05 11:45 ` Paolo Bonzini
1 sibling, 0 replies; 13+ messages in thread
From: Vitaly Kuznetsov @ 2019-12-05 11:28 UTC (permalink / raw)
To: Jim Mattson; +Cc: Liran Alon, kvm, Paolo Bonzini
Jim Mattson <jmattson@google.com> writes:
> 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 <jmattson@google.com>
> Cc: Liran Alon <liran.alon@oracle.com>
> ---
> arch/x86/kvm/vmx/nested.c | 38 +++++++++++++++++++-------------------
> 1 file changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 4aea7d304beb..146e1b40c69f 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -4864,6 +4864,25 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
> if (vmx->nested.current_vmptr == -1ull)
> return nested_vmx_failInvalid(vcpu);
>
> + 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);
But (unless I'm missing some pre-requisite patch) we haven't set 'field'
yet, it happens later when we do
field = kvm_register_readl(vcpu, (((vmx_instruction_info) >> 28) & 0xf));
> + } 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,
--
Vitaly
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field
2019-12-04 21:40 [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field Jim Mattson
2019-12-05 11:28 ` Vitaly Kuznetsov
@ 2019-12-05 11:45 ` Paolo Bonzini
2019-12-05 13:11 ` Jim Mattson
1 sibling, 1 reply; 13+ messages in thread
From: Paolo Bonzini @ 2019-12-05 11:45 UTC (permalink / raw)
To: Jim Mattson, kvm; +Cc: Liran Alon
On 04/12/19 22:40, Jim Mattson wrote:
> 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 <jmattson@google.com>
> Cc: Liran Alon <liran.alon@oracle.com>
> ---
> 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 <pbonzini@redhat.com>
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 <jmattson@google.com>
Cc: Liran Alon <liran.alon@oracle.com>
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 4aea7d304beb..c080a879b95d 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field
2019-12-05 11:45 ` Paolo Bonzini
@ 2019-12-05 13:11 ` Jim Mattson
2019-12-05 21:30 ` Jim Mattson
0 siblings, 1 reply; 13+ messages in thread
From: Jim Mattson @ 2019-12-05 13:11 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kvm list, Liran Alon
On Thu, Dec 5, 2019 at 3:46 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 04/12/19 22:40, Jim Mattson wrote:
> > 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 <jmattson@google.com>
> > Cc: Liran Alon <liran.alon@oracle.com>
> > ---
> > 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.
> ---------------- 8< -----------------------
> From 3b9d87060e800ffae2bd19da94ede05018066c87 Mon Sep 17 00:00:00 2001
> From: Paolo Bonzini <pbonzini@redhat.com>
> 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.
Okay.
> Fixes: 6d894f498f5d1 ("KVM: nVMX: vmread/vmwrite: Use shadow vmcs12 if running L2")
> Signed-off-by: Jim Mattson <jmattson@google.com>
> Cc: Liran Alon <liran.alon@oracle.com>
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 4aea7d304beb..c080a879b95d 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -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?
I'll put one together, along with a test that shows the current
priority inversion between read-only and unsupported VMCS fields.
> Thanks,
>
> Paolo
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field
2019-12-05 13:11 ` Jim Mattson
@ 2019-12-05 21:30 ` Jim Mattson
2019-12-05 21:54 ` Liran Alon
2019-12-09 16:12 ` Paolo Bonzini
0 siblings, 2 replies; 13+ messages in thread
From: Jim Mattson @ 2019-12-05 21:30 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kvm list, Liran Alon
On Thu, Dec 5, 2019 at 5:11 AM Jim Mattson <jmattson@google.com> wrote:
>
> On Thu, Dec 5, 2019 at 3:46 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> > On 04/12/19 22:40, Jim Mattson wrote:
> > > 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 <jmattson@google.com>
> > > Cc: Liran Alon <liran.alon@oracle.com>
> > > ---
> > > 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.
>
> > ---------------- 8< -----------------------
> > From 3b9d87060e800ffae2bd19da94ede05018066c87 Mon Sep 17 00:00:00 2001
> > From: Paolo Bonzini <pbonzini@redhat.com>
> > 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.
>
> Okay.
>
> > Fixes: 6d894f498f5d1 ("KVM: nVMX: vmread/vmwrite: Use shadow vmcs12 if running L2")
> > Signed-off-by: Jim Mattson <jmattson@google.com>
> > Cc: Liran Alon <liran.alon@oracle.com>
> >
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index 4aea7d304beb..c080a879b95d 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -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?
>
> 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.
> > Thanks,
> >
> > Paolo
> >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field
2019-12-05 21:30 ` Jim Mattson
@ 2019-12-05 21:54 ` Liran Alon
2019-12-05 22:11 ` Jim Mattson
2019-12-09 15:28 ` Nadav Amit
2019-12-09 16:12 ` Paolo Bonzini
1 sibling, 2 replies; 13+ messages in thread
From: Liran Alon @ 2019-12-05 21:54 UTC (permalink / raw)
To: Jim Mattson; +Cc: Paolo Bonzini, kvm list
> On 5 Dec 2019, at 23:30, Jim Mattson <jmattson@google.com> wrote:
>
> On Thu, Dec 5, 2019 at 5:11 AM Jim Mattson <jmattson@google.com> wrote:
>>
>> On Thu, Dec 5, 2019 at 3:46 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>
>>> On 04/12/19 22:40, Jim Mattson wrote:
>>>> 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 <jmattson@google.com>
>>>> Cc: Liran Alon <liran.alon@oracle.com>
>>>> ---
>>>> 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.
>>
>>> ---------------- 8< -----------------------
>>> From 3b9d87060e800ffae2bd19da94ede05018066c87 Mon Sep 17 00:00:00 2001
>>> From: Paolo Bonzini <pbonzini@redhat.com>
>>> 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.
>>
>> Okay.
>>
>>> Fixes: 6d894f498f5d1 ("KVM: nVMX: vmread/vmwrite: Use shadow vmcs12 if running L2")
>>> Signed-off-by: Jim Mattson <jmattson@google.com>
>>> Cc: Liran Alon <liran.alon@oracle.com>
>>>
>>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>>> index 4aea7d304beb..c080a879b95d 100644
>>> --- a/arch/x86/kvm/vmx/nested.c
>>> +++ b/arch/x86/kvm/vmx/nested.c
>>> @@ -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?
>>
>> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field
2019-12-05 21:54 ` Liran Alon
@ 2019-12-05 22:11 ` Jim Mattson
2019-12-09 15:28 ` Nadav Amit
1 sibling, 0 replies; 13+ messages in thread
From: Jim Mattson @ 2019-12-05 22:11 UTC (permalink / raw)
To: Liran Alon; +Cc: Paolo Bonzini, kvm list
On Thu, Dec 5, 2019 at 1:54 PM Liran Alon <liran.alon@oracle.com> wrote:
>
>
>
> > On 5 Dec 2019, at 23:30, Jim Mattson <jmattson@google.com> wrote:
> >
> > On Thu, Dec 5, 2019 at 5:11 AM Jim Mattson <jmattson@google.com> wrote:
> >>
> >> On Thu, Dec 5, 2019 at 3:46 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>>
> >>> On 04/12/19 22:40, Jim Mattson wrote:
> >>>> 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 <jmattson@google.com>
> >>>> Cc: Liran Alon <liran.alon@oracle.com>
> >>>> ---
> >>>> 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.
> >>
> >>> ---------------- 8< -----------------------
> >>> From 3b9d87060e800ffae2bd19da94ede05018066c87 Mon Sep 17 00:00:00 2001
> >>> From: Paolo Bonzini <pbonzini@redhat.com>
> >>> 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.
> >>
> >> Okay.
> >>
> >>> Fixes: 6d894f498f5d1 ("KVM: nVMX: vmread/vmwrite: Use shadow vmcs12 if running L2")
> >>> Signed-off-by: Jim Mattson <jmattson@google.com>
> >>> Cc: Liran Alon <liran.alon@oracle.com>
> >>>
> >>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> >>> index 4aea7d304beb..c080a879b95d 100644
> >>> --- a/arch/x86/kvm/vmx/nested.c
> >>> +++ b/arch/x86/kvm/vmx/nested.c
> >>> @@ -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?
> >>
> >> 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
Okay. Given the high barrier to entry, I will not be providing a test
at this time.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field
2019-12-05 21:54 ` Liran Alon
2019-12-05 22:11 ` Jim Mattson
@ 2019-12-09 15:28 ` Nadav Amit
2019-12-09 22:58 ` Liran Alon
1 sibling, 1 reply; 13+ messages in thread
From: Nadav Amit @ 2019-12-09 15:28 UTC (permalink / raw)
To: Liran Alon; +Cc: Jim Mattson, Paolo Bonzini, kvm list
> On Dec 5, 2019, at 1:54 PM, Liran Alon <liran.alon@oracle.com> wrote:
>
>
>
>> On 5 Dec 2019, at 23:30, Jim Mattson <jmattson@google.com> wrote:
>>
>> On Thu, Dec 5, 2019 at 5:11 AM Jim Mattson <jmattson@google.com> wrote:
>>> On Thu, Dec 5, 2019 at 3:46 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>> On 04/12/19 22:40, Jim Mattson wrote:
>>>>> 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 <jmattson@google.com>
>>>>> Cc: Liran Alon <liran.alon@oracle.com>
>>>>> ---
>>>>> 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.
>>>
>>>> ---------------- 8< -----------------------
>>>> From 3b9d87060e800ffae2bd19da94ede05018066c87 Mon Sep 17 00:00:00 2001
>>>> From: Paolo Bonzini <pbonzini@redhat.com>
>>>> 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.
>>>
>>> Okay.
>>>
>>>> Fixes: 6d894f498f5d1 ("KVM: nVMX: vmread/vmwrite: Use shadow vmcs12 if running L2")
>>>> Signed-off-by: Jim Mattson <jmattson@google.com>
>>>> Cc: Liran Alon <liran.alon@oracle.com>
>>>>
>>>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>>>> index 4aea7d304beb..c080a879b95d 100644
>>>> --- a/arch/x86/kvm/vmx/nested.c
>>>> +++ b/arch/x86/kvm/vmx/nested.c
>>>> @@ -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?
>>>
>>> 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.
Did you use for the matter the “infrastructure” that I added?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field
2019-12-05 21:30 ` Jim Mattson
2019-12-05 21:54 ` Liran Alon
@ 2019-12-09 16:12 ` Paolo Bonzini
2019-12-09 23:34 ` Jim Mattson
1 sibling, 1 reply; 13+ messages in thread
From: Paolo Bonzini @ 2019-12-09 16:12 UTC (permalink / raw)
To: Jim Mattson; +Cc: kvm list, Liran Alon
On 05/12/19 22:30, Jim Mattson wrote:
>> 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.
>
With the next version of QEMU it will be "-cpu
host,-vmx-vmwrite-vmexit-fields".
Paolo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field
2019-12-09 15:28 ` Nadav Amit
@ 2019-12-09 22:58 ` Liran Alon
2019-12-10 22:22 ` Nadav Amit
0 siblings, 1 reply; 13+ messages in thread
From: Liran Alon @ 2019-12-09 22:58 UTC (permalink / raw)
To: Nadav Amit; +Cc: Jim Mattson, Paolo Bonzini, kvm list
> On 9 Dec 2019, at 17:28, Nadav Amit <nadav.amit@gmail.com> wrote:
>
>> On Dec 5, 2019, at 1:54 PM, Liran Alon <liran.alon@oracle.com> wrote:
>>
>>
>>
>>> On 5 Dec 2019, at 23:30, Jim Mattson <jmattson@google.com> wrote:
>>>
>>> On Thu, Dec 5, 2019 at 5:11 AM Jim Mattson <jmattson@google.com> wrote:
>>>> On Thu, Dec 5, 2019 at 3:46 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>> On 04/12/19 22:40, Jim Mattson wrote:
>>>>>> 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 <jmattson@google.com>
>>>>>> Cc: Liran Alon <liran.alon@oracle.com>
>>>>>> ---
>>>>>> 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.
>>>>
>>>>> ---------------- 8< -----------------------
>>>>> From 3b9d87060e800ffae2bd19da94ede05018066c87 Mon Sep 17 00:00:00 2001
>>>>> From: Paolo Bonzini <pbonzini@redhat.com>
>>>>> 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.
>>>>
>>>> Okay.
>>>>
>>>>> Fixes: 6d894f498f5d1 ("KVM: nVMX: vmread/vmwrite: Use shadow vmcs12 if running L2")
>>>>> Signed-off-by: Jim Mattson <jmattson@google.com>
>>>>> Cc: Liran Alon <liran.alon@oracle.com>
>>>>>
>>>>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>>>>> index 4aea7d304beb..c080a879b95d 100644
>>>>> --- a/arch/x86/kvm/vmx/nested.c
>>>>> +++ b/arch/x86/kvm/vmx/nested.c
>>>>> @@ -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?
>>>>
>>>> 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.
>
> 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. :)
-Liran
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field
2019-12-09 16:12 ` Paolo Bonzini
@ 2019-12-09 23:34 ` Jim Mattson
2019-12-10 8:39 ` Paolo Bonzini
0 siblings, 1 reply; 13+ messages in thread
From: Jim Mattson @ 2019-12-09 23:34 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kvm list, Liran Alon
On Mon, Dec 9, 2019 at 8:12 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 05/12/19 22:30, Jim Mattson wrote:
> >> 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.
> >
>
> With the next version of QEMU it will be "-cpu
> host,-vmx-vmwrite-vmexit-fields".
>
> Paolo
Or, presumably, -cpu Westmere?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field
2019-12-09 23:34 ` Jim Mattson
@ 2019-12-10 8:39 ` Paolo Bonzini
0 siblings, 0 replies; 13+ messages in thread
From: Paolo Bonzini @ 2019-12-10 8:39 UTC (permalink / raw)
To: Jim Mattson; +Cc: kvm list, Liran Alon
On 10/12/19 00:34, Jim Mattson wrote:
> On Mon, Dec 9, 2019 at 8:12 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> On 05/12/19 22:30, Jim Mattson wrote:
>>>> 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.
>>>
>>
>> With the next version of QEMU it will be "-cpu
>> host,-vmx-vmwrite-vmexit-fields".
>
> Or, presumably, -cpu Westmere?
Yes, more precisely -cpu Westmere,+vmx because nested is still not the
default for named CPU models.
Paolo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field
2019-12-09 22:58 ` Liran Alon
@ 2019-12-10 22:22 ` Nadav Amit
0 siblings, 0 replies; 13+ messages in thread
From: Nadav Amit @ 2019-12-10 22:22 UTC (permalink / raw)
To: Liran Alon; +Cc: Jim Mattson, Paolo Bonzini, kvm list
> On Dec 9, 2019, at 2:58 PM, Liran Alon <liran.alon@oracle.com> wrote:
>
>
>
>> On 9 Dec 2019, at 17:28, Nadav Amit <nadav.amit@gmail.com> wrote:
>>
>>> On Dec 5, 2019, at 1:54 PM, Liran Alon <liran.alon@oracle.com> wrote:
>>>
>>>
>>>
>>>> On 5 Dec 2019, at 23:30, Jim Mattson <jmattson@google.com> wrote:
>>>>
>>>> On Thu, Dec 5, 2019 at 5:11 AM Jim Mattson <jmattson@google.com> wrote:
>>>>> On Thu, Dec 5, 2019 at 3:46 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>>> On 04/12/19 22:40, Jim Mattson wrote:
>>>>>>> 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 <jmattson@google.com>
>>>>>>> Cc: Liran Alon <liran.alon@oracle.com>
>>>>>>> ---
>>>>>>> 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.
>>>>>
>>>>>> ---------------- 8< -----------------------
>>>>>> From 3b9d87060e800ffae2bd19da94ede05018066c87 Mon Sep 17 00:00:00 2001
>>>>>> From: Paolo Bonzini <pbonzini@redhat.com>
>>>>>> 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.
>>>>>
>>>>> Okay.
>>>>>
>>>>>> Fixes: 6d894f498f5d1 ("KVM: nVMX: vmread/vmwrite: Use shadow vmcs12 if running L2")
>>>>>> Signed-off-by: Jim Mattson <jmattson@google.com>
>>>>>> Cc: Liran Alon <liran.alon@oracle.com>
>>>>>>
>>>>>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>>>>>> index 4aea7d304beb..c080a879b95d 100644
>>>>>> --- a/arch/x86/kvm/vmx/nested.c
>>>>>> +++ b/arch/x86/kvm/vmx/nested.c
>>>>>> @@ -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?
>>>>>
>>>>> 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.
>>
>> 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. :)
Thanks for the explanation. I may give QEMU+WHPX a try just to see how many
bugs it reports.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-12-10 22:22 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-04 21:40 [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field Jim Mattson
2019-12-05 11:28 ` Vitaly Kuznetsov
2019-12-05 11:45 ` Paolo Bonzini
2019-12-05 13:11 ` Jim Mattson
2019-12-05 21:30 ` Jim Mattson
2019-12-05 21:54 ` Liran Alon
2019-12-05 22:11 ` Jim Mattson
2019-12-09 15:28 ` Nadav Amit
2019-12-09 22:58 ` Liran Alon
2019-12-10 22:22 ` Nadav Amit
2019-12-09 16:12 ` Paolo Bonzini
2019-12-09 23:34 ` Jim Mattson
2019-12-10 8:39 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).