[PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field

[PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field

[Jim Mattson]

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

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

Re: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field

[Vitaly Kuznetsov]

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

Re: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field

[Paolo Bonzini]

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

Re: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field

[Jim Mattson]

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
>

Re: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field

[Jim Mattson]

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
> >

Re: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field

[Liran Alon]

> 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

Re: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field

[Jim Mattson]

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.

Re: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field

[Nadav Amit]

> 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?

Re: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field

[Liran Alon]

> 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

Re: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field

[Nadav Amit]

> 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.

Re: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field

[Paolo Bonzini]

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

Re: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field

[Jim Mattson]

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?

Re: [PATCH] kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field

[Paolo Bonzini]

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