[PATCH] KVM: nSVM: prepare guest save area while is_guest_mode is true

[PATCH] KVM: nSVM: prepare guest save area while is_guest_mode is true

[Paolo Bonzini]

Right now, enter_svm_guest_mode is calling nested_prepare_vmcb_save and
nested_prepare_vmcb_control.  This results in is_guest_mode being false
until the end of nested_prepare_vmcb_control.

This is a problem because nested_prepare_vmcb_save can in turn cause
changes to the intercepts and these have to be applied to the "host VMCB"
(stored in svm->nested.hsave) and then merged with the VMCB12 intercepts
into svm->vmcb.

In particular, without this change we forget to set the CR0 read and CR0
write intercepts when running a real mode L2 guest with NPT disabled.
The guest is therefore able to see the CR0.PG bit that KVM sets to
enable "paged real mode".  This patch fixes the svm.flat mode_switch
test case with npt=0.  There are no other problematic calls in
nested_prepare_vmcb_save.

The bug is present since commit 06fc7772690d ("KVM: SVM: Activate nested
state only when guest state is complete", 2010-04-25).  Unfortunately,
it is not clear from the commit message what issue exactly led to the
change back then.  It was probably related to svm_set_cr0 however because
the patch series cover letter[1] mentioned lazy FPU switching.

[1] https://lore.kernel.org/kvm/1266493115-28386-1-git-send-email-joerg.roedel@amd.com/

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/svm/nested.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 92d3aaaac612..35891d9a1099 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -469,8 +469,8 @@ int enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb12_gpa,
 
 	svm->nested.vmcb12_gpa = vmcb12_gpa;
 	load_nested_vmcb_control(svm, &vmcb12->control);
-	nested_prepare_vmcb_save(svm, vmcb12);
 	nested_prepare_vmcb_control(svm);
+	nested_prepare_vmcb_save(svm, vmcb12);
 
 	ret = nested_svm_load_cr3(&svm->vcpu, vmcb12->save.cr3,
 				  nested_npt_enabled(svm));
-- 
2.26.2

Re: [PATCH] KVM: nSVM: prepare guest save area while is_guest_mode is true

[Sean Christopherson]

On Thu, Feb 18, 2021, Paolo Bonzini wrote:
> Right now, enter_svm_guest_mode is calling nested_prepare_vmcb_save and
> nested_prepare_vmcb_control.  This results in is_guest_mode being false
> until the end of nested_prepare_vmcb_control.
> 
> This is a problem because nested_prepare_vmcb_save can in turn cause
> changes to the intercepts and these have to be applied to the "host VMCB"
> (stored in svm->nested.hsave) and then merged with the VMCB12 intercepts
> into svm->vmcb.
> 
> In particular, without this change we forget to set the CR0 read and CR0
> write intercepts when running a real mode L2 guest with NPT disabled.
> The guest is therefore able to see the CR0.PG bit that KVM sets to
> enable "paged real mode".  This patch fixes the svm.flat mode_switch
> test case with npt=0.  There are no other problematic calls in
> nested_prepare_vmcb_save.

It might be worth explicitly pointing out that get_host_vmcb() in
svm_clr_intercept() and svm_set_intercept() will grab the wrong VMCB.
 
> The bug is present since commit 06fc7772690d ("KVM: SVM: Activate nested
> state only when guest state is complete", 2010-04-25).  Unfortunately,
> it is not clear from the commit message what issue exactly led to the
> change back then.  It was probably related to svm_set_cr0 however because
> the patch series cover letter[1] mentioned lazy FPU switching.

Aha!  It was indeed related to svm_set_cr0().  Specifically, the next patch,
commit 66a562f7e257 ("KVM: SVM: Make lazy FPU switching work with nested svm"),
added is_nested() checks in update_cr0_intercept() to merge L1's intercepts with
L0's intercepts.

I dug through all other is_nested() usage, none of them were reachable via the
world switch logic.

> [1] https://lore.kernel.org/kvm/1266493115-28386-1-git-send-email-joerg.roedel@amd.com/
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Sean Christopherson <seanjc@google.com>

> ---
>  arch/x86/kvm/svm/nested.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 92d3aaaac612..35891d9a1099 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -469,8 +469,8 @@ int enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb12_gpa,
>  
>  	svm->nested.vmcb12_gpa = vmcb12_gpa;
>  	load_nested_vmcb_control(svm, &vmcb12->control);
> -	nested_prepare_vmcb_save(svm, vmcb12);
>  	nested_prepare_vmcb_control(svm);
> +	nested_prepare_vmcb_save(svm, vmcb12);
>  
>  	ret = nested_svm_load_cr3(&svm->vcpu, vmcb12->save.cr3,
>  				  nested_npt_enabled(svm));
> -- 
> 2.26.2
> 

Re: [PATCH] KVM: nSVM: prepare guest save area while is_guest_mode is true

[Paolo Bonzini]

On 18/02/21 18:42, Sean Christopherson wrote:
>> The bug is present since commit 06fc7772690d ("KVM: SVM: Activate nested
>> state only when guest state is complete", 2010-04-25).  Unfortunately,
>> it is not clear from the commit message what issue exactly led to the
>> change back then.  It was probably related to svm_set_cr0 however because
>> the patch series cover letter[1] mentioned lazy FPU switching.
>
> Aha!  It was indeed related to svm_set_cr0().  Specifically, the next patch,
> commit 66a562f7e257 ("KVM: SVM: Make lazy FPU switching work with nested svm"),
> added is_nested() checks in update_cr0_intercept() to merge L1's intercepts with
> L0's intercepts.

Yeah, the problem is I don't understand why 06fc7772690d fixed things in 
11 year old KVM instead of breaking them, because effectively this patch 
is reverting it.

I don't care _that_ much because so much has changed since then; the 
world switch logic is abstracted better nowadays, and it is easier to 
review the change.  But it is weird, nevertheless.

Paolo

Re: [PATCH] KVM: nSVM: prepare guest save area while is_guest_mode is true

[Sean Christopherson]

On Thu, Feb 18, 2021, Paolo Bonzini wrote:
> On 18/02/21 18:42, Sean Christopherson wrote:
> > > The bug is present since commit 06fc7772690d ("KVM: SVM: Activate nested
> > > state only when guest state is complete", 2010-04-25).  Unfortunately,
> > > it is not clear from the commit message what issue exactly led to the
> > > change back then.  It was probably related to svm_set_cr0 however because
> > > the patch series cover letter[1] mentioned lazy FPU switching.
> > 
> > Aha!  It was indeed related to svm_set_cr0().  Specifically, the next patch,
> > commit 66a562f7e257 ("KVM: SVM: Make lazy FPU switching work with nested svm"),
> > added is_nested() checks in update_cr0_intercept() to merge L1's intercepts with
> > L0's intercepts.
> 
> Yeah, the problem is I don't understand why 06fc7772690d fixed things in 11
> year old KVM instead of breaking them, because effectively this patch is
> reverting it.

11 year old KVM didn't grab a different VMCB when updating the intercepts, it
had already copied/merged L1's stuff to L0's VMCB, and then updated L0's VMCB
regardless of is_nested().

> I don't care _that_ much because so much has changed since then; the world
> switch logic is abstracted better nowadays, and it is easier to review the
> change.  But it is weird, nevertheless.
> 
> Paolo
> 

Re: [PATCH] KVM: nSVM: prepare guest save area while is_guest_mode is true

[Paolo Bonzini]

On 18/02/21 19:12, Sean Christopherson wrote:
>> Yeah, the problem is I don't understand why 06fc7772690d fixed things in 11
>> year old KVM instead of breaking them, because effectively this patch is
>> reverting it.
>
> 11 year old KVM didn't grab a different VMCB when updating the intercepts, it
> had already copied/merged L1's stuff to L0's VMCB, and then updated L0's VMCB
> regardless of is_nested().

Ok, so the bug was introduced when adding recalc_intercepts, which threw 
away the intercept manipulations that svm_set_cr0 had done in the 
meanwhile.  That's commit 384c63684397 ("KVM: SVM: Add function to 
recalculate intercept masks", 2011-01-12).

That piece of information makes me feel less uneasy about missing 
something.  recalc_intercepts has been there for a long time, but not as 
long as 06fc7772690d.

Paolo

Re: [PATCH] KVM: nSVM: prepare guest save area while is_guest_mode is true

[Vitaly Kuznetsov]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Right now, enter_svm_guest_mode is calling nested_prepare_vmcb_save and
> nested_prepare_vmcb_control.  This results in is_guest_mode being false
> until the end of nested_prepare_vmcb_control.
>
> This is a problem because nested_prepare_vmcb_save can in turn cause
> changes to the intercepts and these have to be applied to the "host VMCB"
> (stored in svm->nested.hsave) and then merged with the VMCB12 intercepts
> into svm->vmcb.
>
> In particular, without this change we forget to set the CR0 read and CR0
> write intercepts when running a real mode L2 guest with NPT disabled.
> The guest is therefore able to see the CR0.PG bit that KVM sets to
> enable "paged real mode".  This patch fixes the svm.flat mode_switch
> test case with npt=0.  There are no other problematic calls in
> nested_prepare_vmcb_save.
>
> The bug is present since commit 06fc7772690d ("KVM: SVM: Activate nested
> state only when guest state is complete", 2010-04-25).  Unfortunately,
> it is not clear from the commit message what issue exactly led to the
> change back then.  It was probably related to svm_set_cr0 however because
> the patch series cover letter[1] mentioned lazy FPU switching.
>
> [1] https://lore.kernel.org/kvm/1266493115-28386-1-git-send-email-joerg.roedel@amd.com/
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/svm/nested.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 92d3aaaac612..35891d9a1099 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -469,8 +469,8 @@ int enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb12_gpa,
>  
>  	svm->nested.vmcb12_gpa = vmcb12_gpa;
>  	load_nested_vmcb_control(svm, &vmcb12->control);
> -	nested_prepare_vmcb_save(svm, vmcb12);
>  	nested_prepare_vmcb_control(svm);
> +	nested_prepare_vmcb_save(svm, vmcb12);
>  
>  	ret = nested_svm_load_cr3(&svm->vcpu, vmcb12->save.cr3,
>  				  nested_npt_enabled(svm));

For the record,

this seems to fix the bug when Gen2 guests were not booting on Hyper-V
on KVM (SVM). They boot now, thanks!

Tested-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly