[PATCH 0/4] nSVM: fix L0 crash if L2 has shutdown condtion which L1 doesn't intercept

[PATCH 0/4] nSVM: fix L0 crash if L2 has shutdown condtion which L1 doesn't intercept

[Maxim Levitsky]

Recently while trying to fix some unit tests I found another CVE in SVM nested code.

In 'shutdown_interception' vmexit handler we call kvm_vcpu_reset.

However if running nested and L1 doesn't intercept shutdown, we will still end
up running this function and trigger a bug in it.

The bug is that this function resets the 'vcpu->arch.hflags' without properly
leaving the nested state, which leaves the vCPU in inconsistent state, which
later triggers a kernel panic in SVM code.

The same bug can likely be triggered by sending INIT via local apic to a vCPU
which runs a nested guest.

On VMX we are lucky that the issue can't happen because VMX always intercepts
triple faults, thus triple fault in L2 will always be redirected to L1.
Plus the 'handle_triple_fault' of VMX doesn't reset the vCPU.

INIT IPI can't happen on VMX either because INIT events are masked while in
VMX mode.

Best regards,
	Maxim Levitsky

Maxim Levitsky (4):
  KVM: x86: nSVM: leave nested mode on vCPU free
  KVM: x86: nSVM: harden svm_free_nested against freeing vmcb02 while
    still in use
  KVM: x86: add kvm_leave_nested
  KVM: x86: forcibly leave nested mode on vCPU reset

 arch/x86/kvm/svm/nested.c | 6 +++---
 arch/x86/kvm/svm/svm.c    | 1 +
 arch/x86/kvm/vmx/nested.c | 3 ---
 arch/x86/kvm/x86.c        | 9 ++++++++-
 4 files changed, 12 insertions(+), 7 deletions(-)

-- 
2.26.3

[PATCH 1/4] KVM: x86: nSVM: leave nested mode on vCPU free

[Maxim Levitsky]

If the VM was terminated while nested, we free the nested state
while the vCPU still is in nested mode.

Soon a warning will be added for this condition.

Cc: stable@vger.kernel.org
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/svm/svm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 58f0077d935799..958faa8807f52b 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1439,6 +1439,7 @@ static void svm_vcpu_free(struct kvm_vcpu *vcpu)
 	 */
 	svm_clear_current_vmcb(svm->vmcb);
 
+	svm_leave_nested(vcpu);
 	svm_free_nested(svm);
 
 	sev_free_vcpu(vcpu);
-- 
2.26.3

[PATCH 2/4] KVM: x86: nSVM: harden svm_free_nested against freeing vmcb02 while still in use

[Maxim Levitsky]

Make sure that KVM uses vmcb01 before freeing nested state,
and warn if that is not the case.

This is a minimal fix for CVE-2022-3344 making the kernel
print a warning instead of a kernel panic.

Cc: stable@vger.kernel.org
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/svm/nested.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 4c620999d230a5..b02a3a1792f194 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -1125,6 +1125,9 @@ void svm_free_nested(struct vcpu_svm *svm)
 	if (!svm->nested.initialized)
 		return;
 
+	if (WARN_ON_ONCE(svm->vmcb != svm->vmcb01.ptr))
+		svm_switch_vmcb(svm, &svm->vmcb01);
+
 	svm_vcpu_free_msrpm(svm->nested.msrpm);
 	svm->nested.msrpm = NULL;
 
-- 
2.26.3

[PATCH 3/4] KVM: x86: add kvm_leave_nested

[Maxim Levitsky]

Wrap a call to nested_ops->leave_nested into a function.

Cc: stable@vger.kernel.org
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/svm/nested.c | 3 ---
 arch/x86/kvm/vmx/nested.c | 3 ---
 arch/x86/kvm/x86.c        | 8 +++++++-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index b02a3a1792f194..7354f0035a691d 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -1146,9 +1146,6 @@ void svm_free_nested(struct vcpu_svm *svm)
 	svm->nested.initialized = false;
 }
 
-/*
- * Forcibly leave nested mode in order to be able to reset the VCPU later on.
- */
 void svm_leave_nested(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 8f67a9c4a28706..a66c5e276ab408 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -6440,9 +6440,6 @@ static int vmx_get_nested_state(struct kvm_vcpu *vcpu,
 	return kvm_state.size;
 }
 
-/*
- * Forcibly leave nested mode in order to be able to reset the VCPU later on.
- */
 void vmx_leave_nested(struct kvm_vcpu *vcpu)
 {
 	if (is_guest_mode(vcpu)) {
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4bd5f8a751de91..d86a8aae1471d3 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -628,6 +628,12 @@ static void kvm_queue_exception_vmexit(struct kvm_vcpu *vcpu, unsigned int vecto
 	ex->payload = payload;
 }
 
+/* Forcibly leave the nested mode in cases like a vCPU reset */
+static void kvm_leave_nested(struct kvm_vcpu *vcpu)
+{
+	kvm_x86_ops.nested_ops->leave_nested(vcpu);
+}
+
 static void kvm_multiple_exception(struct kvm_vcpu *vcpu,
 		unsigned nr, bool has_error, u32 error_code,
 	        bool has_payload, unsigned long payload, bool reinject)
@@ -5200,7 +5206,7 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
 
 	if (events->flags & KVM_VCPUEVENT_VALID_SMM) {
 		if (!!(vcpu->arch.hflags & HF_SMM_MASK) != events->smi.smm) {
-			kvm_x86_ops.nested_ops->leave_nested(vcpu);
+			kvm_leave_nested(vcpu);
 			kvm_smm_changed(vcpu, events->smi.smm);
 		}
 
-- 
2.26.3

[PATCH 4/4] KVM: x86: forcibly leave nested mode on vCPU reset

[Maxim Levitsky]

While not obivous, kvm_vcpu_reset leaves the nested mode by
clearing 'vcpu->arch.hflags' but it does so without all the
required housekeeping.

This makes SVM and VMX continue to use vmcs02/vmcb02 while
the cpu is not in nested mode.

In particular, in SVM code, it makes the 'svm_free_nested'
free the vmcb02, while still in use, which later triggers
use after free and a kernel crash.

This issue is assigned CVE-2022-3344

Cc: stable@vger.kernel.org
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/x86.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d86a8aae1471d3..313c4a6dc65e45 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11931,6 +11931,7 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 	WARN_ON_ONCE(!init_event &&
 		     (old_cr0 || kvm_read_cr3(vcpu) || kvm_read_cr4(vcpu)));
 
+	kvm_leave_nested(vcpu);
 	kvm_lapic_reset(vcpu, init_event);
 
 	vcpu->arch.hflags = 0;
-- 
2.26.3

Re: [PATCH 4/4] KVM: x86: forcibly leave nested mode on vCPU reset

[Sean Christopherson]

On Thu, Oct 20, 2022, Maxim Levitsky wrote:
> While not obivous, kvm_vcpu_reset leaves the nested mode by

Please add () when referencing function, and wrap closer to ~75 chars.

> clearing 'vcpu->arch.hflags' but it does so without all the
> required housekeeping.
> 
> This makes SVM and VMX continue to use vmcs02/vmcb02 while

This bug should be impossible to hit on VMX as INIT and TRIPLE_FAULT unconditionally
cause VM-Exit, i.e. will always be forwarded to L1.

> the cpu is not in nested mode.

Can you add a blurb to call out exactly how this bug can be triggered?  Doesn't
take much effort to suss out the "how", but it'd be nice to capture that info in
the changelog.

> In particular, in SVM code, it makes the 'svm_free_nested'
> free the vmcb02, while still in use, which later triggers
> use after free and a kernel crash.
> 
> This issue is assigned CVE-2022-3344
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  arch/x86/kvm/x86.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index d86a8aae1471d3..313c4a6dc65e45 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -11931,6 +11931,7 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>  	WARN_ON_ONCE(!init_event &&
>  		     (old_cr0 || kvm_read_cr3(vcpu) || kvm_read_cr4(vcpu)));
>  
> +	kvm_leave_nested(vcpu);

Not a big deal, especially if/when nested_ops are turned into static_calls, but
at the same time it's quite easy to do:

	if (is_guest_mode(vcpu))
		kvm_leave_nested(vcpu);

I think it's worth adding a comment explaining how this can happen, and to also
call out that EFER is cleared on INIT, i.e. that virtualization is disabled due
to EFER.SVME=0.  Unsurprisingly, I don't see anything in the APM that explicitly
states what happens if INIT occurs in guest mode, i.e. it's not immediately obvious
that forcing the vCPU back to L1 is architecturally correct.


>  	kvm_lapic_reset(vcpu, init_event);
>  
>  	vcpu->arch.hflags = 0;

Maybe add a WARN above this to try and detect other potential issues?  Kinda silly,
but it'd at least help draw attention to the importance of hflags.

E.g. this?

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4bd5f8a751de..c50fa0751a0b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11915,6 +11915,15 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
        unsigned long old_cr0 = kvm_read_cr0(vcpu);
        unsigned long new_cr0;
 
+       /*
+        * SVM doesn't unconditionally VM-Exit on INIT and SHUTDOWN, thus it's
+        * possible to INIT the vCPU while L2 is active.  Force the vCPU back
+        * into L1 as EFER.SVME is cleared on INIT (along with all other EFER
+        * bits), i.e. virtualization is disabled.
+        */
+       if (is_guest_mode(vcpu))
+               kvm_leave_nested(vcpu);
+
        /*
         * Several of the "set" flows, e.g. ->set_cr0(), read other registers
         * to handle side effects.  RESET emulation hits those flows and relies
@@ -11927,6 +11936,7 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 
        kvm_lapic_reset(vcpu, init_event);
 
+       WARN_ON_ONCE(is_guest_mode(vcpu) || is_smm(vcpu));
        vcpu->arch.hflags = 0;
 
        vcpu->arch.smi_pending = 0;

Re: [PATCH 4/4] KVM: x86: forcibly leave nested mode on vCPU reset

[Maxim Levitsky]

On Thu, 2022-10-20 at 15:33 +0000, Sean Christopherson wrote:
> On Thu, Oct 20, 2022, Maxim Levitsky wrote:
> > While not obivous, kvm_vcpu_reset leaves the nested mode by
> 
> Please add () when referencing function, and wrap closer to ~75 chars.
> 
> > clearing 'vcpu->arch.hflags' but it does so without all the
> > required housekeeping.
> > 
> > This makes SVM and VMX continue to use vmcs02/vmcb02 while
> 
> This bug should be impossible to hit on VMX as INIT and TRIPLE_FAULT unconditionally
> cause VM-Exit, i.e. will always be forwarded to L1.

True I guess as I found out as well, in VMX the physical CPU can't be reset while
in guest mode. I'll update the changelog.

> 
> > the cpu is not in nested mode.
> 
> Can you add a blurb to call out exactly how this bug can be triggered?  Doesn't
> take much effort to suss out the "how", but it'd be nice to capture that info in
> the changelog.

I will add (in another patch) a selftest for this.

> 
> > In particular, in SVM code, it makes the 'svm_free_nested'
> > free the vmcb02, while still in use, which later triggers
> > use after free and a kernel crash.
> > 
> > This issue is assigned CVE-2022-3344
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  arch/x86/kvm/x86.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index d86a8aae1471d3..313c4a6dc65e45 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -11931,6 +11931,7 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> >         WARN_ON_ONCE(!init_event &&
> >                      (old_cr0 || kvm_read_cr3(vcpu) || kvm_read_cr4(vcpu)));
> >  
> > +       kvm_leave_nested(vcpu);
> 
> Not a big deal, especially if/when nested_ops are turned into static_calls, but
> at the same time it's quite easy to do:
> 
>         if (is_guest_mode(vcpu))
>                 kvm_leave_nested(vcpu);
> 
> I think it's worth adding a comment explaining how this can happen, and to also
> call out that EFER is cleared on INIT, i.e. that virtualization is disabled due
> to EFER.SVME=0.  Unsurprisingly, I don't see anything in the APM that explicitly
> states what happens if INIT occurs in guest mode, i.e. it's not immediately obvious
> that forcing the vCPU back to L1 is architecturally correct.
> 
> 
> >         kvm_lapic_reset(vcpu, init_event);
> >  
> >         vcpu->arch.hflags = 0;
> 
> Maybe add a WARN above this to try and detect other potential issues?  Kinda silly,
> but it'd at least help draw attention to the importance of hflags.
> 
> E.g. this?
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 4bd5f8a751de..c50fa0751a0b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -11915,6 +11915,15 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>         unsigned long old_cr0 = kvm_read_cr0(vcpu);
>         unsigned long new_cr0;
>  
> +       /*
> +        * SVM doesn't unconditionally VM-Exit on INIT and SHUTDOWN, thus it's
> +        * possible to INIT the vCPU while L2 is active.  Force the vCPU back
> +        * into L1 as EFER.SVME is cleared on INIT (along with all other EFER
> +        * bits), i.e. virtualization is disabled.
> +        */


> +       if (is_guest_mode(vcpu))
> +               kvm_leave_nested(vcpu);
> +
>         /*
>          * Several of the "set" flows, e.g. ->set_cr0(), read other registers
>          * to handle side effects.  RESET emulation hits those flows and relies
> @@ -11927,6 +11936,7 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>  
>         kvm_lapic_reset(vcpu, init_event);
>  
> +       WARN_ON_ONCE(is_guest_mode(vcpu) || is_smm(vcpu));
>         vcpu->arch.hflags = 0;
>  
>         vcpu->arch.smi_pending = 0;
> 

Best regards,
	Maxim Levitsky