* [PATCH 0/4] nSVM: fix L0 crash if L2 has shutdown condtion which L1 doesn't intercept @ 2022-10-20 9:30 Maxim Levitsky 2022-10-20 9:30 ` [PATCH 1/4] KVM: x86: nSVM: leave nested mode on vCPU free Maxim Levitsky ` (3 more replies) 0 siblings, 4 replies; 7+ messages in thread From: Maxim Levitsky @ 2022-10-20 9:30 UTC (permalink / raw) To: kvm Cc: linux-kernel, Paolo Bonzini, Borislav Petkov, Ingo Molnar, Sean Christopherson, x86, Thomas Gleixner, Dave Hansen, H. Peter Anvin, 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/4] KVM: x86: nSVM: leave nested mode on vCPU free 2022-10-20 9:30 [PATCH 0/4] nSVM: fix L0 crash if L2 has shutdown condtion which L1 doesn't intercept Maxim Levitsky @ 2022-10-20 9:30 ` Maxim Levitsky 2022-10-20 9:30 ` [PATCH 2/4] KVM: x86: nSVM: harden svm_free_nested against freeing vmcb02 while still in use Maxim Levitsky ` (2 subsequent siblings) 3 siblings, 0 replies; 7+ messages in thread From: Maxim Levitsky @ 2022-10-20 9:30 UTC (permalink / raw) To: kvm Cc: linux-kernel, Paolo Bonzini, Borislav Petkov, Ingo Molnar, Sean Christopherson, x86, Thomas Gleixner, Dave Hansen, H. Peter Anvin, Maxim Levitsky, stable 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/4] KVM: x86: nSVM: harden svm_free_nested against freeing vmcb02 while still in use 2022-10-20 9:30 [PATCH 0/4] nSVM: fix L0 crash if L2 has shutdown condtion which L1 doesn't intercept Maxim Levitsky 2022-10-20 9:30 ` [PATCH 1/4] KVM: x86: nSVM: leave nested mode on vCPU free Maxim Levitsky @ 2022-10-20 9:30 ` Maxim Levitsky 2022-10-20 9:30 ` [PATCH 3/4] KVM: x86: add kvm_leave_nested Maxim Levitsky 2022-10-20 9:30 ` [PATCH 4/4] KVM: x86: forcibly leave nested mode on vCPU reset Maxim Levitsky 3 siblings, 0 replies; 7+ messages in thread From: Maxim Levitsky @ 2022-10-20 9:30 UTC (permalink / raw) To: kvm Cc: linux-kernel, Paolo Bonzini, Borislav Petkov, Ingo Molnar, Sean Christopherson, x86, Thomas Gleixner, Dave Hansen, H. Peter Anvin, Maxim Levitsky, stable 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/4] KVM: x86: add kvm_leave_nested 2022-10-20 9:30 [PATCH 0/4] nSVM: fix L0 crash if L2 has shutdown condtion which L1 doesn't intercept Maxim Levitsky 2022-10-20 9:30 ` [PATCH 1/4] KVM: x86: nSVM: leave nested mode on vCPU free Maxim Levitsky 2022-10-20 9:30 ` [PATCH 2/4] KVM: x86: nSVM: harden svm_free_nested against freeing vmcb02 while still in use Maxim Levitsky @ 2022-10-20 9:30 ` Maxim Levitsky 2022-10-20 9:30 ` [PATCH 4/4] KVM: x86: forcibly leave nested mode on vCPU reset Maxim Levitsky 3 siblings, 0 replies; 7+ messages in thread From: Maxim Levitsky @ 2022-10-20 9:30 UTC (permalink / raw) To: kvm Cc: linux-kernel, Paolo Bonzini, Borislav Petkov, Ingo Molnar, Sean Christopherson, x86, Thomas Gleixner, Dave Hansen, H. Peter Anvin, Maxim Levitsky, stable 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 4/4] KVM: x86: forcibly leave nested mode on vCPU reset 2022-10-20 9:30 [PATCH 0/4] nSVM: fix L0 crash if L2 has shutdown condtion which L1 doesn't intercept Maxim Levitsky ` (2 preceding siblings ...) 2022-10-20 9:30 ` [PATCH 3/4] KVM: x86: add kvm_leave_nested Maxim Levitsky @ 2022-10-20 9:30 ` Maxim Levitsky 2022-10-20 15:33 ` Sean Christopherson 3 siblings, 1 reply; 7+ messages in thread From: Maxim Levitsky @ 2022-10-20 9:30 UTC (permalink / raw) To: kvm Cc: linux-kernel, Paolo Bonzini, Borislav Petkov, Ingo Molnar, Sean Christopherson, x86, Thomas Gleixner, Dave Hansen, H. Peter Anvin, Maxim Levitsky, stable 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 4/4] KVM: x86: forcibly leave nested mode on vCPU reset 2022-10-20 9:30 ` [PATCH 4/4] KVM: x86: forcibly leave nested mode on vCPU reset Maxim Levitsky @ 2022-10-20 15:33 ` Sean Christopherson 2022-10-25 13:37 ` Maxim Levitsky 0 siblings, 1 reply; 7+ messages in thread From: Sean Christopherson @ 2022-10-20 15:33 UTC (permalink / raw) To: Maxim Levitsky Cc: kvm, linux-kernel, Paolo Bonzini, Borislav Petkov, Ingo Molnar, x86, Thomas Gleixner, Dave Hansen, H. Peter Anvin, stable 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; ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 4/4] KVM: x86: forcibly leave nested mode on vCPU reset 2022-10-20 15:33 ` Sean Christopherson @ 2022-10-25 13:37 ` Maxim Levitsky 0 siblings, 0 replies; 7+ messages in thread From: Maxim Levitsky @ 2022-10-25 13:37 UTC (permalink / raw) To: Sean Christopherson Cc: kvm, linux-kernel, Paolo Bonzini, Borislav Petkov, Ingo Molnar, x86, Thomas Gleixner, Dave Hansen, H. Peter Anvin, stable 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-10-25 13:37 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-20 9:30 [PATCH 0/4] nSVM: fix L0 crash if L2 has shutdown condtion which L1 doesn't intercept Maxim Levitsky 2022-10-20 9:30 ` [PATCH 1/4] KVM: x86: nSVM: leave nested mode on vCPU free Maxim Levitsky 2022-10-20 9:30 ` [PATCH 2/4] KVM: x86: nSVM: harden svm_free_nested against freeing vmcb02 while still in use Maxim Levitsky 2022-10-20 9:30 ` [PATCH 3/4] KVM: x86: add kvm_leave_nested Maxim Levitsky 2022-10-20 9:30 ` [PATCH 4/4] KVM: x86: forcibly leave nested mode on vCPU reset Maxim Levitsky 2022-10-20 15:33 ` Sean Christopherson 2022-10-25 13:37 ` Maxim Levitsky
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).