| Message ID | 20191204214027.85958-1-jmattson@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field | expand |
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,
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
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 >
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 > >
> 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
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.
> 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?
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
> 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
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?
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
> 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.
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,
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(-)